Re: [PATCH 26/26] dmaengine: xgene-dma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Incomplete removal of redundant error messages for `devm_request_irq()` failures.

Pre-existing issues:
- [High] Incorrect channel index used during IRQ rollback path in `xgene_dma_request_irqs()`, leading to multiple kernel warnings and failed rollback.
- [High] Race condition in probe sequence causes NULL pointer dereference via uninitialized tasklet.
- [High] Probe error path fails to disable DMA hardware, leading to potential memory corruption.
--

--- Patch [26]: [PATCH 26/26] dmaengine: xgene-dma: Remove redundant dev_err()/dev_err_probe() ---
commit b27e7ae82dbb69011f33590c87faea09bb044017
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: xgene-dma: Remove redundant dev_err()/dev_err_probe()

The devm_request_irq() now automatically logs detailed error messages on
failure. This eliminates the need for driver-specific dev_err() and
dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/dma/xgene-dma.c b/drivers/dma/xgene-dma.c
> --- a/drivers/dma/xgene-dma.c
> +++ b/drivers/dma/xgene-dma.c
> @@ -1436,11 +1436,8 @@ static int xgene_dma_request_irqs(struct xgene_dma *pdma)
>  	/* Register DMA error irq */
>  	ret = devm_request_irq(pdma->dev, pdma->err_irq, xgene_dma_err_isr,
>  			       0, "dma_error", pdma);
> -	if (ret) {
> -		dev_err(pdma->dev,
> -			"Failed to register error IRQ %d\n", pdma->err_irq);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	/* Register DMA channel rx irq */
>  	for (i = 0; i < XGENE_DMA_MAX_CHANNEL; i++) {
>  		chan = &pdma->chan[i];
>  		irq_set_status_flags(chan->rx_irq, IRQ_DISABLE_UNLAZY);
>  		ret = devm_request_irq(chan->dev, chan->rx_irq,
>  				       xgene_dma_chan_ring_isr,
>  				       0, chan->name, chan);
>  		if (ret) {
>  			chan_err(chan, "Failed to register Rx IRQ %d\n",
>  				 chan->rx_irq);

[Severity: Medium]
The commit message states that the patch eliminates driver-specific error
logs for devm_request_irq failures because the core now logs them
automatically.

While the patch removes the dev_err for the global error IRQ above, it
appears to miss this chan_err macro for the Rx IRQ registration failure.

Should this chan_err also be removed to fully align with the commit message?

>  			devm_free_irq(pdma->dev, pdma->err_irq, pdma);
>  
>  			for (j = 0; j < i; j++) {
>  				chan = &pdma->chan[i];

[Severity: High]
This is a pre-existing issue, but there appears to be a typo in the rollback
loop in xgene_dma_request_irqs.

It assigns chan using the outer loop index i instead of the inner loop
index j. Will this cause devm_free_irq to be called multiple times on an
unregistered IRQ and fail to clear the IRQ_DISABLE_UNLAZY flag for the
successfully registered ones?

>  				irq_clear_status_flags(chan->rx_irq, IRQ_DISABLE_UNLAZY);
>  				devm_free_irq(chan->dev, chan->rx_irq, chan);
>  			}
>  
>  			return ret;
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at the probe sequence
in xgene_dma_probe:

drivers/dma/xgene-dma.c:xgene_dma_probe() {
    ...
    ret = xgene_dma_request_irqs(pdma);
    if (ret)
        goto err_request_irq;

    /* Configure and enable DMA engine */
    xgene_dma_init_hw(pdma);

    /* Register DMA device with linux async framework */
    ret = xgene_dma_init_async(pdma);
    ...
}

The function xgene_dma_request_irqs is called before the hardware is unmasked
in xgene_dma_init_hw. If a hardware interrupt fires immediately after
xgene_dma_init_hw, xgene_dma_chan_ring_isr will execute and unconditionally
schedule the tasklet:

drivers/dma/xgene-dma.c:xgene_dma_chan_ring_isr() {
    ...
    tasklet_schedule(&chan->tasklet);
}

However, chan->tasklet is not set up until xgene_dma_init_async runs later
in the probe sequence. Can this trigger a NULL pointer dereference by
scheduling an uninitialized tasklet?

[Severity: High]
This is also a pre-existing issue, but looking at the error handling path
in xgene_dma_probe:

drivers/dma/xgene-dma.c:xgene_dma_probe() {
    ...
err_async_init:
    xgene_dma_free_irqs(pdma);

err_request_irq:
    for (i = 0; i < XGENE_DMA_MAX_CHANNEL; i++)
        xgene_dma_delete_chan_rings(&pdma->chan[i]);
    ...
}

If xgene_dma_init_async fails, the code jumps to err_async_init and frees
software resources and DMA descriptor ring buffers.

This path does not appear to call xgene_dma_disable or
xgene_dma_mask_interrupts. Could this leave the DMA controller actively
running and pointing at freed memory, potentially causing memory corruption
if the hardware continues to process descriptors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=26