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