Re: [PATCH 25/26] dmaengine: uniphier-xdmac: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Returning early from uniphier_xdmac_remove() if a channel fails to terminate leaves the device registered with the DMA engine while devres frees its underlying memory, causing a Use-After-Free.
- [Medium] The shared IRQ handler unconditionally returns IRQ_HANDLED, which breaks the kernel's spurious interrupt detection.
--

--- Patch [25]: [PATCH 25/26] dmaengine: uniphier-xdmac: Remove redundant dev_err()/dev_err_probe() ---
commit d83da39937d839c163b8d27546ec157138f20ec2
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: uniphier-xdmac: 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/uniphier-xdmac.c b/drivers/dma/uniphier-xdmac.c
> index 120c0d4f12dda..b23d61bf7fd5b 100644
> --- a/drivers/dma/uniphier-xdmac.c
> +++ b/drivers/dma/uniphier-xdmac.c
> @@ -532,10 +532,8 @@ static int uniphier_xdmac_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(dev, irq, uniphier_xdmac_irq_handler,
>  			       IRQF_SHARED, "xdmac", xdev);

[Severity: Medium]
This is a pre-existing issue, but since we are registering this handler with
IRQF_SHARED here, does unconditionally returning IRQ_HANDLED break the
kernel's spurious interrupt detection?

Looking at uniphier_xdmac_irq_handler():

static irqreturn_t uniphier_xdmac_irq_handler(int irq, void *dev_id)
{
	struct uniphier_xdmac_device *xdev = dev_id;
	int i;

	for (i = 0; i < xdev->nr_chans; i++)
		uniphier_xdmac_chan_irq(&xdev->channels[i]);

	return IRQ_HANDLED;
}

If a shared interrupt line experiences an interrupt storm or unhandled
interrupt from another device, and no channels here have pending interrupts
(uniphier_xdmac_chan_irq does nothing), this driver will still claim to have
handled it. Will this prevent the kernel from disabling the broken line,
ultimately leading to a system hang?

> -	if (ret) {
> -		dev_err(dev, "Failed to request IRQ\n");
> +	if (ret)
>  		return ret;
> -	}
>  
>  	ret = dma_async_device_register(ddev);

[Severity: High]
This is a pre-existing issue, but concerning the device registration here,
what happens during teardown if dmaengine_terminate_sync() fails?

drivers/dma/uniphier-xdmac.c:uniphier_xdmac_remove() {
	...
	list_for_each_entry(chan, &ddev->channels, device_node) {
		ret = dmaengine_terminate_sync(chan);
		if (ret) {
			dev_alert(&pdev->dev, "Failed to terminate...");
			return;
		}
		...
	}

	of_dma_controller_free(pdev->dev.of_node);
	dma_async_device_unregister(ddev);
}

If we return early because of a termination failure,
dma_async_device_unregister() is skipped. Because the xdev structure is
managed by devm_kzalloc, devres will automatically free the memory when the
unbind completes. Will this leave a dangling pointer in the global DMA
device list, leading to a use-after-free if the subsystem accesses the
freed memory?

>  	if (ret) {

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