Re: [PATCH 2/2] dmaengine: fsl-edma: add per-channel IOMMU support via iommu-map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] dmaengine: fsl-edma: TCD pool mapped to parent device causing IOMMU faults
- [High] dmaengine: fsl-edma: masking -EPROBE_DEFER in xlate breaks deferred probe
- [High] dmaengine: fsl-edma: late modification of dev->bus causes refcount underflow
- [High] dmaengine: fsl-edma: DMA_MEMCPY channels bypass IOMMU configuration
- [High] dmaengine: fsl-edma: missing IOMMU configuration in fsl_edma_xlate
- [High] dmaengine: fsl-edma: dirty srcid state on error path blocks future requests
--

commit 94975a647e48c96637ca91c3ba2539ad15656ede
Author: Peng Fan <peng.fan@nxp.com>

dmaengine: fsl-edma: add per-channel IOMMU support via iommu-map

This patch adds iommu-map support so each eDMA channel can be individually
mapped to an IOMMU stream ID. At probe time, per-channel devices are
initialized, and at xlate time the IOMMU domain is configured.

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index bb7531c456dfa..58a92ff4ddbe7 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -283,8 +283,10 @@ int fsl_edma_resume(struct dma_chan *chan)
>  
>  static void fsl_edma_unprep_slave_dma(struct fsl_edma_chan *fsl_chan)
>  {
[ ... ]
> @@ -294,7 +296,7 @@ static void fsl_edma_unprep_slave_dma(struct fsl_edma_chan *fsl_chan)
>  static bool fsl_edma_prep_slave_dma(struct fsl_edma_chan *fsl_chan,
>  				    enum dma_transfer_direction dir)
>  {
> -	struct device *dev = fsl_chan->vchan.chan.device->dev;
> +	struct device *dev = dmaengine_get_dma_device(&fsl_chan->vchan.chan);
>  	enum dma_data_direction dma_dir;
>  	phys_addr_t addr = 0;
>  	u32 size = 0;

[Severity: High]
Since the new per-channel IOMMU feature maps channel devices differently from
the parent controller, does the hardware TCD pool allocation in
fsl_edma_alloc_chan_resources() also need to be updated?

It currently maps the TCD pool using the parent controller device:

	fsl_chan->tcd_pool = dma_pool_create("tcd_pool", chan->device->dev, ...

If the pool uses the parent device, won't its DMA addresses be invalid in
the channel's hardware IOMMU domain, causing translation faults when fetching
the TCDs?

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index d9fb717b5b53c..eabb0086fc9de 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
[ ... ]
> @@ -332,6 +334,12 @@ static struct dma_chan *fsl_edma3_xlate(struct of_phandle_args *dma_spec,
>  		fsl_chan->is_remote = dma_spec->args[2] & FSL_EDMA_REMOTE;
>  		fsl_chan->is_multi_fifo = dma_spec->args[2] & FSL_EDMA_MULTI_FIFO;
>  
> +		if (fsl_edma->has_iommu_map) {
> +			ret = fsl_edma_chan_configure_iommu(fsl_chan);
> +			if (ret)
> +				return NULL;
> +		}
> +

[Severity: High]
If fsl_edma_chan_configure_iommu() fails and returns -EPROBE_DEFER because the
IOMMU driver is not yet probed, this drops the valid error code and returns
NULL. Will this mislead the core DMA engine (which treats NULL as 'channel not
found') and break deferred probing for client devices?

[Severity: High]
If fsl_edma_chan_configure_iommu() fails and returns an error, returning NULL
here does not roll back the state assignments made just above
(e.g., fsl_chan->srcid = dma_spec->args[0]). Because the channel wasn't
successfully allocated, fsl_edma_free_chan_resources() will never be called to
clear it. Will this leave a dirty srcid state that permanently locks out
future client requests for this hardware signal due to fsl_edma_srcid_in_use()?

[Severity: High]
Because this IOMMU configuration logic is placed exclusively inside the DT
xlate function, does it completely bypass non-DT channel allocations? If a
client driver requests a channel for DMA_MEMCPY, memory will be mapped using
the parent controller device while the channel hardware fetches using its
hardwired Stream ID. Won't this cause IOMMU translation faults for non-DT
channels?

>  		chan = dma_get_slave_channel(chan);
>  		chan->device->privatecnt++;
>  		return chan;
[ ... ]
> +static void fsl_edma_init_chan_iommu(struct platform_device *pdev,
> +				    struct fsl_edma_engine *fsl_edma)
> +{
[ ... ]
> +	for (i = 0; i < fsl_edma->n_chans; i++) {
> +		struct fsl_edma_chan *fsl_chan = &fsl_edma->chans[i];
> +		struct device *dev;
> +
> +		if (fsl_edma->chan_masked & BIT(i))
> +			continue;
> +
> +		dev = &fsl_chan->vchan.chan.dev->device;
> +		dev->coherent_dma_mask = pdev->dev.coherent_dma_mask;
> +		dev->dma_mask = &dev->coherent_dma_mask;
> +		dev->bus = pdev->dev.bus;

[Severity: High]
Does this illegally modify the bus type of an already-registered device?

This is executed after dmaenginem_async_device_register() has already called
device_add(). Since bus_add_device() is never executed, won't device_del()
invoke bus_remove_device() during driver unbinding or removal, causing a fatal
reference count underflow and use-after-free on the global platform_bus_type
subsystem?

> +	}
> +
> +	fsl_edma->has_iommu_map = true;
> +}
[ ... ]
> @@ -882,6 +954,8 @@ static int fsl_edma_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "Can't register Freescale eDMA engine.\n");
>  
> +	fsl_edma_init_chan_iommu(pdev, fsl_edma);
> +
>  	ret = devm_of_dma_controller_register(&pdev->dev, np,
>  			drvdata->dmamuxs ? fsl_edma_xlate : fsl_edma3_xlate,
>  			fsl_edma);

[Severity: High]
Since the per-channel IOMMU configuration was only added to fsl_edma3_xlate(),
does this entirely omit configuration for hardware variants relying on
fsl_edma_xlate()? Will controller variants using fsl_edma_xlate bypass the
IOMMU configuration and cause hardware translation faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-edma-iommu-v1-0-e1731968081e@nxp.com?part=2