Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <c66810ae52446db41fd187ee4be24d04d40c1abb.camel@iokpp.de>
Subject: Re: [PATCH v2] mmc: Add quirk MMC_QUIRK_BROKEN_CACHE_FLUSH for
 Micron eMMC Q2J54A
From:   Bean Huo <beanhuo@iokpp.de>
To:     Ulf Hansson <ulf.hansson@linaro.org>
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
Date:   Sun, 17 Sep 2023 22:11:24 +0200
In-Reply-To: <CAPDyKFruYooG4yQ4yqrwEcrvy4xe6hYByF2Q57eQ1g0tiUtcug@mail.gmail.com>
References: <20230914202749.470100-1-beanhuo@iokpp.de>
         <CAPDyKFruYooG4yQ4yqrwEcrvy4xe6hYByF2Q57eQ1g0tiUtcug@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Precedence: bulk


Hi Ulf,

Thanks for your comment, much appreciated!


On Fri, 2023-09-15 at 00:09 +0200, Ulf Hansson wrote:
> On Thu, 14 Sept 2023 at 22:28, Bean Huo <beanhuo@iokpp.de> wrote:
> >=20
> > From: Bean Huo <beanhuo@micron.com>
> >=20
> > 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.
>=20
> For my information, more exactly, how can we trigger this problem?
>=20

This issue may be likely reproduced in this command sequence:

eMMC power-cycle/reset-->...(other operations, but no data write)...-
>cache flush-->...... ->write data, must say, it is not 100%
reproducable.

> >=20
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > Co-developed-by: Rafael Beims <rafael.beims@toradex.com>
> > Tested-by: Rafael Beims <rafael.beims@toradex.com>
> > Cc: stable@vger.kernel.org
> > ---
> > Changelog:
> >=20
> > v1--v2:
> > =C2=A0=C2=A0=C2=A0 1. Add Rafael's test-tag, and Co-developed-by.
> > =C2=A0=C2=A0=C2=A0 2. Check host->card whether NULL or not in
> > __mmc_start_request() before asserting host->card->->quirks
> >=20
> > ---
> > =C2=A0drivers/mmc/core/core.c=C2=A0=C2=A0 | 7 +++++++
> > =C2=A0drivers/mmc/core/mmc.c=C2=A0=C2=A0=C2=A0 | 5 +++++
> > =C2=A0drivers/mmc/core/quirks.h | 7 ++++---
> > =C2=A0include/linux/mmc/card.h=C2=A0 | 2 ++
> > =C2=A04 files changed, 18 insertions(+), 3 deletions(-)
> >=20
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 3d3e0ca52614..86a669b35b91 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -259,6 +259,13 @@ static void __mmc_start_request(struct
> > mmc_host *host, struct mmc_request *mrq)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 host->cqe_ops->cqe_off(host);
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 host->ops->request(host, mrq=
);
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (host->card && host->card->qui=
rks &
> > MMC_QUIRK_BROKEN_CACHE_FLUSH &&
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !host->ca=
rd->written_flag) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 if (mrq->cmd->opcode =3D=3D MMC_WRITE_MULTIPLE_BLOCK ||
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mrq->cmd->opcode =3D=3D MMC_WRITE_B=
LOCK)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 host->card-=
>written_flag =3D true;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>=20
> I don't quite like that we are adding the above code here - as it's
> used for *all* requests.
>=20
> Seems like the flag is better set from the mmc block device driver
> instead. Somewhere in the path when we serve I/O write requests.
>=20

yes, you are correct, I will update the patch and add this flag set in
mmc block driver mmc_blk_mq_issue_rq().

> > =C2=A0}
> >=20
> > =C2=A0static void mmc_mrq_pr_debug(struct mmc_host *host, struct
> > mmc_request *mrq,
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index 89cd48fcec79..a2edd065fa1b 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,
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!oldcard)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 host->card =3D card;
> >=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 card->written_flag =3D false;
> > +
>=20
> According to your earlier reply, it sounds like the problem isn't
> really about the card being re-initialized, but rather that we
> actually need a write request to happen before a flush. No matter
> what, no?
>=20
> See more about this below.
>=20

Actually, it matters that the first cache flush command after
reboot/reset. means that for the first cache flush command, before
execution, the write data operation should occur,
After that, there is no problem even if there are no writes before the
cache is flushed.


> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0;
> >=20
> > =C2=A0free_card:
> > @@ -2081,6 +2083,9 @@ static int _mmc_flush_cache(struct mmc_host
> > *host)
> > =C2=A0{
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int err =3D 0;
> >=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (host->card->quirks & MMC_QUIR=
K_BROKEN_CACHE_FLUSH &&
> > !host->card->written_flag)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 return err;
> > +
>=20
> Could an option to the above, be to reset the flag here instead.
> After
> a successful cache flush has been done.
>=20
>=20

As I explained above, the first cache flush after a power cycle/reset
is important. We just want to eliminate the first unnecessary cache
flush.
We can eliminate all unnecessary cache flushes when the cache is empty.
But I don't want to change the current logic too much, only want to
focus on the first unnecessary cache flush, how do you think?

Kind regards,
Bean