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

[Rafael Beims]

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?

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