[v1] mmc: core: apply SD quirks earlier during probe

[PATCH] mmc: core: apply SD quirks earlier during probe

Posted by Keita Aihara 1 year, 6 months ago

Applying MMC_QUIRK_BROKEN_SD_CACHE is broken, as the card's extended
registers are parsed prior to the quirk being applied in mmc_blk.

Split this out into an SD-specific list of quirks and apply in
mmc_sd_init_card instead.

Fixes: c467c8f08185 ("mmc: Add MMC_QUIRK_BROKEN_SD_CACHE for Kingston Canvas Go Plus from 11/2019")
Authored-by: Jonathan Bell <jonathan@raspberrypi.com>
Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
Signed-off-by: Keita Aihara <keita.aihara@sony.com>
---
 drivers/mmc/core/quirks.h | 22 +++++++++++++---------
 drivers/mmc/core/sd.c     |  4 ++++
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index cca71867bc4a..92905fc46436 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -15,6 +15,19 @@

 #include "card.h"

+static const struct mmc_fixup __maybe_unused mmc_sd_fixups[] = {
+	/*
+	 * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
+	 * This has so far only been observed on cards from 11/2019, while new
+	 * cards from 2023/05 do not exhibit this behavior.
+	 */
+	_FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
+		   0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
+		   MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
+
+	END_FIXUP
+};
+
 static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
 #define INAND_CMD38_ARG_EXT_CSD  113
 #define INAND_CMD38_ARG_ERASE    0x00
@@ -53,15 +66,6 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
 	MMC_FIXUP("MMC32G", CID_MANFID_TOSHIBA, CID_OEMID_ANY, add_quirk_mmc,
 		  MMC_QUIRK_BLK_NO_CMD23),

-	/*
-	 * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
-	 * This has so far only been observed on cards from 11/2019, while new
-	 * cards from 2023/05 do not exhibit this behavior.
-	 */
-	_FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
-		   0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
-		   MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
-
 	/*
 	 * Some SD cards lockup while using CMD23 multiblock transfers.
 	 */
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 1c8148cdda50..ee37ad14e79e 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -26,6 +26,7 @@
 #include "host.h"
 #include "bus.h"
 #include "mmc_ops.h"
+#include "quirks.h"
 #include "sd.h"
 #include "sd_ops.h"

@@ -1475,6 +1476,9 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 			goto free_card;
 	}

+	/* Apply quirks prior to card setup */
+	mmc_fixup_device(card, mmc_sd_fixups);
+
 	err = mmc_sd_setup_card(host, card, oldcard != NULL);
 	if (err)
 		goto free_card;
--
2.43.2

[PATCH] mmc: core: apply SD quirks earlier during probe

Posted by Olexa Bilaniuk 1 year, 4 months ago

> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index cca71867bc4a..92905fc46436 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -15,6 +15,19 @@
> 
>  #include "card.h"
> 
> +static const struct mmc_fixup __maybe_unused mmc_sd_fixups[] = {
> +	/*
> +	 * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
> +	 * This has so far only been observed on cards from 11/2019, while new
> +	 * cards from 2023/05 do not exhibit this behavior.
> +	 */
> +	_FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
> +		   0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
> +		   MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
> +
> +	END_FIXUP
> +};
> +

By happenstance I have just such a Kingston Canvas Go! Plus card, but
in standard SD form-factor and not microSD, and dating from *08/2021*.
I had heretofore-inexplicable filesystem corruption problems with this
card that may well be attributable to a failure to flush the cache.
Despite diligence in issuing an explicit "sync" before ejecting, the
filesystem would reliably appear corrupt after reconnecting it, and
would require a "fsck", but no bad blocks could be identified.

The vitals of my card are:

  type:   SD
  name:   SD64G
  date:   08/2021
  manfid: 0x00009f (Kingston)
  oemid:  0x5449
  fwrev:  0x1
  hwrev:  0x6
  csd:    400e00325b590001cf9f7f800a400001
  ocr:    0x00300000

I think the quirk here can be safely broadened to all variants of
this card and to the date range 11/2019 - 08/2021 inclusive.


Sincerely,
Olexa Bilaniuk

Re: [PATCH] mmc: core: apply SD quirks earlier during probe

Posted by Ulf Hansson 1 year, 6 months ago

On Fri, 2 Aug 2024 at 05:21, Keita Aihara <keita.aihara@sony.com> wrote:
>
> Applying MMC_QUIRK_BROKEN_SD_CACHE is broken, as the card's extended
> registers are parsed prior to the quirk being applied in mmc_blk.

In what way is it a problem to read the extended registers first?

>
> Split this out into an SD-specific list of quirks and apply in
> mmc_sd_init_card instead.
>
> Fixes: c467c8f08185 ("mmc: Add MMC_QUIRK_BROKEN_SD_CACHE for Kingston Canvas Go Plus from 11/2019")
> Authored-by: Jonathan Bell <jonathan@raspberrypi.com>
> Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
> Signed-off-by: Keita Aihara <keita.aihara@sony.com>

Kind regards
Uffe

