Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231026075230.414685-1-dominique.martinet@atmark-techno.com>
In-Reply-To: <20231026075230.414685-1-dominique.martinet@atmark-techno.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 2 Nov 2023 14:25:09 +0100
Message-ID: <CAPDyKFqkKibcXnwjnhc3+W1iJBHLeqQ9BpcZrSwhW2u9K2oUtg@mail.gmail.com>
Subject: Re: [PATCH] mmc: truncate quirks' oemid to 8 bits
To:     Avri Altman <avri.altman@wdc.com>,
        Dominique Martinet <dominique.martinet@atmark-techno.com>
Cc:     Dominique Martinet <asmadeus@codewreck.org>,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Alex Fetters <Alex.Fetters@garmin.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 26 Oct 2023 at 09:52, Dominique Martinet
<dominique.martinet@atmark-techno.com> wrote:
>
> We now only capture 8 bits for oemid in card->cid.oemid, so quirks that
> were filling up the full 16 bits up till now would no longer apply.

Huh, thanks for spotting this!

>
> Work around the problem by only checking for the bottom 8 bits when
> checking if quirks should be applied
>
> Fixes: 84ee19bffc93 ("mmc: core: Capture correct oemid-bits for eMMC cards")

I wonder if the quirk approach is really the correct thing to do. I
had a closer look around what has changed along the new versions of
the MMC/eMMC specs, the below is what I found.

Before v4.3: OID [119:104] 16-bits.
Between v4.3-v5.1: OID [111:104] 8-bits, CBX [113:112] 2-bits,
reserved [119:114] 6-bits.
Beyond v5.1A: OID [111:104] 8-bits, CBX [113:112] 2-bits, BIN [119:114] 6-bits.

OID: OEM/Application ID
CBX: Device/BGA
BIN: Bank Index Number

It looks to me that the offending commit (84ee19bffc93) should be
reverted instead of trying to introduce some weird parsing of the card
quirks.

In fact, up until v5.1 it seems not to be a problem to use 16-bits for
the OID, as the CBX and the reserved bits are probably just given some
fixed values by the vendors, right?

Beyond v5.1A, we may have a problem as the BIN may actually be used
for something valuable. Maybe Avri knows more here?

That said, if the offending commit is really needed to fix a problem,
we need to figure out exactly what that problem is. The EXT_CSD_REV
doesn't provide us with the exact version that the card is supporting,
but at least we know if v5.1 and onwards is supported, so perhaps that
can be used to fixup/improve the OID/CBX/BIN parsing.

Kind regards
Uffe

> Link: https://lkml.kernel.org/r/ZToJsSLHr8RnuTHz@codewreck.org
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> Cc: stable@vger.kernel.org
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Alex Fetters <Alex.Fetters@garmin.com>
> ---
> Notes:
>  - mmc_fixup_device() was rewritten in 5.17, so older stable kernels
>    will need a separate patch... I suppose I can send it to stable
>    after this is merged if we go this way
>  - struct mmc_cid's and mmc_fixup's oemid fields are unsigned shorts,
>    we probably just want to make them unsigned char instead in which
>    case we don't need that check anymore?
>    But it's kind of nice to have a wider type so CID_OEMID_ANY can never
>    be a match.... Which unfortunately my patch makes moot as
>    ((unsigned short)-1) & 0xff will be 0xff which can match anything...
>  - this could also be worked around in the _FIXUP_EXT macro that builds
>    the fixup structs, but we're getting ugly here... Or we can just go
>    for the big boom and try to fix all MMC_FIXUP() users in tree and
>    call it a day, but that'll also be fun to backport.
>
>  drivers/mmc/core/quirks.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index 32b64b564fb1..27e0349e176d 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -211,8 +211,9 @@ static inline void mmc_fixup_device(struct mmc_card *card,
>                 if (f->manfid != CID_MANFID_ANY &&
>                     f->manfid != card->cid.manfid)
>                         continue;
> +               /* Only the bottom 8bits are valid in JESD84-B51 */
>                 if (f->oemid != CID_OEMID_ANY &&
> -                   f->oemid != card->cid.oemid)
> +                    (f->oemid & 0xff) != (card->cid.oemid & 0xff))
>                         continue;
>                 if (f->name != CID_NAME_ANY &&
>                     strncmp(f->name, card->cid.prod_name,
> --
> 2.39.2
>
>