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

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

[Frank.Li]

From: Frank Li <Frank.Li@nxp.com>

Use dmaengine_get_dma_device() instead of directly accessing chan->device
and use dmaengine_prep_dma_memcpy() instead of calling the function pointer
directly. This follows the dmaengine API convention of using accessor
helpers to decouple drivers from internal dmaengine structures.

Affects cadence-nand-controller and fsmc_nand drivers.

No functional change and prepare to move struct dma_device to private.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../mtd/nand/raw/cadence-nand-controller.c    | 22 +++++++++----------
 drivers/mtd/nand/raw/fsmc_nand.c              | 13 +++++------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/raw/cadence-nand-controller.c b/drivers/mtd/nand/raw/cadence-nand-controller.c
index 41a5219a4d4ed..256a372119655 100644
--- a/drivers/mtd/nand/raw/cadence-nand-controller.c
+++ b/drivers/mtd/nand/raw/cadence-nand-controller.c
@@ -1873,16 +1873,16 @@ static int cadence_nand_slave_dma_transfer(struct cdns_nand_ctrl *cdns_ctrl,
 {
 	DECLARE_COMPLETION_ONSTACK(finished);
 	struct dma_chan *chan;
-	struct dma_device *dma_dev;
+	struct device *dma_dev;
 	dma_addr_t src_dma, dst_dma, buf_dma;
 	struct dma_async_tx_descriptor *tx;
 	dma_cookie_t cookie;
 
 	chan = cdns_ctrl->dmac;
-	dma_dev = chan->device;
+	dma_dev = dmaengine_get_dma_device(chan);
 
-	buf_dma = dma_map_single(dma_dev->dev, buf, len, dir);
-	if (dma_mapping_error(dma_dev->dev, buf_dma)) {
+	buf_dma = dma_map_single(dma_dev, buf, len, dir);
+	if (dma_mapping_error(dma_dev, buf_dma)) {
 		dev_err(cdns_ctrl->dev, "Failed to map DMA buffer\n");
 		goto err;
 	}
@@ -1913,12 +1913,12 @@ static int cadence_nand_slave_dma_transfer(struct cdns_nand_ctrl *cdns_ctrl,
 	dma_async_issue_pending(cdns_ctrl->dmac);
 	wait_for_completion(&finished);
 
-	dma_unmap_single(dma_dev->dev, buf_dma, len, dir);
+	dma_unmap_single(dma_dev, buf_dma, len, dir);
 
 	return 0;
 
 err_unmap:
-	dma_unmap_single(dma_dev->dev, buf_dma, len, dir);
+	dma_unmap_single(dma_dev, buf_dma, len, dir);
 
 err:
 	dev_dbg(cdns_ctrl->dev, "Fall back to CPU I/O\n");
@@ -3125,7 +3125,7 @@ cadence_nand_irq_cleanup(int irqnum, struct cdns_nand_ctrl *cdns_ctrl)
 static int cadence_nand_init(struct cdns_nand_ctrl *cdns_ctrl)
 {
 	dma_cap_mask_t mask;
-	struct dma_device *dma_dev;
+	struct device *dma_dev;
 	int ret;
 
 	cdns_ctrl->cdma_desc = dma_alloc_coherent(cdns_ctrl->dev,
@@ -3169,12 +3169,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;
@@ -3203,7 +3203,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:
diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index e28a69ceb0663..7b5e74dff7bdd 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;
@@ -546,7 +545,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;
 }
-- 
2.43.0


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

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

[sashiko-bot]

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/