Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230718011504.10947-1-wenchao.chen@unisoc.com> <b4ef97ba-440a-2641-0811-bb05e630ccb1@intel.com>
In-Reply-To: <b4ef97ba-440a-2641-0811-bb05e630ccb1@intel.com>
From:   Wenchao Chen <wenchao.chen666@gmail.com>
Date:   Wed, 19 Jul 2023 10:46:18 +0800
Message-ID: <CA+Da2qxOhK7Uc8_ONVgkR=3pTnTo7KgcJi-yS3Cv730+J0pAxA@mail.gmail.com>
Subject: Re: [PATCH] mmc: core: Remove FW revision from CID check
To:     Adrian Hunter <adrian.hunter@intel.com>
Cc:     Wenchao Chen <wenchao.chen@unisoc.com>, ulf.hansson@linaro.org,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhenxiong.lai@unisoc.com, chunyan.zhang@unisoc.com,
        yuelin.tang@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jul 18, 2023 at 2:13=E2=80=AFPM Adrian Hunter <adrian.hunter@intel.=
com> wrote:
>
> On 18/07/23 04:15, Wenchao Chen wrote:
> > When the card is reset, mmc_card_init() will check if this
> > card is the previous card by comparing the CID.
> >
> > If the firmware is upgraded, the product version may change,
> > so we remove the product version from the CID check.
>
> What is the use-case for this?  I would have thought it is safer
> not to assume anything about the card after the firmware has been
> upgraded.
>
Hi adrian
    Understood, but we have case:
    1.Before the firmware upgrade
        [T5745@C0] mmc0 oldcard raw->cid[2]: 32691160, raw->cid[3]: d924180=
0
        PRV=3D69
    2.After the firmware upgrade
        [T5745@C0] mmc0 cid[2]: 32011160 cid[3]: d9241800
        PRV=3D01
    If the PRV is not excluded in the CID check, then the mmc
initialization will fail after the mmc reset.
    In addition, CRC is excluded because some controllers support
SDHCI_QUIRK2_RSP_136_HAS_CRC.

> >
> > Signed-off-by: Wenchao Chen <wenchao.chen@unisoc.com>
> > ---
> >  drivers/mmc/core/mmc.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index 89cd48fcec79..32a73378d5c3 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -32,6 +32,9 @@
> >  #define MIN_CACHE_EN_TIMEOUT_MS 1600
> >  #define CACHE_FLUSH_TIMEOUT_MS 30000 /* 30s */
> >
> > +#define MMC_CID_PRV_MASK GENMASK(23, 16)
> > +#define MMC_CID_CRC_MASK GENMASK(7, 0)
> > +
> >  static const unsigned int tran_exp[] =3D {
> >       10000,          100000,         1000000,        10000000,
> >       0,              0,              0,              0
> > @@ -126,6 +129,19 @@ static int mmc_decode_cid(struct mmc_card *card)
> >       return 0;
> >  }
> >
> > +static int mmc_check_cid(u32 *cid, u32 *raw_cid)
> > +{
> > +     /*
> > +      * When comparing CID, we need to remove the product
> > +      * version (Field PRV, offset 55:48) and CRC. Because
> > +      * the product version will change when the firmware
> > +      * is upgraded. Also, the new CRC is different.
> > +      */
> > +     return cid[0] !=3D raw_cid[0] || cid[1] !=3D raw_cid[1] ||
> > +             (cid[2] & ~MMC_CID_PRV_MASK) !=3D (raw_cid[2] & ~MMC_CID_=
PRV_MASK) ||
> > +             (cid[3] & ~MMC_CID_CRC_MASK) !=3D (raw_cid[3] & ~MMC_CID_=
CRC_MASK);
> > +}
> > +
> >  static void mmc_set_erase_size(struct mmc_card *card)
> >  {
> >       if (card->ext_csd.erase_group_def & 1)
> > @@ -1640,7 +1656,7 @@ static int mmc_init_card(struct mmc_host *host, u=
32 ocr,
> >               goto err;
> >
> >       if (oldcard) {
> > -             if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) !=3D 0) {
> > +             if (mmc_check_cid(cid, oldcard->raw_cid)) {
> >                       pr_debug("%s: Perhaps the card was replaced\n",
> >                               mmc_hostname(host));
> >                       err =3D -ENOENT;
>