Re: [PATCH RFC 2/2] mmc: jz4740: Use the integrated DMA controller on the X1600

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mmc: jz4740: unreset bytes_xfered on idma read error masks I/O failure
--

commit 03cdfaf8519d89524e6b08dadbfa32014a811a97
Author: Olivier Louvignes <olivier@mgcrea.io>

mmc: jz4740: Use the integrated DMA controller on the X1600

This commit adds support for the MSC's integrated descriptor-based DMA
controller on the Ingenic X1600 when external DMA channels are not
provided in the device tree. It falls back to PIO for unaligned buffers,
transfers smaller than a block, or segments not aligned to the block size.

> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 6a3c26b7c82d8..300ad2a01b177 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c

[ ... ]

> @@ -371,13 +411,101 @@ static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host,

[ ... ]

> +static void jz4740_mmc_idma_wait_read(struct jz4740_mmc_host *host,
> +				      struct mmc_data *data)
> +{
> +	unsigned int timeout = 10000;
> +
> +	while (!jz4740_mmc_idma_read_done(host) && --timeout)
> +		udelay(1);
> +
> +	if (!timeout && !data->error) {
> +		dev_err(mmc_dev(host->mmc), "DMA did not finish a read\n");
> +		data->error = -EIO;
> +	}
> +}

[ ... ]

> @@ -786,6 +929,9 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
>  			 */
>  			timeout = jz4740_mmc_start_dma_transfer(host, data);
>  			data->bytes_xfered = data->blocks * data->blksz;
> +		} else if (host->idma_active) {
> +			/* Running since the command started */
> +			data->bytes_xfered = data->blocks * data->blksz;
>  		} else if (data->flags & MMC_DATA_READ)
>  			/* Use PIO if DMA is not enabled.
>  			 * Data transfer direction was defined before
> @@ -812,6 +958,9 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
>  		fallthrough;
>  
>  	case JZ4740_MMC_STATE_SEND_STOP:
> +		if (host->idma_active && (data->flags & MMC_DATA_READ))
> +			jz4740_mmc_idma_wait_read(host, data);

[Severity: High]
Can a failed or timed-out integrated DMA read complete with BLK_STS_OK and
return stale buffer contents to the caller?

In jz_mmc_irq_worker() under JZ4740_MMC_STATE_TRANSFER_DATA, when
host->idma_active is true, data->bytes_xfered is set to
data->blocks * data->blksz before JZ_MMC_IRQ_DATA_TRAN_DONE is polled and
before jz4740_mmc_idma_wait_read() verifies that the DMA engine has drained
the read data to memory.

If jz4740_mmc_idma_wait_read() times out in JZ4740_MMC_STATE_SEND_STOP, it
sets data->error = -EIO, but leaves data->bytes_xfered set to the full
request size (and if data->error was already set earlier by
jz4740_mmc_transfer_check_state() or jz_mmc_irq(), jz4740_mmc_idma_wait_read()
still spins for up to 10 ms while leaving data->bytes_xfered untouched).

When the request completes, mmc_blk_mq_poll_completion() enters
mmc_blk_mq_rw_recovery():

drivers/mmc/core/block.c:mmc_blk_mq_rw_recovery() {
    ...
	err = __mmc_send_status(card, &status, 0);
	if (err || mmc_blk_status_error(req, status))
		brq->data.bytes_xfered = 0;
    ...
	if (brq->data.bytes_xfered)
		return;
    ...
}

Because the bus transfer from the card finished cleanly and the failure
occurred in the host's integrated DMA controller, __mmc_send_status()
succeeds without card status error bits, so brq->data.bytes_xfered is not
cleared. Then mmc_blk_mq_complete_rq() sees nr_bytes equal to the full
request length and finishes the block request with BLK_STS_OK:

drivers/mmc/core/block.c:mmc_blk_mq_complete_rq() {
	struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
	unsigned int nr_bytes = mqrq->brq.data.bytes_xfered;

	if (nr_bytes) {
		if (blk_update_request(req, BLK_STS_OK, nr_bytes))
			blk_mq_requeue_request(req, true);
		else
			__blk_mq_end_request(req, BLK_STS_OK);
	} else if (!blk_rq_bytes(req)) {
    ...
}

Should data->bytes_xfered only be set after the transfer and
jz4740_mmc_idma_wait_read() succeed without data->error (or be reset to 0 on
error), and should jz4740_mmc_idma_wait_read() skip waiting when data->error
is already set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025140325.73204-1-olivier@mgcrea.io?part=2