> ---
>  drivers/mmc/core/quirks.h | 22 +++++++++++++---------
>  drivers/mmc/core/sd.c     |  4 ++++
>  2 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index cca71867bc4a..92905fc46436 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -15,6 +15,19 @@
>
>  #include "card.h"
>
> +static const struct mmc_fixup __maybe_unused mmc_sd_fixups[] = {
> +       /*
> +        * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
> +        * This has so far only been observed on cards from 11/2019, while new
> +        * cards from 2023/05 do not exhibit this behavior.
> +        */
> +       _FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
> +                  0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
> +                  MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
> +
> +       END_FIXUP
> +};
> +
>  static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
>  #define INAND_CMD38_ARG_EXT_CSD  113
>  #define INAND_CMD38_ARG_ERASE    0x00
> @@ -53,15 +66,6 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
>         MMC_FIXUP("MMC32G", CID_MANFID_TOSHIBA, CID_OEMID_ANY, add_quirk_mmc,
>                   MMC_QUIRK_BLK_NO_CMD23),
>
> -       /*
> -        * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
> -        * This has so far only been observed on cards from 11/2019, while new
> -        * cards from 2023/05 do not exhibit this behavior.
> -        */
> -       _FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
> -                  0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
> -                  MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
> -
>         /*
>          * Some SD cards lockup while using CMD23 multiblock transfers.
>          */
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 1c8148cdda50..ee37ad14e79e 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -26,6 +26,7 @@
>  #include "host.h"
>  #include "bus.h"
>  #include "mmc_ops.h"
> +#include "quirks.h"
>  #include "sd.h"
>  #include "sd_ops.h"
>
> @@ -1475,6 +1476,9 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>                         goto free_card;
>         }
>
> +       /* Apply quirks prior to card setup */
> +       mmc_fixup_device(card, mmc_sd_fixups);
> +
>         err = mmc_sd_setup_card(host, card, oldcard != NULL);
>         if (err)
>                 goto free_card;
> --
> 2.43.2
>

Re: [PATCH] mmc: core: apply SD quirks earlier during probe

Posted by Keita Aihara 1 year, 6 months ago

On Mon, Aug 05, 2024 at 12:14:25PM +0200, Ulf Hansson wrote:
> On Fri, 2 Aug 2024 at 05:21, Keita Aihara <keita.aihara@sony.com> wrote:
> >
> > Applying MMC_QUIRK_BROKEN_SD_CACHE is broken, as the card's extended
> > registers are parsed prior to the quirk being applied in mmc_blk.
>
> In what way is it a problem to read the extended registers first?

SD quirks are referenced by mmc_card_broken_sd_cache() in
sd_parse_ext_reg_perf(). If the quirk is set, SD_EXT_PERF_CACHE is not
set to card->ext_perf.feature_support and the cache support will not be
enabled.

Therefore, SD quirks should be initialized before parsing the extension
registers.

>
> >
> > Split this out into an SD-specific list of quirks and apply in
> > mmc_sd_init_card instead.
> >
> > Fixes: c467c8f08185 ("mmc: Add MMC_QUIRK_BROKEN_SD_CACHE for Kingston Canvas Go Plus from 11/2019")
> > Authored-by: Jonathan Bell <jonathan@raspberrypi.com>
> > Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
> > Signed-off-by: Keita Aihara <keita.aihara@sony.com>
>
> Kind regards
> Uffe

Best regards,
Keita Aihara

>
> > ---
> >  drivers/mmc/core/quirks.h | 22 +++++++++++++---------
> >  drivers/mmc/core/sd.c     |  4 ++++
> >  2 files changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> > index cca71867bc4a..92905fc46436 100644
> > --- a/drivers/mmc/core/quirks.h
> > +++ b/drivers/mmc/core/quirks.h
> > @@ -15,6 +15,19 @@
> >
> >  #include "card.h"
> >
> > +static const struct mmc_fixup __maybe_unused mmc_sd_fixups[] = {
> > +       /*
> > +        * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
> > +        * This has so far only been observed on cards from 11/2019, while new
> > +        * cards from 2023/05 do not exhibit this behavior.
> > +        */
> > +       _FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
> > +                  0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
> > +                  MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
> > +
> > +       END_FIXUP
> > +};
> > +
> >  static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
> >  #define INAND_CMD38_ARG_EXT_CSD  113
> >  #define INAND_CMD38_ARG_ERASE    0x00
> > @@ -53,15 +66,6 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
> >         MMC_FIXUP("MMC32G", CID_MANFID_TOSHIBA, CID_OEMID_ANY, add_quirk_mmc,
> >                   MMC_QUIRK_BLK_NO_CMD23),
> >
> > -       /*
> > -        * Kingston Canvas Go! Plus microSD cards never finish SD cache flush.
> > -        * This has so far only been observed on cards from 11/2019, while new
> > -        * cards from 2023/05 do not exhibit this behavior.
> > -        */
> > -       _FIXUP_EXT("SD64G", CID_MANFID_KINGSTON_SD, 0x5449, 2019, 11,
> > -                  0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
> > -                  MMC_QUIRK_BROKEN_SD_CACHE, EXT_CSD_REV_ANY),
> > -
> >         /*
> >          * Some SD cards lockup while using CMD23 multiblock transfers.
> >          */
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 1c8148cdda50..ee37ad14e79e 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -26,6 +26,7 @@
> >  #include "host.h"
> >  #include "bus.h"
> >  #include "mmc_ops.h"
> > +#include "quirks.h"
> >  #include "sd.h"
> >  #include "sd_ops.h"
> >
> > @@ -1475,6 +1476,9 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> >                         goto free_card;
> >         }
> >
> > +       /* Apply quirks prior to card setup */
> > +       mmc_fixup_device(card, mmc_sd_fixups);
> > +
> >         err = mmc_sd_setup_card(host, card, oldcard != NULL);
> >         if (err)
> >                 goto free_card;
> > --
> > 2.43.2
> >

