Re: [PATCH 1/1] mtd: nand: raw: use dmaengine accessor API functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtd: rawnand: cadence: unmap device mismatch in
  cadence_nand_remove()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] mtd: rawnand: fsmc: missing reinit_completion() in
  dma_xfer() [1]
--

commit 9eda52238f6d5af7214b32eeb0870d65126ccf3d
Author: Frank Li <Frank.Li@nxp.com>

mtd: nand: raw: use dmaengine accessor API functions

This commit replaces direct chan->device and device_prep_dma_memcpy()
accesses in the Cadence and FSMC raw NAND drivers with
dmaengine_get_dma_device() and dmaengine_prep_dma_memcpy().

> diff --git a/drivers/mtd/nand/raw/cadence-nand-controller.c b/drivers/mtd/nand/raw/cadence-nand-controller.c
> index 2085632631c02..5e3d34f24f9d9 100644
> --- a/drivers/mtd/nand/raw/cadence-nand-controller.c
> +++ b/drivers/mtd/nand/raw/cadence-nand-controller.c
[ ... ]
> @@ -3170,12 +3170,12 @@ static int cadence_nand_init(struct cdns_nand_ctrl *cdns_ctrl)
>  		}
>  	}
>  
> -	dma_dev = cdns_ctrl->dmac->device;
> -	cdns_ctrl->io.iova_dma = dma_map_resource(dma_dev->dev, cdns_ctrl->io.dma,
> +	dma_dev = dmaengine_get_dma_device(cdns_ctrl->dmac);
> +	cdns_ctrl->io.iova_dma = dma_map_resource(dma_dev, cdns_ctrl->io.dma,
>  						  cdns_ctrl->io.size,
>  						  DMA_BIDIRECTIONAL, 0);
>  
> -	ret = dma_mapping_error(dma_dev->dev, cdns_ctrl->io.iova_dma);
> +	ret = dma_mapping_error(dma_dev, cdns_ctrl->io.iova_dma);
>  	if (ret) {
>  		dev_err(cdns_ctrl->dev, "Failed to map I/O resource to DMA\n");
>  		goto dma_release_chnl;
> @@ -3204,7 +3204,7 @@ static int cadence_nand_init(struct cdns_nand_ctrl *cdns_ctrl)
>  	return 0;
>  
>  unmap_dma_resource:
> -	dma_unmap_resource(dma_dev->dev, cdns_ctrl->io.iova_dma,
> +	dma_unmap_resource(dma_dev, cdns_ctrl->io.iova_dma,
>  			   cdns_ctrl->io.size, DMA_BIDIRECTIONAL, 0);
>  
>  dma_release_chnl:

[Severity: Medium]
Should cadence_nand_remove() also use dmaengine_get_dma_device() when
unmapping cdns_ctrl->io.iova_dma?

drivers/mtd/nand/raw/cadence-nand-controller.c:cadence_nand_remove() {
	...
	if (cdns_ctrl->dmac)
		dma_unmap_resource(cdns_ctrl->dmac->device->dev,
				   cdns_ctrl->io.iova_dma, cdns_ctrl->io.size,
				   DMA_BIDIRECTIONAL, 0);
	...
}

When chan->dev->chan_dma_dev is set, dmaengine_get_dma_device() returns
&chan->dev->device rather than chan->device->dev, so cadence_nand_remove()
unmaps the resource against a different struct device than
cadence_nand_init() used to map it, leaking the mapping on driver removal.
It also leaves a direct dereference of struct dma_device.

> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index b13b2b0c3f300..831293a5bd814 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -486,7 +486,7 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>  		    enum dma_data_direction direction)
>  {
>  	struct dma_chan *chan;
> -	struct dma_device *dma_dev;
> +	struct device *dma_dev;
>  	struct dma_async_tx_descriptor *tx;
>  	dma_addr_t dma_dst, dma_src, dma_addr;
>  	dma_cookie_t cookie;
> @@ -501,9 +501,9 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>  	else
>  		return -EINVAL;
>  
> -	dma_dev = chan->device;
> -	dma_addr = dma_map_single(dma_dev->dev, buffer, len, direction);
> -	if (dma_mapping_error(dma_dev->dev, dma_addr))
> +	dma_dev = dmaengine_get_dma_device(chan);
> +	dma_addr = dma_map_single(dma_dev, buffer, len, direction);
> +	if (dma_mapping_error(dma_dev, dma_addr))
>  		return -EINVAL;
>  
>  	if (direction == DMA_TO_DEVICE) {
> @@ -514,8 +514,7 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>  		dma_dst = dma_addr;
>  	}
>  
> -	tx = dma_dev->device_prep_dma_memcpy(chan, dma_dst, dma_src,
> -			len, flags);
> +	tx = dmaengine_prep_dma_memcpy(chan, dma_dst, dma_src, len, flags);
>  	if (!tx) {
>  		dev_err(host->dev, "device_prep_dma_memcpy error\n");
>  		ret = -EIO;
> @@ -547,7 +546,7 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>  	ret = 0;
>  
>  unmap_dma:
> -	dma_unmap_single(dma_dev->dev, dma_addr, len, direction);
> +	dma_unmap_single(dma_dev, dma_addr, len, direction);
>  
>  	return ret;
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can dma_xfer() unmap
dma_addr while the DMA transfer is still in flight [1]?

If an earlier transfer times out and its callback fires late,
host->dma_access_complete is left signaled.  Because dma_xfer() does not
call reinit_completion(&host->dma_access_complete) before submitting a new
transfer, wait_for_completion_timeout() consumes the stale completion and
returns immediately, so dma_unmap_single() runs while hardware DMA is
still active on buffer.

[1] https://sashiko.dev/#/bug/linux-836e69de-023a-4bd2-8b41-03e05feb8137

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111202502.3860647-1-Frank.Li@oss.nxp.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/