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.
Signed-off-by: Bean Huo beanhuo@micron.com Signed-off-by: Rafael Beims rafael.beims@toradex.com Cc: stable@vger.kernel.org
--- Changelog:
v3--v4: 1. Add helper function for this quirk in drivers/mmc/core/card.h. 2. Set card->written_flag only for REQ_OP_WRITE. v2--v3: 1. Set card->written_flag in mmc_blk_mq_issue_rq(). 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/block.c | 5 ++++- drivers/mmc/core/card.h | 4 ++++ drivers/mmc/core/mmc.c | 5 +++++ drivers/mmc/core/quirks.h | 7 ++++--- include/linux/mmc/card.h | 2 ++ 5 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3a8f27c3e310..dfa67d9c80bb 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break; - case REQ_OP_READ: case REQ_OP_WRITE: + if (mmc_card_broken_cache_flush(card) && !card->written_flag) + card->written_flag = true; + fallthrough; + case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq, req); else diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; }
+static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{ + return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH; +} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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 (mmc_card_broken_cache_flush(host->card) && !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 */
Hi Ulf,
Any comments on this version?
Kind regards,
Beam
On 01.10.23 9:49 PM, Bean Huo 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.
Signed-off-by: Bean Huo beanhuo@micron.com Signed-off-by: Rafael Beims rafael.beims@toradex.com Cc: stable@vger.kernel.org
Changelog:
v3--v4: 1. Add helper function for this quirk in drivers/mmc/core/card.h. 2. Set card->written_flag only for REQ_OP_WRITE. v2--v3: 1. Set card->written_flag in mmc_blk_mq_issue_rq(). 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/block.c | 5 ++++- drivers/mmc/core/card.h | 4 ++++ drivers/mmc/core/mmc.c | 5 +++++ drivers/mmc/core/quirks.h | 7 ++++--- include/linux/mmc/card.h | 2 ++ 5 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3a8f27c3e310..dfa67d9c80bb 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break;
case REQ_OP_READ:
if (mmc_card_broken_cache_flush(card) && !card->written_flag)card->written_flag = true;fallthrough;case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq, req); elsediff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; } +static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{
- return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH;
+} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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 (mmc_card_broken_cache_flush(host->card) && !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_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 */
Hi Ulf,
ping you again, have you recieved this mail?
Kind regards, Bean
On Wed, 2023-10-04 at 21:07 +0200, Bean Huo wrote:
Hi Ulf,
Any comments on this version?
Kind regards,
Beam
On 01.10.23 9:49 PM, Bean Huo 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.
Signed-off-by: Bean Huo beanhuo@micron.com Signed-off-by: Rafael Beims rafael.beims@toradex.com Cc: stable@vger.kernel.org
Changelog:
v3--v4: 1. Add helper function for this quirk in drivers/mmc/core/card.h. 2. Set card->written_flag only for REQ_OP_WRITE. v2--v3: 1. Set card->written_flag in mmc_blk_mq_issue_rq(). 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
On Sun, 1 Oct 2023 at 21:50, 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.
Signed-off-by: Bean Huo beanhuo@micron.com Signed-off-by: Rafael Beims rafael.beims@toradex.com Cc: stable@vger.kernel.org
Changelog:
v3--v4: 1. Add helper function for this quirk in drivers/mmc/core/card.h. 2. Set card->written_flag only for REQ_OP_WRITE. v2--v3: 1. Set card->written_flag in mmc_blk_mq_issue_rq(). 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/block.c | 5 ++++- drivers/mmc/core/card.h | 4 ++++ drivers/mmc/core/mmc.c | 5 +++++ drivers/mmc/core/quirks.h | 7 ++++--- include/linux/mmc/card.h | 2 ++ 5 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3a8f27c3e310..dfa67d9c80bb 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break;
case REQ_OP_READ: case REQ_OP_WRITE:
if (mmc_card_broken_cache_flush(card) && !card->written_flag)
It looks superfluous to me to check mmc_card_broken_cache_flush() and !card->written_flag. Just set the card->written_flag unconditionally.
card->written_flag = true;fallthrough;case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq, req); elsediff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; }
+static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{
return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH;+} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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;
How about doing this after a successful flush operation instead? In other words in _mmc_flush_cache().
return 0;free_card: @@ -2081,6 +2083,9 @@ static int _mmc_flush_cache(struct mmc_host *host) { int err = 0;
if (mmc_card_broken_cache_flush(host->card) && !host->card->written_flag)return err;
return 0;
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 todiff --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 comments, I didn't quite get your points:
On Tue, 2023-10-10 at 16:20 +0200, Ulf Hansson wrote:
@@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break; - case REQ_OP_READ: case REQ_OP_WRITE: + if (mmc_card_broken_cache_flush(card) && !card->written_flag)
It looks superfluous to me to check mmc_card_broken_cache_flush() and !card->written_flag. Just set the card->written_flag unconditionally.
what did you mean "Just set the card->written_flag unconditionally."? This means I just need to check card->written_flag and set card-
written_flag to true and false in the case
MMC_QUIRK_BROKEN_CACHE_FLUSH? or don't need to call mmc_card_broken_cache_flush()?
+ card->written_flag = true; + fallthrough; + case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq, req); else diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; }
+static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{ + return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH; +} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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;
How about doing this after a successful flush operation instead? In other words in _mmc_flush_cache().
Here initializes flag and the patch is intenting to eliminate the cache flush command before writing. what do you mean adding in mmc_flush_cache()?
Kind regards, Bean
On Tue, 10 Oct 2023 at 17:33, Bean Huo beanhuo@iokpp.de wrote:
Hi Ulf,
thanks for your comments, I didn't quite get your points:
On Tue, 2023-10-10 at 16:20 +0200, Ulf Hansson wrote:
@@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break;
case REQ_OP_READ: case REQ_OP_WRITE:
if (mmc_card_broken_cache_flush(card) &&!card->written_flag)
It looks superfluous to me to check mmc_card_broken_cache_flush() and !card->written_flag. Just set the card->written_flag unconditionally.
what did you mean "Just set the card->written_flag unconditionally."? This means I just need to check card->written_flag and set card-
written_flag to true and false in the case
MMC_QUIRK_BROKEN_CACHE_FLUSH? or don't need to call mmc_card_broken_cache_flush()?
I mean skip the checks above and just do the assignment below.
card->written_flag = true;fallthrough;case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq,req); else diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; }
+static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{
return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH;+} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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;How about doing this after a successful flush operation instead? In other words in _mmc_flush_cache().
Here initializes flag and the patch is intenting to eliminate the cache flush command before writing. what do you mean adding in mmc_flush_cache()?
mmc_init_card() is called while initializing and re-initializing the card. So, it certainly works to reset the flag from here.
However, _mmc_flush_cache() is called before powering off the card, which then would work similarly to the above, but also gets called for REQ_OP_FLUSH. Wouldn't that mean that we may be able to skip some unnecessary/troublesome cache flush requests if we would reset the flag from mmc_flush_cache(), rather than from mmc_init_card()?
Kind regards Uffe
On 10/10/2023 18:04, Ulf Hansson wrote:
On Tue, 10 Oct 2023 at 17:33, Bean Huo beanhuo@iokpp.de wrote:
Hi Ulf,
thanks for your comments, I didn't quite get your points:
On Tue, 2023-10-10 at 16:20 +0200, Ulf Hansson wrote:
@@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break;
case REQ_OP_READ: case REQ_OP_WRITE:
if (mmc_card_broken_cache_flush(card) &&!card->written_flag)
It looks superfluous to me to check mmc_card_broken_cache_flush() and !card->written_flag. Just set the card->written_flag unconditionally.
what did you mean "Just set the card->written_flag unconditionally."? This means I just need to check card->written_flag and set card-
written_flag to true and false in the case
MMC_QUIRK_BROKEN_CACHE_FLUSH? or don't need to call mmc_card_broken_cache_flush()?
I mean skip the checks above and just do the assignment below.
card->written_flag = true;fallthrough;case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq,req); else diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; }
+static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{
return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH;+} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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;How about doing this after a successful flush operation instead? In other words in _mmc_flush_cache().
Here initializes flag and the patch is intenting to eliminate the cache flush command before writing. what do you mean adding in mmc_flush_cache()?
mmc_init_card() is called while initializing and re-initializing the card. So, it certainly works to reset the flag from here.
However, _mmc_flush_cache() is called before powering off the card, which then would work similarly to the above, but also gets called for REQ_OP_FLUSH. Wouldn't that mean that we may be able to skip some unnecessary/troublesome cache flush requests if we would reset the flag from mmc_flush_cache(), rather than from mmc_init_card()?
Kind regards Uffe
Forgive me if I'm misunderstanding something, but wouldn't resetting the flag on _mmc_flush_cache() essentially disable cache when the MMC_QUIRK_BROKEN_CACHE_FLUSH is active?
From what I see, the card->written flag is supposed to stay true until we need to reinitialize the eMMC again, otherwise we would be skipping cache flushes we don't want to skip. It's only before writing for the first time that we want to skip the flush.
Regards, Rafael
On Wed, 18 Oct 2023 at 18:05, Rafael Beims rafael@beims.me wrote:
On 10/10/2023 18:04, Ulf Hansson wrote:
On Tue, 10 Oct 2023 at 17:33, Bean Huo beanhuo@iokpp.de wrote:
Hi Ulf,
thanks for your comments, I didn't quite get your points:
On Tue, 2023-10-10 at 16:20 +0200, Ulf Hansson wrote:
@@ -2381,8 +2381,11 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req) } ret = mmc_blk_cqe_issue_flush(mq, req); break;
case REQ_OP_READ: case REQ_OP_WRITE:
if (mmc_card_broken_cache_flush(card) &&!card->written_flag)
It looks superfluous to me to check mmc_card_broken_cache_flush() and !card->written_flag. Just set the card->written_flag unconditionally.
what did you mean "Just set the card->written_flag unconditionally."? This means I just need to check card->written_flag and set card-
written_flag to true and false in the case
MMC_QUIRK_BROKEN_CACHE_FLUSH? or don't need to call mmc_card_broken_cache_flush()?
I mean skip the checks above and just do the assignment below.
card->written_flag = true;fallthrough;case REQ_OP_READ: if (host->cqe_enabled) ret = mmc_blk_cqe_issue_rw_rq(mq,req); else diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 4edf9057fa79..b7754a1b8d97 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -280,4 +280,8 @@ static inline int mmc_card_broken_sd_cache(const struct mmc_card *c) return c->quirks & MMC_QUIRK_BROKEN_SD_CACHE; }
+static inline int mmc_card_broken_cache_flush(const struct mmc_card *c) +{
return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH;+} #endif diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 89cd48fcec79..47896c32086e 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;How about doing this after a successful flush operation instead? In other words in _mmc_flush_cache().
Here initializes flag and the patch is intenting to eliminate the cache flush command before writing. what do you mean adding in mmc_flush_cache()?
mmc_init_card() is called while initializing and re-initializing the card. So, it certainly works to reset the flag from here.
However, _mmc_flush_cache() is called before powering off the card, which then would work similarly to the above, but also gets called for REQ_OP_FLUSH. Wouldn't that mean that we may be able to skip some unnecessary/troublesome cache flush requests if we would reset the flag from mmc_flush_cache(), rather than from mmc_init_card()?
Kind regards Uffe
Forgive me if I'm misunderstanding something, but wouldn't resetting the flag on _mmc_flush_cache() essentially disable cache when the MMC_QUIRK_BROKEN_CACHE_FLUSH is active?
It would, until after there has been a new I/O write. Is that a problem?
From what I see, the card->written flag is supposed to stay true until we need to reinitialize the eMMC again, otherwise we would be skipping cache flushes we don't want to skip. It's only before writing for the first time that we want to skip the flush.
If that's really what we want, that's perfectly okay to me. The commit message isn't really clear about this, so I think that Bean could improve that too.
I guess we need some kind of confirmation from Bean about this. Don't get me wrong, I am happy to go with whatever approach as long as it's clear *why* the selected approach is taken.
Kind regards Uffe
linux-stable-mirror@lists.linaro.org
-
Bean Huo -
Bean Huo -
Rafael Beims -
Ulf Hansson