This adresses some bugs found after the introduction of the
memory iterator sg_miter to the sh mmcif driver.
This was first just one patch for fixing the atomic bug, but
now also a second patch is needed to fix a semantic issue.
Signed-off-by: Linus Walleij <[email protected]>
---
Changes in v2:
- Collect Geerts test tag on patch 1
- Add a second patch fixing the problem with advancing to
the next sglist entry before reading/writing the first block and
after reading/writing each block in a multiblock operation.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Linus Walleij (2):
mmc: sh_mmcif: sg_miter does not need to be atomic
mmc: sh_mmcif: Advance sg_miter before reading blocks
drivers/mmc/host/sh_mmcif.c | 46 +++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
---
base-commit: 2d5c7b7eb345249cb34d42cbc2b97b4c57ea944e
change-id: 20240220-fix-sh-mmcif-49c1de70c5b7
Best regards,
--
Linus Walleij <[email protected]>
All the sglist iterations happen in the *threaded* interrupt handler
and that context is not atomic, so don't request an atomic
sglist miter. Using an atomic miter results in "BUG: scheduling while
atomic" splats.
Reported-by: Geert Uytterhoeven <[email protected]>
Fixes: 27b57277d9ba ("mmc: sh_mmcif: Use sg_miter for PIO")
Tested-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
Hi Geert, it'd be great if you could test this!
---
drivers/mmc/host/sh_mmcif.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 1ef6e153e5a3..669555b5e8fa 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -607,7 +607,7 @@ static void sh_mmcif_single_read(struct sh_mmcif_host *host,
BLOCK_SIZE_MASK) + 3;
sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
- SG_MITER_ATOMIC | SG_MITER_TO_SG);
+ SG_MITER_TO_SG);
host->wait_for = MMCIF_WAIT_FOR_READ;
@@ -662,7 +662,7 @@ static void sh_mmcif_multi_read(struct sh_mmcif_host *host,
BLOCK_SIZE_MASK;
sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
- SG_MITER_ATOMIC | SG_MITER_TO_SG);
+ SG_MITER_TO_SG);
host->wait_for = MMCIF_WAIT_FOR_MREAD;
@@ -710,7 +710,7 @@ static void sh_mmcif_single_write(struct sh_mmcif_host *host,
BLOCK_SIZE_MASK) + 3;
sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
- SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+ SG_MITER_FROM_SG);
host->wait_for = MMCIF_WAIT_FOR_WRITE;
@@ -765,7 +765,7 @@ static void sh_mmcif_multi_write(struct sh_mmcif_host *host,
BLOCK_SIZE_MASK;
sg_miter_start(&host->sg_miter, data->sg, data->sg_len,
- SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+ SG_MITER_FROM_SG);
host->wait_for = MMCIF_WAIT_FOR_MWRITE;
--
2.34.1
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 <[email protected]>
Fixes: 27b57277d9ba ("mmc: sh_mmcif: Use sg_miter for PIO")
Signed-off-by: Linus Walleij <[email protected]>
---
drivers/mmc/host/sh_mmcif.c | 38 ++++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)
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;
--
2.34.1
Hi Linus,
On Wed, Feb 21, 2024 at 10:23 PM Linus Walleij <[email protected]> wrote:
> All the sglist iterations happen in the *threaded* interrupt handler
> and that context is not atomic, so don't request an atomic
> sglist miter. Using an atomic miter results in "BUG: scheduling while
> atomic" splats.
>
> Reported-by: Geert Uytterhoeven <[email protected]>
> Fixes: 27b57277d9ba ("mmc: sh_mmcif: Use sg_miter for PIO")
> Tested-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> Hi Geert, it'd be great if you could test this!
Done before (see the Tb above ;-)
You probably still want to s/does not need to/must not/?
(sorry, I miswrote that previously)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Feb 21, 2024 at 10:23 PM Linus Walleij <[email protected]> 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 <[email protected]>
> Fixes: 27b57277d9ba ("mmc: sh_mmcif: Use sg_miter for PIO")
> Signed-off-by: Linus Walleij <[email protected]>
Thanks, eMMC works again (tested on APE6EVM and KZM9-GT-Dual).
Tested-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 21 Feb 2024 at 22:23, Linus Walleij <[email protected]> wrote:
>
> This adresses some bugs found after the introduction of the
> memory iterator sg_miter to the sh mmcif driver.
>
> This was first just one patch for fixing the atomic bug, but
> now also a second patch is needed to fix a semantic issue.
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> Changes in v2:
> - Collect Geerts test tag on patch 1
> - Add a second patch fixing the problem with advancing to
> the next sglist entry before reading/writing the first block and
> after reading/writing each block in a multiblock operation.
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> Linus Walleij (2):
> mmc: sh_mmcif: sg_miter does not need to be atomic
> mmc: sh_mmcif: Advance sg_miter before reading blocks
>
> drivers/mmc/host/sh_mmcif.c | 46 +++++++++++++++++++++++++++++----------------
> 1 file changed, 30 insertions(+), 16 deletions(-)
> ---
Applied for next and by amending the commit message header as pointed
out by Geert, thanks!
Kind regards
Uffe