diff mbox series

[v2,2/2] mmc: sh_mmcif: Advance sg_miter before reading blocks

Message ID 20240221-fix-sh-mmcif-v2-2-5e521eb25ae4@linaro.org
State New
Headers show
Series Try to fix the sg_mitet bugs in SH MMCIF | expand

Commit Message

Linus Walleij Feb. 21, 2024, 9:23 p.m. UTC 
The introduction of sg_miter was a bit sloppy as it didn't
exactly mimic the semantics of the old code on multiblock reads
and writes: these like you to:

- Advance to the first sglist entry *before* starting to read
  any blocks from the card.

- Advance and check availability of the next entry *right after*
  processing one block.

Not checking if we have more sglist entries right after
reading a block will lead to this not being checked until we
return to the callback to read out more blocks, i.e. until the
next interrupt arrives. Since the last block is the last one
(no more data will arrive) there will not be a next interrupt,
and we will be waiting forever resulting in a timeout for
command 18 when reading multiple blocks.

The same bug was fixed also in the writing of multiple blocks.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: 27b57277d9ba ("mmc: sh_mmcif: Use sg_miter for PIO")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/sh_mmcif.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

Comments

Geert Uytterhoeven Feb. 22, 2024, 9:20 a.m. UTC | #1 
On Wed, Feb 21, 2024 at 10:23 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> The introduction of sg_miter was a bit sloppy as it didn't
> exactly mimic the semantics of the old code on multiblock reads
> and writes: these like you to:
>
> - Advance to the first sglist entry *before* starting to read
>   any blocks from the card.
>
> - Advance and check availability of the next entry *right after*
>   processing one block.
>
> Not checking if we have more sglist entries right after
> reading a block will lead to this not being checked until we
> return to the callback to read out more blocks, i.e. until the
> next interrupt arrives. Since the last block is the last one
> (no more data will arrive) there will not be a next interrupt,
> and we will be waiting forever resulting in a timeout for
> command 18 when reading multiple blocks.
>
> The same bug was fixed also in the writing of multiple blocks.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: 27b57277d9ba ("mmc: sh_mmcif: Use sg_miter for PIO")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks, eMMC works again (tested on APE6EVM and KZM9-GT-Dual).
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 669555b5e8fa..08b4312af94e 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -653,6 +653,7 @@  static bool sh_mmcif_read_block(struct sh_mmcif_host *host)
 static void sh_mmcif_multi_read(struct sh_mmcif_host *host,
 				struct mmc_request *mrq)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct mmc_data *data = mrq->data;
 
 	if (!data->sg_len || !data->sg->length)
@@ -661,9 +662,15 @@  static void sh_mmcif_multi_read(struct sh_mmcif_host *host,
 	host->blocksize = sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
 		BLOCK_SIZE_MASK;
 
-	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
+	sg_miter_start(sgm, data->sg, data->sg_len,
 		       SG_MITER_TO_SG);
 
+	/* Advance to the first sglist entry */
+	if (!sg_miter_next(sgm)) {
+		sg_miter_stop(sgm);
+		return;
+	}
+
 	host->wait_for = MMCIF_WAIT_FOR_MREAD;
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
@@ -684,11 +691,6 @@  static bool sh_mmcif_mread_block(struct sh_mmcif_host *host)
 		return false;
 	}
 
-	if (!sg_miter_next(sgm)) {
-		sg_miter_stop(sgm);
-		return false;
-	}
-
 	p = sgm->addr;
 
 	for (i = 0; i < host->blocksize / 4; i++)
@@ -698,6 +700,11 @@  static bool sh_mmcif_mread_block(struct sh_mmcif_host *host)
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFREN);
 
+	if (!sg_miter_next(sgm)) {
+		sg_miter_stop(sgm);
+		return false;
+	}
+
 	return true;
 }
 
@@ -756,6 +763,7 @@  static bool sh_mmcif_write_block(struct sh_mmcif_host *host)
 static void sh_mmcif_multi_write(struct sh_mmcif_host *host,
 				struct mmc_request *mrq)
 {
+	struct sg_mapping_iter *sgm = &host->sg_miter;
 	struct mmc_data *data = mrq->data;
 
 	if (!data->sg_len || !data->sg->length)
@@ -764,9 +772,15 @@  static void sh_mmcif_multi_write(struct sh_mmcif_host *host,
 	host->blocksize = sh_mmcif_readl(host->addr, MMCIF_CE_BLOCK_SET) &
 		BLOCK_SIZE_MASK;
 
-	sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
+	sg_miter_start(sgm, data->sg, data->sg_len,
 		       SG_MITER_FROM_SG);
 
+	/* Advance to the first sglist entry */
+	if (!sg_miter_next(sgm)) {
+		sg_miter_stop(sgm);
+		return;
+	}
+
 	host->wait_for = MMCIF_WAIT_FOR_MWRITE;
 
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
@@ -787,11 +801,6 @@  static bool sh_mmcif_mwrite_block(struct sh_mmcif_host *host)
 		return false;
 	}
 
-	if (!sg_miter_next(sgm)) {
-		sg_miter_stop(sgm);
-		return false;
-	}
-
 	p = sgm->addr;
 
 	for (i = 0; i < host->blocksize / 4; i++)
@@ -799,6 +808,11 @@  static bool sh_mmcif_mwrite_block(struct sh_mmcif_host *host)
 
 	sgm->consumed = host->blocksize;
 
+	if (!sg_miter_next(sgm)) {
+		sg_miter_stop(sgm);
+		return false;
+	}
+
 	sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFWEN);
 
 	return true;