Re: [RFC PATCH] mmc: meson-mx-sdhc: add DMA bounce buffer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] mmc: meson-mx-sdhc: missing cmd->error check in meson_mx_sdhc_irq_thread()
--

commit dab15b5e51f3b15121298d5095440b6a71ad6e0f
Author: ZHENPENG YUAN <yuanzhenpeng210@gmail.com>

mmc: meson-mx-sdhc: add DMA bounce buffer support

This commit adds a 128 KiB DMA bounce buffer for multi-segment scatterlist
requests in the Meson MX SDHC driver and increases max_segs to 64. Single
segment requests continue to use direct DMA, while multi-segment transfers
are copied through the bounce buffer.

> diff --git a/drivers/mmc/host/meson-mx-sdhc-mmc.c b/drivers/mmc/host/meson-mx-sdhc-mmc.c
> index fb49ea71289e6..c62583a804f49 100644
> --- a/drivers/mmc/host/meson-mx-sdhc-mmc.c
> +++ b/drivers/mmc/host/meson-mx-sdhc-mmc.c

[ ... ]

> @@ -625,6 +702,40 @@ static irqreturn_t meson_mx_sdhc_irq_thread(int irq, void *irq_data)
>  		 */
>  		meson_mx_sdhc_clear_fifo(host->mmc);
>  
> +	if (cmd->data) {
> +		struct mmc_data *data = cmd->data;
> +		size_t len = data->blksz * data->blocks;
> +		bool bounced = host->dma_bounce;
> +
> +		/*
> +		 * Unmap the data after the controller has been quiesced so
> +		 * that neither the reset nor the FIFO flush can race with an
> +		 * ongoing DMA transfer.  dma_unmap_single() (for bounced
> +		 * reads) also invalidates the CPU caches, so the copy below
> +		 * sees the data written by the controller.
> +		 */
> +		meson_mx_sdhc_unmap_dma(host, data);
> +
> +		if (!data->error) {
> +			if (bounced && data->flags & MMC_DATA_READ) {
> +				size_t copied;
> +
> +				copied = sg_copy_from_buffer(data->sg,
> +							     data->sg_len,
> +							     host->bounce_buf,
> +							     len);
> +				if (copied != len) {
> +					dev_err(mmc_dev(host->mmc),
> +						"failed to copy data from the bounce buffer\n");
> +					data->error = -EIO;
> +				}
> +			}
> +
> +			if (!data->error)
> +				data->bytes_xfered = len;
> +		}
> +	}

[Severity: Critical]
Should this block in meson_mx_sdhc_irq_thread() check !cmd->error in addition
to !data->error before copying from host->bounce_buf and setting
data->bytes_xfered?

When a command with cmd->data encounters a FIFO overrun/underrun or a command
response CRC or timeout error, meson_mx_sdhc_irq() sets cmd->error while
leaving cmd->data->error as 0:

drivers/mmc/host/meson-mx-sdhc-mmc.c:meson_mx_sdhc_irq() {
	if (ista & MESON_SDHC_ISTA_RXFIFO_FULL ||
	    ista & MESON_SDHC_ISTA_TXFIFO_EMPTY)
		cmd->error = -EIO;
	else if (ista & MESON_SDHC_ISTA_RESP_ERR_CRC)
		cmd->error = -EILSEQ;
	else if (ista & MESON_SDHC_ISTA_RESP_TIMEOUT)
		cmd->error = -ETIMEDOUT;

	if (cmd->data) {
		if (ista & MESON_SDHC_ISTA_DATA_ERR_CRC)
			cmd->data->error = -EILSEQ;
		else if (ista & MESON_SDHC_ISTA_DATA_TIMEOUT)
			cmd->data->error = -ETIMEDOUT;
	}
}

In meson_mx_sdhc_irq_thread(), if cmd->error is -EIO or -ETIMEDOUT,
meson_mx_sdhc_reset() resets the controller. However, because the post-unmap
completion check only tests !data->error:

1. On a bounced multi-segment read, sg_copy_from_buffer() still copies len
   bytes from host->bounce_buf into data->sg even though the transfer failed
   or was aborted, overwriting the caller's scatterlist with uninitialized
   devm_kmalloc() heap memory or stale data from an earlier transfer.

2. data->bytes_xfered is set to len despite cmd->error being non-zero. In
   mmc_blk_mq_poll_completion() -> mmc_blk_mq_rw_recovery(), if CMD13 status
   polling finds the card in the transfer state without card-side error bits,
   recovery sees a non-zero brq->data.bytes_xfered and returns early:

drivers/mmc/core/block.c:mmc_blk_mq_rw_recovery() {
	...
	if (brq->data.bytes_xfered)
		return;
}

Could this cause mmc_blk_mq_complete_rq() to complete a failed read or write
request with BLK_STS_OK, resulting in silent data corruption or data loss?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031200925.3364-1-yuanzhenpeng210@gmail.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic