mmc: truncate quirks' oemid to 8 bits

Commit Message

Dominique MARTINET Oct. 26, 2023, 7:52 a.m. UTC

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.

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")
Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Dominique Martinet <[e-mail]>
Cc: [e-mail]
Cc: Avri Altman <[e-mail]>
Cc: Ulf Hansson <[e-mail]>
Cc: Alex Fetters <[e-mail]>
---
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(-)

Comments

Avri Altman Oct. 26, 2023, 10:16 a.m. UTC | #1

> 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.
> 
> 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")
> Link: https://lkml.kernel.org/r/[e-mail]
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-
> techno.com>
> Cc: [e-mail]
> Cc: Avri Altman <[e-mail]>
> Cc: Ulf Hansson <[e-mail]>
> Cc: Alex Fetters <[e-mail]>
Reviewed-by: Avri Altman <[e-mail]>

> ---
> 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.
To me, your fix is clean, elegant and does the job.
I would let the quirk owners to fix that hard-coded bogus oemid - should they choose to.
I guess Sandisk would need to do that as well.

Thanks,
Avri

> 
>  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
>

Dominique Martinet Nov. 1, 2023, 4:37 a.m. UTC | #2

Avri Altman wrote on Thu, Oct 26, 2023 at 10:16:53AM +0000:
> Reviewed-by: Avri Altman <[e-mail]>

Thanks for the review!

> > ---
> > 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.
> To me, your fix is clean, elegant and does the job.
> I would let the quirk owners to fix that hard-coded bogus oemid - should they choose to.
> I guess Sandisk would need to do that as well.

Yes, this was exactly my intention - leave the workaround in place for a
while while owners fix their quirks then eventually fix types and remove
this when it is no longer needed.

Meanwhile, all stable kernels including the newly released 6.6 have many
broken quirks and at the very least the MMC I have here would
periodically hang when issuing a flush, so as a selfish user I'd
appreciate if this (or something equivalent) could be making its way
towards Linus' tree.
Ulf, would you have a bit of time to move this forward, or should I ask
Greg to temporarily revert Avri's "mmc: core: Capture correct oemid-bits
for eMMC cards" commit in stable trees until the way forward is decided?

Thanks!

Ulf Hansson Nov. 2, 2023, 1:25 p.m. UTC | #3

On Thu, 26 Oct 2023 at 09:52, Dominique Martinet
<[e-mail]> 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/[e-mail]
> Signed-off-by: Dominique Martinet <[e-mail]>
> Cc: [e-mail]
> Cc: Avri Altman <[e-mail]>
> Cc: Ulf Hansson <[e-mail]>
> Cc: Alex Fetters <[e-mail]>
> ---
> 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
>
>

Avri Altman Nov. 2, 2023, 1:37 p.m. UTC | #4

> On Thu, 26 Oct 2023 at 09:52, Dominique Martinet
> <[e-mail]> 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/[e-mail]
> > Signed-off-by: Dominique Martinet
> > <[e-mail]>
> > Cc: [e-mail]
> > Cc: Avri Altman <[e-mail]>
> > Cc: Ulf Hansson <[e-mail]>
> > Cc: Alex Fetters <[e-mail]>
> > ---
> > 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
> >
> >

Dominique Martinet Nov. 3, 2023, 12:33 a.m. UTC | #5

Ulf Hansson wrote on Thu, Nov 02, 2023 at 02:25:09PM +0100:
> > Fixes: 84ee19bffc93 ("mmc: core: Capture correct oemid-bits for eMMC cards")
> 
> [...]
> 
> It looks to me that the offending commit (84ee19bffc93) should be
> reverted instead of trying to introduce some weird parsing of the card
> quirks.

I agree that's better -- that's what I did on our stable tree until the
dust settles down, I probably should have sent that instead.

As Avri pointed out the offending commit was picked up to stable, but
the revert should apply cleanly so if we send Greg a mail after Linus
picked it up it can be reverted on all stable branches quickly.

There's little value in me resending this as a revert, but process-wise
I guess it's easier if someone sends it as a mail so I'll whip up a
commit message and send that now.

> 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?

Right, it's possible that using 8 bits here would apply the quirks to
more devices than what was intended if the other 8 bits made a
difference... Unfortunately that's something only vendors would know.

> 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.

Keep filling the full 16 bits unless rev is higher, in which case we
read half?
At this point (mmc_decode_cid) we can use card's ext_csd.rev so if v5.1A
bumped it then it's a possibility; I don't have access to the jedec
standard to check right now.

Patch

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,