Re: [PATCH v3 6/7] dmaengine: fsl-edma: support per-channel DT child nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] dmaengine: fsl-edma: missing dma_parms on chdev in fsl_edma_configure_chan_iommu()
- [Medium] dmaengine: fsl-edma: 64-bit division and missing bounds check in fsl_edma_child_to_chan_id()
- [Medium] dmaengine: fsl-edma: 32-bit BIT() used on u64 chan_masked in DT channel helpers
- [Medium] dmaengine: fsl-edma: probe failure and errirq lookup in fsl_edma3_irq_init() with child IRQs
- [Medium] dmaengine: fsl-edma: unimplemented child power-domains support claimed in commit message
- [Medium] dmaengine: fsl-edma: silent non-deferral IOMMU errors in fsl_edma_configure_chan_iommu()
--

commit 6337712c7d2219f406bdb1b2711aad20264a4f74
Author: Peng Fan <peng.fan@nxp.com>

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

This commit adds support for parsing per-channel devicetree child nodes in the
fsl-edma driver to configure per-channel interrupts and IOMMU mappings.

>     dmaengine: fsl-edma: support per-channel DT child nodes
>     
>     Add support for parsing per-channel child nodes from devicetree.
>     Each child node can carry its own interrupts, iommus, and
>     power-domains properties.
>     
>     The implementation is split into three functions:
>     
>     - fsl_edma_child_to_chan_id(): derives the channel index from the
>       child MMIO address relative to the parent base.
>     
>     - fsl_edma_configure_chan_from_dt(): called before setup_irq() to
>       populate per-channel txirq from child interrupt properties.
>       fsl_edma3_irq_init() is updated to skip platform_get_irq() for
>       channels that already have an IRQ assigned from their child node.
>     
>     - fsl_edma_configure_chan_iommu(): called after
>       dmaenginem_async_device_register() to configure per-channel IOMMU
>       by calling of_dma_configure() on each dma_chan_dev and setting
>       chan_dma_dev so dmaengine_get_dma_device() returns the per-channel
>       device.
>     
>     Existing DTS without child nodes continues to work unchanged.
>     
>     Signed-off-by: Peng Fan <peng.fan@nxp.com>

[Severity: Medium]
The commit message states that each child node can carry its own interrupts,
iommus, and power-domains properties, and that support is added for parsing
per-channel child nodes from devicetree.

However, fsl_edma_configure_chan_from_dt() only parses reg and interrupts, and
fsl_edma_configure_chan_iommu() only parses iommus. In fsl_edma3_attach_pd(),
power domains are still attached by index from the parent platform_device node
via dev_pm_domain_attach_by_id(dev, i), so any power-domains properties placed
on child nodes are ignored.

