| Message ID | 20240802032121.GA4019194@sony.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: core: apply SD quirks earlier during probe | expand |
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 >
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 > >
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
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
> 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
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;
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(-) -- 2.43.2