RE: [PATCH] mmc: truncate quirks' oemid to 8 bits

[Avri Altman]

> On Thu, 26 Oct 2023 at 09:52, Dominique Martinet
> <dominique.martinet@xxxxxxxxxxxxxxxxx> 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.
Agreed.

> 
> 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?
Or some random garbage...

> 
> Beyond v5.1A, we may have a problem as the BIN may actually be used for
> something valuable. Maybe Avri knows more here?
AFAIK, we don't use it.  But I can ask around.

Yeah, I think its best just to revert it.
If an eMMC vendor has an issue with this 16bits bogus oemid (Sandisk does) -
they can handle their oemid-specific quirks - I know I will.

Please note that it was picked by stable as well.

Thanks,
Avri
> 
> 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@xxxxxxxxxxxxx
> > Signed-off-by: Dominique Martinet
> > <dominique.martinet@xxxxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Cc: Avri Altman <avri.altman@xxxxxxx>
> > Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > Cc: Alex Fetters <Alex.Fetters@xxxxxxxxxx>
> > ---
> > 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
> >
> >