Re: [PATCH v4] mmc: Add quirk MMC_QUIRK_BROKEN_CACHE_FLUSH for Micron eMMC Q2J54A

[Ulf Hansson]

On Wed, 18 Oct 2023 at 18:05, Rafael Beims <rafael@xxxxxxxx> wrote:
>
> On 10/10/2023 18:04, Ulf Hansson wrote:
> > On Tue, 10 Oct 2023 at 17:33, Bean Huo <beanhuo@xxxxxxxx> 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