Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231001194943.658299-1-beanhuo@iokpp.de> <CAPDyKFpgPEhVuTbxe5YhYQ0W6907SwJRxSQLB1F2FbVW3zKHxg@mail.gmail.com>
 <254df9b8ca7475ad9dbd3a303c60e1f99eca9c56.camel@iokpp.de>
In-Reply-To: <254df9b8ca7475ad9dbd3a303c60e1f99eca9c56.camel@iokpp.de>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Tue, 10 Oct 2023 23:04:16 +0200
Message-ID: <CAPDyKFofJR0xtpmjCQXM7=VzUeN5jQLD2AZYEWocuyCnYNObYw@mail.gmail.com>
Subject: Re: [PATCH v4] mmc: Add quirk MMC_QUIRK_BROKEN_CACHE_FLUSH for Micron
 eMMC Q2J54A
To:     Bean Huo <beanhuo@iokpp.de>
Cc:     adrian.hunter@intel.com, beanhuo@micron.com,
        jakub.kwapisz@toradex.com, rafael.beims@toradex.com,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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