Re: [PATCH v3 27/34] dmaengine: plx_dma: use channel BH helpers

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] plx_dma_process_desc() fails to disable bottom halves when acquiring ring_lock, leading to a spinlock deadlock if preempted by a softirq.
- [High] plx_dma_process_desc() and plx_dma_abort_desc() invoke client completion callbacks while holding the driver's ring_lock, causing a recursive deadlock if the client submits a new transaction.
- [High] Race condition in plx_dma_create() where request_irq() is called before plxdev->bar is initialized, leading to a potential NULL pointer dereference.
--

--- Patch [27]: [PATCH v3 27/34] dmaengine: plx_dma: use channel BH helpers ---
commit 3f31218d1705896b2087c850d94c4632503584c8
Author: Allen Pais <[email protected]>
Date:   Mon Aug 10 18:09:28 2026 +0000

    dmaengine: plx_dma: use channel BH helpers
    
    Replace the device tasklet with the per-channel BH helper so completion
    callbacks run via dmaengine_*_bh.

> diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
> index 84941a918b01f..a9a798c54ef15 100644
> --- a/drivers/dma/plx_dma.c
> +++ b/drivers/dma/plx_dma.c

[ ... ]

> @@ -241,9 +240,10 @@ static void plx_dma_stop(struct plx_dma_dev *plxdev)
>  	rcu_read_unlock();
>  }
>  
> -static void plx_dma_desc_task(struct tasklet_struct *t)
> +static void plx_dma_desc_task(struct dma_chan *chan)
>  {
> -	struct plx_dma_dev *plxdev = from_tasklet(plxdev, t, desc_task);
> +	struct plx_dma_dev *plxdev = container_of(chan, struct plx_dma_dev,
> +						  dma_chan);
>  
>  	plx_dma_process_desc(plxdev);

[Severity: High]
This isn't a bug introduced by this patch, but does plx_dma_process_desc()
leave bottom halves enabled while acquiring ring_lock?

If plx_dma_process_desc() is called from process context (like through
plx_dma_tx_status() or plx_dma_abort_desc()), it acquires ring_lock with
spin_lock():

	spin_lock(&plxdev->ring_lock);

If a hardware interrupt fires on the same CPU, plx_dma_isr() schedules
the BH work. When the ISR returns, the kernel will run the softirq before
returning to the preempted process context. The BH work (plx_dma_desc_task)
will then call plx_dma_process_desc() and try to acquire the already-held
ring_lock, causing a deadlock.

[Severity: High]
This is a pre-existing issue, but can invoking the client callback
synchronously while holding ring_lock cause a recursive deadlock?

In plx_dma_process_desc() and plx_dma_abort_desc(), the callback is invoked
while holding the spinlock:

	spin_lock(&plxdev->ring_lock);
	...
	dmaengine_desc_get_callback_invoke(&desc->txd, &res);

If a DMA client prepares and submits a new DMA transaction directly from
its completion callback, it will call plx_dma_prep_memcpy(). This attempts
to acquire ring_lock via spin_lock_bh() on the same CPU, which is already
held, resulting in a self-deadlock.

>  }

[ ... ]

> @@ -510,7 +510,7 @@ static int plx_dma_create(struct pci_dev *pdev)
>  		goto free_plx;
>  
>  	spin_lock_init(&plxdev->ring_lock);
> -	tasklet_setup(&plxdev->desc_task, plx_dma_desc_task);
> +	dmaengine_init_bh(&plxdev->dma_chan, plx_dma_desc_task);
>  
>  	RCU_INIT_POINTER(plxdev->pdev, pdev);
>  	plxdev->bar = pcim_iomap_table(pdev)[0];

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
with the interrupt handler here?

request_irq() is called earlier in plx_dma_create(), unmasking the interrupt
before plxdev->bar is initialized.

If an interrupt fires immediately upon registration (for example, due to
pending hardware state), plx_dma_isr() executes and unconditionally reads from
plxdev->bar:

	status = readw(plxdev->bar + PLX_REG_INTR_STATUS);

Since plxdev->bar is still NULL at that point, this will result in a NULL
pointer dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=27