Re: [PATCH v2 1/1] mmc: use dmaengine_get_dma_device() instead of chan->device->dev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mmc: mxcmmc: NULL pointer dereference in mxcmci_setup_data fallback to PIO
- [Medium] mmc: missed device->dev conversions in omap.c and mxs-mmc.c
--

commit fac187edba18928bfcb50c068ba67dea2103d2aa
Author: Frank Li <Frank.Li@nxp.com>
Subject: mmc: use dmaengine_get_dma_device() instead of chan->device->dev

This patch replaces direct dma_chan::device::dev access with the 
dmaengine_get_dma_device() macro across the MMC subsystem. This change 
prepares the subsystem for making the DMA engine provider data structures 
private.

> mmc: use dmaengine_get_dma_device() instead of chan->device->dev
>
> Replace direct dma_chan::device::dev access with the proper
> dmaengine_get_dma_device() for consumer API
>
> chan->device->dev is not always the device used for DMA mapping.
> Some DMA engines support per-channel IOMMU mappings, so different
> channels may use different DMA devices.  dmaengine_get_dma_device()
> returns the correct device for each channel.
>
> This also prepares for making the DMA engine provider data structures
> private. DMA consumers should not access DMA engine internals directly.
>
> Assisted-by: LLM
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>
> ---
> Change in v2
> - find more place, which use device->dev for dma mapping. This time should
> clean all.

[Severity: Medium]
Does this patch catch all instances across the MMC subsystem?

The commit message mentions this should clean all occurrences, but there
appear to be remaining direct chan->device->dev accesses in
drivers/mmc/host/omap.c (e.g., c->device->dev in mmc_omap_prepare_data())
and drivers/mmc/host/mxs-mmc.c (ssp->dmach->device->dev in mxs_mmc_probe()).
These will likely cause compilation failures when the DMA engine fields are
actually made private.

[ ... ]
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 097498a3f8ff4..c20cff770395e 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -290,6 +290,7 @@ static int mxcmci_setup_data(struct mxcmci_host *host, struct mmc_data *data)
>  	unsigned int datasize = nob * blksz;
>  	struct scatterlist *sg;
>  	enum dma_transfer_direction slave_dirn;
> +	struct device *dma_dev = dmaengine_get_dma_device(host->dma);

[Severity: High]
Will this cause a NULL pointer dereference if the driver falls back to 
PIO mode?

In mxcmci_probe(), if dma_request_chan() fails, host->dma is set to NULL to
fallback to PIO mode. However, in mxcmci_setup_data(), we now unconditionally
initialize dma_dev by calling dmaengine_get_dma_device(host->dma) before the
!mxcmci_use_dma(host) check happens. This macro dereferences the channel
pointer to access chan->chan_dev->chan_dma_dev, which will crash upon the 
first PIO data transfer if host->dma is NULL.

>  	int i, nents;
>  
>  	host->data = data;
[ ... ]

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