Re: [PATCH] mmc: core: apply SD quirks earlier during probe

Posted by Ulf Hansson 1 year, 5 months ago

On Tue, 6 Aug 2024 at 03:36, Keita Aihara <keita.aihara@sony.com> wrote:
>
> On Mon, Aug 05, 2024 at 12:14:25PM +0200, Ulf Hansson wrote:
> > On Fri, 2 Aug 2024 at 05:21, Keita Aihara <keita.aihara@sony.com> wrote:
> > >
> > > Applying MMC_QUIRK_BROKEN_SD_CACHE is broken, as the card's extended
> > > registers are parsed prior to the quirk being applied in mmc_blk.
> >
> > In what way is it a problem to read the extended registers first?
>
> SD quirks are referenced by mmc_card_broken_sd_cache() in
> sd_parse_ext_reg_perf(). If the quirk is set, SD_EXT_PERF_CACHE is not
> set to card->ext_perf.feature_support and the cache support will not be
> enabled.
>
> Therefore, SD quirks should be initialized before parsing the extension
> registers.

Makes perfect sense! Please include some of this information in the
commit message to make this clear.

>
> >
> > >
> > > Split this out into an SD-specific list of quirks and apply in
> > > mmc_sd_init_card instead.
> > >
> > > Fixes: c467c8f08185 ("mmc: Add MMC_QUIRK_BROKEN_SD_CACHE for Kingston Canvas Go Plus from 11/2019")
> > > Authored-by: Jonathan Bell <jonathan@raspberrypi.com>

This tag isn't normally what we use. I suggest you change the author
of the patch to Jonathan and keep his sob-tag.

Then add yourself with a "Co-developed-by" tag and keep your sob-tag.

> > > Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
> > > Signed-off-by: Keita Aihara <keita.aihara@sony.com>

[...]

Kind regards
Uffe

Re: [PATCH] mmc: core: apply SD quirks earlier during probe

Posted by Keita Aihara 1 year, 5 months ago

On Tue, Aug 20, 2024 at 12:10:36PM +0200, Ulf Hansson wrote:
> On Tue, 6 Aug 2024 at 03:36, Keita Aihara <keita.aihara@sony.com> wrote:
> >
> > On Mon, Aug 05, 2024 at 12:14:25PM +0200, Ulf Hansson wrote:
> > > On Fri, 2 Aug 2024 at 05:21, Keita Aihara <keita.aihara@sony.com> wrote:
> > > >
> > > > Applying MMC_QUIRK_BROKEN_SD_CACHE is broken, as the card's extended
> > > > registers are parsed prior to the quirk being applied in mmc_blk.
> > >
> > > In what way is it a problem to read the extended registers first?
> >
> > SD quirks are referenced by mmc_card_broken_sd_cache() in
> > sd_parse_ext_reg_perf(). If the quirk is set, SD_EXT_PERF_CACHE is not
> > set to card->ext_perf.feature_support and the cache support will not be
> > enabled.
> >
> > Therefore, SD quirks should be initialized before parsing the extension
> > registers.
>
> Makes perfect sense! Please include some of this information in the
> commit message to make this clear.

Sure.

>
> >
> > >
> > > >
> > > > Split this out into an SD-specific list of quirks and apply in
> > > > mmc_sd_init_card instead.
> > > >
> > > > Fixes: c467c8f08185 ("mmc: Add MMC_QUIRK_BROKEN_SD_CACHE for Kingston Canvas Go Plus from 11/2019")
> > > > Authored-by: Jonathan Bell <jonathan@raspberrypi.com>
>
> This tag isn't normally what we use. I suggest you change the author
> of the patch to Jonathan and keep his sob-tag.
>
> Then add yourself with a "Co-developed-by" tag and keep your sob-tag.

Thank you for your suggestion.
Removed "Authored-by" tag and added "Co-developed-by" tag to right after
the first "Signed-off-by" tag by Jonathan.

[PATCH v2] mmc: core: apply SD quirks earlier during probe

I hope the v2 patch meets the format to treat Jonathan as the patch
author.

>
> > > > Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
> > > > Signed-off-by: Keita Aihara <keita.aihara@sony.com>
>
> [...]
>
> Kind regards
> Uffe

Best regards,
Keita Aihara