Re: [PATCH] mmc: rpmb: add quirk MMC_QUIRK_BROKEN_RPMB_RETUNE

Re: [PATCH] mmc: rpmb: add quirk MMC_QUIRK_BROKEN_RPMB_RETUNE

[Jorge Ramirez-Ortiz, Foundries]

On 29/11/23 10:32:41, Christian Loehle wrote:
> On 29/11/2023 09:43, Jorge Ramirez-Ortiz wrote:
> > On the eMMC SanDisk iNAND 7250 configured with HS200, requesting a
> > re-tune before switching to the RPMB partition would randomly cause
> > subsequent RPMB requests to fail with EILSEQ:
> > * data error -84, tigggered in __mmc_blk_ioctl_cmd()
> >
> > This commit skips the retune when switching to RPMB.
> > Tested over several days with per minute RPMB reads.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/mmc/core/block.c  | 6 +++++-
> >  drivers/mmc/core/card.h   | 7 +++++++
> >  drivers/mmc/core/quirks.h | 7 +++++++
> >  include/linux/mmc/card.h  | 1 +
> >  4 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 152dfe593c43..9b7ba6562a3b 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -860,6 +860,11 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
> >  				return ret;
> >  		}
> >  		mmc_retune_pause(card->host);
> > +
> > +		/* Do not force retune before RPMB switch */
> > +		if (mmc_can_retune(card->host) &&
> > +		    mmc_card_broken_rpmb_retune(card))
> > +			card->host->need_retune = 0;
> >  	}
>
> Is this just an issue for rpmb switches or did you try other non-rpmb
> partition switches, too?

I only tried the reported failure case (ie, RPMB switch).This failure
case came from a product verification team so I believe all the other
scenarios should be functional - other than RPMB accesses (these tests
started a few months ago), no other issues have been reported from eMMC
validation in the last couple of years.

Is there some additional check/test you would like to see done? if so,
please could you let me know how to trigger them?

> And only HS200 or that's just what you tested with?

Yes that is just the product configuration; so the only one I tested.

The host controller is sdhci-of-arasan for xlnx,zynqmp-8.9a. I inspected
the driver history and auto-tuning was extended to support ZynqMP (DLL
reset) some years ago so I believe tuning itself should be fine.

I am referring to commit 8d2e334377dbe645415fbe031711324bc2281907 "mmc:
sdhci-of-arasan: Add support for DLL reset for ZynqMP platforms "

>
> >
> >  	return ret;
> > @@ -3143,4 +3148,3 @@ module_exit(mmc_blk_exit);
> >
> >  MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("Multimedia Card (MMC) block device driver");
> > -
> > diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> > index b7754a1b8d97..1e1555a15de9 100644
> > --- a/drivers/mmc/core/card.h
> > +++ b/drivers/mmc/core/card.h
> > @@ -85,6 +85,7 @@ struct mmc_fixup {
> >  #define CID_MANFID_MICRON       0x13
> >  #define CID_MANFID_SAMSUNG      0x15
> >  #define CID_MANFID_APACER       0x27

I had to add another ID for SanDisk - maybe 0x45 is for Industrial
controllers? do you know?
> > +#define CID_MANFID_SANDISK2     0x45

> >  #define CID_MANFID_KINGSTON     0x70
> >  #define CID_MANFID_HYNIX	0x90
> >  #define CID_MANFID_KINGSTON_SD	0x9F
> > @@ -284,4 +285,10 @@ static inline int mmc_card_broken_cache_flush(const struct mmc_card *c)
> >  {
> >  	return c->quirks & MMC_QUIRK_BROKEN_CACHE_FLUSH;
> >  }
> > +
> > +static inline int mmc_card_broken_rpmb_retune(const struct mmc_card *c)
> > +{
> > +	return c->quirks & MMC_QUIRK_BROKEN_RPMB_RETUNE;
> > +}
> > +
> >  #endif
> > diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> > index cca71867bc4a..35dfc8437d29 100644
> > --- a/drivers/mmc/core/quirks.h
> > +++ b/drivers/mmc/core/quirks.h
> > @@ -130,6 +130,13 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[] = {
> >  	MMC_FIXUP(CID_NAME_ANY, CID_MANFID_SANDISK_SD, 0x5344, add_quirk_sd,
> >  		  MMC_QUIRK_BROKEN_SD_DISCARD),
> >
> > +	/*
> > +	 * SanDisk iNAND 7250 DDG4064, this quirk shall disable the retune
> > +	 * operation enforced by default when switching to RPMB.
> > +	 */
> > +	MMC_FIXUP("DG4064", CIF_MANFID_SANDISK2, 0x100, add_quirk_mmc,
> > +		  MMC_QUIRK_BROKEN_RPMB_RETUNE),
> > +
> >  	END_FIXUP
> >  };
> >
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 7b12eebc5586..bd6986189e8b 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -296,6 +296,7 @@ struct mmc_card {
> >  #define MMC_QUIRK_BROKEN_SD_DISCARD	(1<<14)	/* Disable broken SD discard support */
> >  #define MMC_QUIRK_BROKEN_SD_CACHE	(1<<15)	/* Disable broken SD cache support */
> >  #define MMC_QUIRK_BROKEN_CACHE_FLUSH	(1<<16)	/* Don't flush cache until the write has occurred */
> > +#define MMC_QUIRK_BROKEN_RPMB_RETUNE	(1<<17) /* Don't force a retune before switching to RPMB */
> >
> >  	bool			written_flag;	/* Indicates eMMC has been written since power on */
> >  	bool			reenable_cmdq;	/* Re-enable Command Queue */
> > --
> > 2.34.1
>

Re: [PATCH] mmc: rpmb: add quirk MMC_QUIRK_BROKEN_RPMB_RETUNE

[Christian Loehle]

On 29/11/2023 14:42, Jorge Ramirez-Ortiz, Foundries wrote:
> On 29/11/23 10:32:41, Christian Loehle wrote:
>> On 29/11/2023 09:43, Jorge Ramirez-Ortiz wrote:
>>> On the eMMC SanDisk iNAND 7250 configured with HS200, requesting a
>>> re-tune before switching to the RPMB partition would randomly cause
>>> subsequent RPMB requests to fail with EILSEQ:
>>> * data error -84, tigggered in __mmc_blk_ioctl_cmd()
>>>
>>> This commit skips the retune when switching to RPMB.
>>> Tested over several days with per minute RPMB reads.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
>>> ---
>>>  drivers/mmc/core/block.c  | 6 +++++-
>>>  drivers/mmc/core/card.h   | 7 +++++++
>>>  drivers/mmc/core/quirks.h | 7 +++++++
>>>  include/linux/mmc/card.h  | 1 +
>>>  4 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>>> index 152dfe593c43..9b7ba6562a3b 100644
>>> --- a/drivers/mmc/core/block.c
>>> +++ b/drivers/mmc/core/block.c
>>> @@ -860,6 +860,11 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
>>>  				return ret;
>>>  		}
>>>  		mmc_retune_pause(card->host);
>>> +
>>> +		/* Do not force retune before RPMB switch */
>>> +		if (mmc_can_retune(card->host) &&
>>> +		    mmc_card_broken_rpmb_retune(card))
>>> +			card->host->need_retune = 0;
>>>  	}
>>
>> Is this just an issue for rpmb switches or did you try other non-rpmb
>> partition switches, too?
> 
> I only tried the reported failure case (ie, RPMB switch).This failure
> case came from a product verification team so I believe all the other
> scenarios should be functional - other than RPMB accesses (these tests
> started a few months ago), no other issues have been reported from eMMC
> validation in the last couple of years.
> 
> Is there some additional check/test you would like to see done? if so,
> please could you let me know how to trigger them?

That's fine with me, probably just a very interesting bug then.

> 
>> And only HS200 or that's just what you tested with?
> 
> Yes that is just the product configuration; so the only one I tested.
> 
> The host controller is sdhci-of-arasan for xlnx,zynqmp-8.9a. I inspected
> the driver history and auto-tuning was extended to support ZynqMP (DLL
> reset) some years ago so I believe tuning itself should be fine.
> 
> I am referring to commit 8d2e334377dbe645415fbe031711324bc2281907 "mmc:
> sdhci-of-arasan: Add support for DLL reset for ZynqMP platforms "
> 
>>
>>>
>>>  	return ret;
>>> @@ -3143,4 +3148,3 @@ module_exit(mmc_blk_exit);
>>>
>>>  MODULE_LICENSE("GPL");
>>>  MODULE_DESCRIPTION("Multimedia Card (MMC) block device driver");
>>> -
>>> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
>>> index b7754a1b8d97..1e1555a15de9 100644
>>> --- a/drivers/mmc/core/card.h
>>> +++ b/drivers/mmc/core/card.h
>>> @@ -85,6 +85,7 @@ struct mmc_fixup {
>>>  #define CID_MANFID_MICRON       0x13
>>>  #define CID_MANFID_SAMSUNG      0x15
>>>  #define CID_MANFID_APACER       0x27
> 
> I had to add another ID for SanDisk - maybe 0x45 is for Industrial
> controllers? do you know?

I don't have access to JEDEC internal docs anymore, but since Avri
probably wants to ACK this potential bug anyway, he should confirm.