| Message ID | 20230914202749.470100-1-beanhuo@iokpp.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mmc: Add quirk MMC_QUIRK_BROKEN_CACHE_FLUSH for Micron eMMC Q2J54A | expand |
On Thu, 14 Sept 2023 at 22:28, Bean Huo <beanhuo@iokpp.de> wrote: > > From: Bean Huo <beanhuo@micron.com> > > Micron MTFC4GACAJCN eMMC supports cache but requires that flush cache > operation be allowed only after a write has occurred. Otherwise, the > cache flush command or subsequent commands will time out. For my information, more exactly, how can we trigger this problem? > > Signed-off-by: Bean Huo <beanhuo@micron.com> > Co-developed-by: Rafael Beims <rafael.beims@toradex.com> > Tested-by: Rafael Beims <rafael.beims@toradex.com> > Cc: stable@vger.kernel.org > --- > Changelog: > > v1--v2: > 1. Add Rafael's test-tag, and Co-developed-by. > 2. Check host->card whether NULL or not in __mmc_start_request() before asserting host->card->->quirks > > --- > drivers/mmc/core/core.c | 7 +++++++ > drivers/mmc/core/mmc.c | 5 +++++ > drivers/mmc/core/quirks.h | 7 ++++--- > include/linux/mmc/card.h | 2 ++ > 4 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 3d3e0ca52614..86a669b35b91 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -259,6 +259,13 @@ static void __mmc_start_request(struct mmc_host *host, struct mmc_request *mrq) > host->cqe_ops->cqe_off(host); > > host->ops->request(host, mrq); > + > + if (host->card && host->card->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH && > + !host->card->written_flag) { > + if (mrq->cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK || > + mrq->cmd->opcode == MMC_WRITE_BLOCK) > + host->card->written_flag = true; > + } I don't quite like that we are adding the above code here - as it's used for *all* requests. Seems like the flag is better set from the mmc block device driver instead. Somewhere in the path when we serve I/O write requests. > } > > static void mmc_mrq_pr_debug(struct mmc_host *host, struct mmc_request *mrq, > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 89cd48fcec79..a2edd065fa1b 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1929,6 +1929,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, > if (!oldcard) > host->card = card; > > + card->written_flag = false; > + According to your earlier reply, it sounds like the problem isn't really about the card being re-initialized, but rather that we actually need a write request to happen before a flush. No matter what, no? See more about this below. > return 0; > > free_card: > @@ -2081,6 +2083,9 @@ static int _mmc_flush_cache(struct mmc_host *host) > { > int err = 0; > > + if (host->card->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH && !host->card->written_flag) > + return err; > + Could an option to the above, be to reset the flag here instead. After a successful cache flush has been done. > if (_mmc_cache_enabled(host)) { > err = mmc_switch(host->card, EXT_CSD_CMD_SET_NORMAL, > EXT_CSD_FLUSH_CACHE, 1, > diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h > index 32b64b564fb1..5e68c8b4cdca 100644 > --- a/drivers/mmc/core/quirks.h > +++ b/drivers/mmc/core/quirks.h > @@ -110,11 +110,12 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = { > MMC_QUIRK_TRIM_BROKEN), > > /* > - * Micron MTFC4GACAJCN-1M advertises TRIM but it does not seems to > - * support being used to offload WRITE_ZEROES. > + * Micron MTFC4GACAJCN-1M supports TRIM but does not appear to suppor > + * WRITE_ZEROES offloading. It also supports caching, but the cache can > + * only be flushed after a write has occurred. > */ > MMC_FIXUP("Q2J54A", CID_MANFID_MICRON, 0x014e, add_quirk_mmc, > - MMC_QUIRK_TRIM_BROKEN), > + MMC_QUIRK_TRIM_BROKEN | MMC_QUIRK_BROKEN_CACHE_FLUSH), > > /* > * Kingston EMMC04G-M627 advertises TRIM but it does not seems to > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h > index daa2f40d9ce6..7b12eebc5586 100644 > --- a/include/linux/mmc/card.h > +++ b/include/linux/mmc/card.h > @@ -295,7 +295,9 @@ struct mmc_card { > #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI support */ > #define MMC_QUIRK_BROKEN_SD_DISCARD (1<<14) /* Disable broken SD discard support */ > #define MMC_QUIRK_BROKEN_SD_CACHE (1<<15) /* Disable broken SD cache support */ > +#define MMC_QUIRK_BROKEN_CACHE_FLUSH (1<<16) /* Don't flush cache until the write has occurred */ > > + bool written_flag; /* Indicates eMMC has been written since power on */ > bool reenable_cmdq; /* Re-enable Command Queue */ > > unsigned int erase_size; /* erase size in sectors */ > -- > 2.34.1 > Kind regards Uffe
Hi Ulf, Thanks for your comment, much appreciated! On Fri, 2023-09-15 at 00:09 +0200, Ulf Hansson wrote: > On Thu, 14 Sept 2023 at 22:28, Bean Huo <beanhuo@iokpp.de> wrote: > > > > From: Bean Huo <beanhuo@micron.com> > > > > Micron MTFC4GACAJCN eMMC supports cache but requires that flush > > cache > > operation be allowed only after a write has occurred. Otherwise, > > the > > cache flush command or subsequent commands will time out. > > For my information, more exactly, how can we trigger this problem? > This issue may be likely reproduced in this command sequence: eMMC power-cycle/reset-->...(other operations, but no data write)...- >cache flush-->...... ->write data, must say, it is not 100% reproducable. > > > > Signed-off-by: Bean Huo <beanhuo@micron.com> > > Co-developed-by: Rafael Beims <rafael.beims@toradex.com> > > Tested-by: Rafael Beims <rafael.beims@toradex.com> > > Cc: stable@vger.kernel.org > > --- > > Changelog: > > > > v1--v2: > > 1. Add Rafael's test-tag, and Co-developed-by. > > 2. Check host->card whether NULL or not in > > __mmc_start_request() before asserting host->card->->quirks > > > > --- > > drivers/mmc/core/core.c | 7 +++++++ > > drivers/mmc/core/mmc.c | 5 +++++ > > drivers/mmc/core/quirks.h | 7 ++++--- > > include/linux/mmc/card.h | 2 ++ > > 4 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > > index 3d3e0ca52614..86a669b35b91 100644 > > --- a/drivers/mmc/core/core.c > > +++ b/drivers/mmc/core/core.c > > @@ -259,6 +259,13 @@ static void __mmc_start_request(struct > > mmc_host *host, struct mmc_request *mrq) > > host->cqe_ops->cqe_off(host); > > > > host->ops->request(host, mrq); > > + > > + if (host->card && host->card->quirks & > > MMC_QUIRK_BROKEN_CACHE_FLUSH && > > + !host->card->written_flag) { > > + if (mrq->cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK || > > + mrq->cmd->opcode == MMC_WRITE_BLOCK) > > + host->card->written_flag = true; > > + } > > I don't quite like that we are adding the above code here - as it's > used for *all* requests. > > Seems like the flag is better set from the mmc block device driver > instead. Somewhere in the path when we serve I/O write requests. > yes, you are correct, I will update the patch and add this flag set in mmc block driver mmc_blk_mq_issue_rq(). > > } > > > > static void mmc_mrq_pr_debug(struct mmc_host *host, struct > > mmc_request *mrq, > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > > index 89cd48fcec79..a2edd065fa1b 100644 > > --- a/drivers/mmc/core/mmc.c > > +++ b/drivers/mmc/core/mmc.c > > @@ -1929,6 +1929,8 @@ static int mmc_init_card(struct mmc_host > > *host, u32 ocr, > > if (!oldcard) > > host->card = card; > > > > + card->written_flag = false; > > + > > According to your earlier reply, it sounds like the problem isn't > really about the card being re-initialized, but rather that we > actually need a write request to happen before a flush. No matter > what, no? > > See more about this below. > Actually, it matters that the first cache flush command after reboot/reset. means that for the first cache flush command, before execution, the write data operation should occur, After that, there is no problem even if there are no writes before the cache is flushed. > > return 0; > > > > free_card: > > @@ -2081,6 +2083,9 @@ static int _mmc_flush_cache(struct mmc_host > > *host) > > { > > int err = 0; > > > > + if (host->card->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH && > > !host->card->written_flag) > > + return err; > > + > > Could an option to the above, be to reset the flag here instead. > After > a successful cache flush has been done. > > As I explained above, the first cache flush after a power cycle/reset is important. We just want to eliminate the first unnecessary cache flush. We can eliminate all unnecessary cache flushes when the cache is empty. But I don't want to change the current logic too much, only want to focus on the first unnecessary cache flush, how do you think? Kind regards, Bean
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 3d3e0ca52614..86a669b35b91 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -259,6 +259,13 @@ static void __mmc_start_request(struct mmc_host *host, struct mmc_request *mrq) host->cqe_ops->cqe_off(host); host->ops->request(host, mrq); + + if (host->card && host->card->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH && + !host->card->written_flag) { + if (mrq->cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK || + mrq->cmd->opcode == MMC_WRITE_BLOCK) + host->card->written_flag = true; + } } static void mmc_mrq_pr_debug(struct mmc_host *host, struct mmc_request *mrq, diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..a2edd065fa1b 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1929,6 +1929,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, if (!oldcard) host->card = card; + card->written_flag = false; + return 0; free_card: @@ -2081,6 +2083,9 @@ static int _mmc_flush_cache(struct mmc_host *host) { int err = 0; + if (host->card->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH && !host->card->written_flag) + return err; + if (_mmc_cache_enabled(host)) { err = mmc_switch(host->card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_FLUSH_CACHE, 1, diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h index 32b64b564fb1..5e68c8b4cdca 100644 --- a/drivers/mmc/core/quirks.h +++ b/drivers/mmc/core/quirks.h @@ -110,11 +110,12 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = { MMC_QUIRK_TRIM_BROKEN), /* - * Micron MTFC4GACAJCN-1M advertises TRIM but it does not seems to - * support being used to offload WRITE_ZEROES. + * Micron MTFC4GACAJCN-1M supports TRIM but does not appear to suppor + * WRITE_ZEROES offloading. It also supports caching, but the cache can + * only be flushed after a write has occurred. */ MMC_FIXUP("Q2J54A", CID_MANFID_MICRON, 0x014e, add_quirk_mmc, - MMC_QUIRK_TRIM_BROKEN), + MMC_QUIRK_TRIM_BROKEN | MMC_QUIRK_BROKEN_CACHE_FLUSH), /* * Kingston EMMC04G-M627 advertises TRIM but it does not seems to diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index daa2f40d9ce6..7b12eebc5586 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -295,7 +295,9 @@ struct mmc_card { #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI support */ #define MMC_QUIRK_BROKEN_SD_DISCARD (1<<14) /* Disable broken SD discard support */ #define MMC_QUIRK_BROKEN_SD_CACHE (1<<15) /* Disable broken SD cache support */ +#define MMC_QUIRK_BROKEN_CACHE_FLUSH (1<<16) /* Don't flush cache until the write has occurred */ + bool written_flag; /* Indicates eMMC has been written since power on */ bool reenable_cmdq; /* Re-enable Command Queue */ unsigned int erase_size; /* erase size in sectors */