Re: [PATCH v2 2/3] dmaengine: fsl-edma: support per-channel DT child nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: fsl-edma: kernel panic due to post-registration dev->bus modification
- [High] dmaengine: fsl-edma: probe deferral masked in DT IRQ lookup
- [Low] dmaengine: fsl-edma: compilation failure due to undefined function and member
--

commit 7b47892ce3017d20fbf0bc531e1531f30816d5d2
Author: Peng Fan <peng.fan@nxp.com>

    dmaengine: fsl-edma: support per-channel DT child nodes

This commit adds support for parsing per-channel child nodes from the
devicetree. It configures per-channel IRQs and IOMMUs based on the child
properties. Existing devicetrees without child nodes continue to work as
before.

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index d9fb717b5b53c..e7e5b33cfcb20 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
[ ... ]
> @@ -695,6 +699,133 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng
[ ... ]
> +static int fsl_edma_configure_chan_from_dt(struct platform_device *pdev,
> +					   struct fsl_edma_engine *fsl_edma)
> +{
[ ... ]
> +	for_each_available_child_of_node(np, child) {
> +		struct fsl_edma_chan *fsl_chan;
> +		int ch_id, irq;
[ ... ]
> +		fsl_chan = &fsl_edma->chans[ch_id];
> +
> +		irq = of_irq_get(child, 0);
> +		if (irq > 0) {
> +			fsl_chan->txirq = irq;
> +			fsl_chan->irq_handler = fsl_edma3_tx_handler;
> +		}

[Severity: High]
If of_irq_get() returns -EPROBE_DEFER because the interrupt controller
is not yet ready, it seems that error code is silently dropped since
only irq > 0 is checked.

Later in fsl_edma3_irq_init(), if fsl_chan->txirq <= 0, it tries to fall
back to platform_get_irq() which will fail because the interrupts are
defined in the child nodes.

Would this cause fsl_edma3_irq_init() to return -EINVAL, permanently failing
the probe instead of correctly deferring it?

> +	}
> +
> +	return 0;
> +}
> +
> +static int fsl_edma_configure_chan_iommu(struct platform_device *pdev,
> +					struct fsl_edma_engine *fsl_edma)
> +{
[ ... ]
> +	for_each_available_child_of_node(np, child) {
> +		struct fsl_edma_chan *fsl_chan;
> +		int ch_id;
[ ... ]
> +		list_for_each_entry(chan, &fsl_edma->dma_dev.channels,
> +				    device_node) {
> +			struct device *chdev;
> +
> +			if (to_fsl_edma_chan(chan) != fsl_chan)
> +				continue;
> +
> +			chdev = dmaengine_chan_dev(chan);

[Severity: Low]
Does dmaengine_chan_dev() exist in the kernel?

It looks like this might cause a compilation failure because the function is
undefined.

> +			chdev->bus = pdev->dev.bus;

[Severity: High]
Is it safe to modify the bus of an already-registered device here?

Since fsl_edma_configure_chan_iommu() is called after
dmaenginem_async_device_register(), bus_add_device() has already seen a
NULL bus during registration and skipped incrementing the subsystem reference
count.

If probe fails or the driver unbinds later, device_del() will call
bus_remove_device(), which will see this newly assigned bus and attempt to
drop the reference twice.

Could this cause a use-after-free panic in the bus subsystem?

> +			if (fsl_edma->drvdata->flags & FSL_EDMA_DRV_TCD64)
> +				dma_coerce_mask_and_coherent(chdev,
> +					DMA_BIT_MASK(64));
> +			else
> +				dma_coerce_mask_and_coherent(chdev,
> +					DMA_BIT_MASK(32));
> +			ret = of_dma_configure(chdev, child, true);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					"IOMMU config failed for channel %d: %d\n",
> +					ch_id, ret);
> +				of_node_put(child);
> +				return ret;
> +			}
> +
> +			chan->chan_dev->chan_dma_dev = true;

[Severity: Low]
Does struct dma_chan have a member named chan_dev?

This looks like it will cause another compilation failure.

> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-edma-iommu-v2-0-72a62bf44c74@nxp.com?part=2