Should per-channel child power-domains be attached here, or should the commit
message be updated to reflect that child power-domains are not handled by this
patch?

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index 30d548a723462..795744e67837d 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
[ ... ]
> @@ -400,12 +403,13 @@ static int fsl_edma3_irq_init(struct platform_device *pdev, struct fsl_edma_engi
>  		if (fsl_edma->chan_masked & BIT(i))
>  			continue;
>  
> -		/* request channel irq */
> -		fsl_chan->txirq = platform_get_irq(pdev, i);
> -		if (fsl_chan->txirq < 0)
> -			return  -EINVAL;
> +		if (fsl_chan->txirq <= 0) {
> +			fsl_chan->txirq = platform_get_irq(pdev, i);
> +			if (fsl_chan->txirq < 0)
> +				return fsl_chan->txirq;
>  
> -		fsl_chan->irq_handler = fsl_edma3_tx_handler;
> +			fsl_chan->irq_handler = fsl_edma3_tx_handler;
> +		}

[Severity: Medium]
What happens in fsl_edma3_irq_init() when a devicetree omits parent interrupts
and only defines available child nodes for a subset of channels (or marks some
child nodes as status = "disabled") without a matching dma-channel-mask?

In fsl_edma_configure_chan_from_dt(), fsl_chan->txirq is only populated for
available child nodes, leaving fsl_chan->txirq == 0 and the channel unmasked
in fsl_edma->chan_masked for any remaining channels. When fsl_edma3_irq_init()
then runs, it falls back to platform_get_irq(pdev, i) on the parent device for
the first channel without an available child node, which fails with -ENXIO and
aborts fsl_edma_probe().

Also, further down in fsl_edma3_irq_init(), when FSL_EDMA_DRV_ERRIRQ_SHARE is
set:

	if (fsl_edma->drvdata->flags & FSL_EDMA_DRV_ERRIRQ_SHARE) {
		/* last one is error irq */
		fsl_edma->errirq = platform_get_irq_optional(pdev, fsl_edma->n_chans);

When per-channel interrupts are moved into child nodes, can the shared error
IRQ still be found at index fsl_edma->n_chans on the parent node?

> @@ -703,6 +707,136 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng
>  	return -EINVAL;
>  }
>  
> +static int fsl_edma_child_to_chan_id(struct platform_device *pdev,
> +				     struct fsl_edma_engine *fsl_edma,
> +				     struct device_node *child,
> +				     struct resource *parent_res)
> +{
> +	const struct fsl_edma_drvdata *drvdata = fsl_edma->drvdata;
> +	struct resource child_res;
> +	u32 ch_id;
> +	int ret;
> +
> +	ret = of_address_to_resource(child, 0, &child_res);
> +	if (ret)
> +		return ret;
> +
> +	if (child_res.start < parent_res->start + drvdata->chreg_off)
> +		return -EINVAL;
> +
> +	ch_id = (child_res.start - parent_res->start - drvdata->chreg_off)
> +		/ drvdata->chreg_space_sz;
> +
> +	if (ch_id >= fsl_edma->n_chans)
> +		return -EINVAL;
> +
> +	return ch_id;
> +}

[Severity: Medium]
Can the division in fsl_edma_child_to_chan_id() cause a link failure on 32-bit
architectures with CONFIG_PHYS_ADDR_T_64BIT=y (such as 32-bit ARM with LPAE or
32-bit PowerPC with CONFIG_PHYS_64BIT=y)?

Because child_res.start and parent_res->start have type resource_size_t, the
expression

	(child_res.start - parent_res->start - drvdata->chreg_off)
		/ drvdata->chreg_space_sz

performs a 64-bit division by the non-constant u32 drvdata->chreg_space_sz
using the plain / operator, which emits an unresolved __aeabi_uldivmod or
__udivdi3 call on 32-bit builds.

In addition, before computing ch_id, only the lower bound is checked. If
child_res is not verified to be contained within parent_res (for example via
resource_contains(parent_res, &child_res)) and aligned to
drvdata->chreg_space_sz, could a large or unaligned child reg address truncate
when assigned to u32 ch_id or alias a valid channel index?

> +
> +static int fsl_edma_configure_chan_from_dt(struct platform_device *pdev,
> +					   struct fsl_edma_engine *fsl_edma)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource parent_res;
> +	struct device_node *child;
> +	int ret;
> +
> +	ret = of_address_to_resource(np, 0, &parent_res);
> +	if (ret)
> +		return ret;
> +
> +	for_each_available_child_of_node(np, child) {
> +		struct fsl_edma_chan *fsl_chan;
> +		int ch_id, irq;
> +
> +		ch_id = fsl_edma_child_to_chan_id(pdev, fsl_edma, child,
> +						  &parent_res);
> +		if (ch_id < 0) {
> +			dev_err(&pdev->dev, "invalid channel in %pOFn\n", child);
> +			of_node_put(child);
> +			return -EINVAL;
> +		}
> +
> +		if (fsl_edma->chan_masked & BIT(ch_id))
> +			continue;

[Severity: Medium]
Should this check in fsl_edma_configure_chan_from_dt() (and the identical
check in fsl_edma_configure_chan_iommu() below) use BIT_ULL(ch_id) instead of
BIT(ch_id)?

fsl_edma->chan_masked is a u64 bitmask supporting up to 64 channels (for
example on fsl,imx95-edma5 where dma-channels is 64 and ch_id can be 0..63),
whereas BIT(ch_id) uses unsigned long and overflows on 32-bit architectures
when ch_id >= 32.

[ ... ]
> +static int fsl_edma_configure_chan_iommu(struct platform_device *pdev,
> +					struct fsl_edma_engine *fsl_edma)
> +{
[ ... ]
> +		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);
> +			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;
> +			}

[Severity: Medium]
As reported by Frank Li on v1 [1], should dev_err_probe() be used here when
of_dma_configure() fails so that -EPROBE_DEFER does not log an error message?

Also, in drivers/of/device.c, of_dma_configure_id() ignores all errors from
of_iommu_configure() other than -EPROBE_DEFER (for example when duplicate
StreamIDs across channels cause arm_smmu_insert_master() to return -ENODEV)
and returns 0 after falling back to non-IOMMU DMA ops. If that happens, will
fsl_edma_configure_chan_iommu() silently set chan_dma_dev = true on a channel
device that failed to attach to the IOMMU?

> +
> +			chan->chan_dev->chan_dma_dev = true;

[Severity: High]
When chan->chan_dev->chan_dma_dev is set to true in
fsl_edma_configure_chan_iommu(), dmaengine_get_dma_device(chan) returns chdev
instead of fsl_edma->dma_dev.dev (&pdev->dev).

In fsl_edma_probe(), max_seg_size is configured on fsl_edma->dma_dev.dev:

	/* Per worst case 'nbytes = 1' take CITER as the max_seg_size */
	dma_set_max_seg_size(fsl_edma->dma_dev.dev,
			     FIELD_GET(EDMA_TCD_ITER_MASK, EDMA_TCD_ITER_MASK));

However, chdev->dma_parms is left as NULL on the per-channel device. Doesn't
that cause dma_get_max_seg_size(dmaengine_get_dma_device(chan)) to return the
64 KiB default (SZ_64K) instead of 0x7fff (32767), allowing iommu_dma_map_sg()
and DMAengine clients to coalesce scatterlist segments up to 64 KiB and
overflow the 15-bit CITER/BITER iteration count in fsl_edma_prep_slave_sg()
and fsl_edma_prep_dma_cyclic()?

Should chdev->dma_parms be set to pdev->dev.dma_parms in
fsl_edma_configure_chan_iommu()?

[1] https://lore.kernel.org/r/aqrem_EkcQM2t0IE@SMW015318

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-edma-iommu-v3-v3-0-9d0e62a954eb@Example.Com?part=6