Re: [PATCH v2 24/64] dmaengine: txx9dmac: route callbacks via channel BH

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized BH work and missing cleanup in shared IRQ mode
- [High] Interrupt storm and livelock due to premature IRQ unmasking in shared IRQ mode
- [High] Invalid `enable_irq()` parameter in shared IRQ mode
--

--- Patch [24]: [PATCH v2 24/64] dmaengine: txx9dmac: route callbacks via channel BH ---
commit ba661d7c1d6f85fdbcef7fb4031ee54ec6ee5c5a
Author: Allen Pais <[email protected]>

dmaengine: txx9dmac: route callbacks via channel BH

Use the per-channel BH helpers for descriptor completion handling and
schedule per-channel work from both channel and controller IRQ paths.

> diff --git a/drivers/dma/txx9dmac.c b/drivers/dma/txx9dmac.c
> index 05622b68a9363..6269baa28c513 100644
> --- a/drivers/dma/txx9dmac.c
> +++ b/drivers/dma/txx9dmac.c
> @@ -601,13 +601,13 @@ static void txx9dmac_scan_descriptors(struct txx9dmac_chan *dc)
>  	}
>  }
>  
> -static void txx9dmac_chan_tasklet(struct tasklet_struct *t)
> +static void txx9dmac_chan_tasklet(struct dma_chan *chan)
>  {
>  	int irq;
>  	u32 csr;
>  	struct txx9dmac_chan *dc;
>  
> -	dc = from_tasklet(dc, t, tasklet);
> +	dc = container_of(chan, struct txx9dmac_chan, chan);
>  	csr = channel_readl(dc, CSR);
>  	dev_vdbg(chan2dev(&dc->chan), "tasklet: status=%x\n", csr);
>  

[Severity: High]
Does this code handle shared IRQs correctly?

When running in shared IRQ mode, dc->irq is set to -1. Now that the shared
controller path schedules this channel BH, txx9dmac_chan_tasklet() will run
and unconditionally call enable_irq(dc->irq) at the end of the function:

    irq = dc->irq;
    enable_irq(irq);

Could this end up calling enable_irq(-1) and causing an issue in the core IRQ
code?

[ ... ]

> @@ -659,7 +659,7 @@ static void txx9dmac_tasklet(struct tasklet_struct *t)
>  			spin_lock(&dc->lock);
>  			if (csr & (TXX9_DMA_CSR_ABCHC | TXX9_DMA_CSR_NCHNC |
>  				   TXX9_DMA_CSR_NTRNFC))
> -				txx9dmac_scan_descriptors(dc);
> +				dma_chan_schedule_bh(&dc->chan);
>  			spin_unlock(&dc->lock);
>  		}
>  	}

[Severity: High]
Could this result in an interrupt storm?

Previously, txx9dmac_scan_descriptors() would synchronously clear the hardware
interrupt condition before the shared tasklet unmasked the IRQ. 

Now, the work is deferred to the async BH handler, but enable_irq(irq) is
still called synchronously at the end of txx9dmac_tasklet():

    irq = ddev->irq;
    enable_irq(irq);

Will the unmasked IRQ immediately fire again before the BH has a chance to run
and clear the hardware interrupt condition?

[ ... ]

> @@ -1113,7 +1113,7 @@ static int __init txx9dmac_chan_probe(struct platform_device *pdev)
>  		irq = platform_get_irq(pdev, 0);
>  		if (irq < 0)
>  			return irq;
> -		tasklet_setup(&dc->tasklet, txx9dmac_chan_tasklet);
> +		dma_chan_init_bh(&dc->chan, txx9dmac_chan_tasklet);
>  		dc->irq = irq;
>  		err = devm_request_irq(&pdev->dev, dc->irq,
>  			txx9dmac_chan_interrupt, 0, dev_name(&pdev->dev), dc);

[Severity: High]
Is the BH work properly initialized when operating in shared IRQ mode?

dma_chan_init_bh() is only called if dc->ddev->irq < 0:

    if (dc->ddev->irq < 0) {
        ...
        dma_chan_init_bh(&dc->chan, txx9dmac_chan_tasklet);
        ...
    } else {
        dc->irq = -1;
    }

If the controller uses a shared IRQ, the BH work remains uninitialized.
When the shared controller tasklet attempts to schedule it, will it trigger a
WARN_ON(!chan->bh_work_initialized) and drop the completion request entirely?

[ ... ]

> @@ -1159,7 +1159,7 @@ static void txx9dmac_chan_remove(struct platform_device *pdev)
>  	dma_async_device_unregister(&dc->dma);
>  	if (dc->irq >= 0) {
>  		devm_free_irq(&pdev->dev, dc->irq, dc);
> -		tasklet_kill(&dc->tasklet);
> +		dma_chan_kill_bh(&dc->chan);
>  	}
>  	dc->ddev->chan[pdev->id % TXX9_DMA_MAX_NR_CHANNELS] = NULL;
>  }

[Severity: High]
Could we leak resources here in shared IRQ mode?

Similar to the probe function, the cleanup relies on dc->irq >= 0. If the
initialization issue above is fixed so the BH work is correctly used in shared
IRQ mode, dma_chan_kill_bh() would still be skipped here since dc->irq is -1.

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