Re: [PATCH 3/3] mmc: rtsx: Add SD Express mode support for RTS5261

[Ulf Hansson]

On Wed, 28 Oct 2020 at 11:05, 冯锐 <rui_feng@xxxxxxxxxxxxxx> wrote:
>
> >
> > >
> > > On Mon, 26 Oct 2020 at 09:22, 冯锐 <rui_feng@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > >
> > > > > + Christoph (to help us understand if PCIe/NVMe devices can be
> > > > > + marked
> > > > > + read-only)
> > > > >
> > > > > On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@xxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > >
> > > > > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@xxxxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > From: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> > > > > > > >
> > > > > > > > RTS5261 support legacy SD mode and SD Express mode.
> > > > > > > > In SD7.x, SD association introduce SD Express as a new mode.
> > > > > > > > This patch makes RTS5261 support SD Express mode.
> > > > > > >
> > > > > > > As per patch 2, can you please add some more information about
> > > > > > > what changes are needed to support SD Express? This just
> > > > > > > states that the support is implemented, but please elaborate how.
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > >  drivers/mmc/host/rtsx_pci_sdmmc.c | 59
> > > > > > > > +++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 59 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > > > index 2763a376b054..efde374a4a5e 100644
> > > > > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct
> > > > > > > > realtek_pci_sdmmc *host,  static int sd_power_on(struct
> > > > > > > > realtek_pci_sdmmc *host)  {
> > > > > > > >         struct rtsx_pcr *pcr = host->pcr;
> > > > > > > > +       struct mmc_host *mmc = host->mmc;
> > > > > > > >         int err;
> > > > > > > > +       u32 val;
> > > > > > > >
> > > > > > > >         if (host->power_state == SDMMC_POWER_ON)
> > > > > > > >                 return 0;
> > > > > > > > @@ -922,6 +924,14 @@ static int sd_power_on(struct
> > > > > > > > realtek_pci_sdmmc
> > > > > > > *host)
> > > > > > > >         if (err < 0)
> > > > > > > >                 return err;
> > > > > > > >
> > > > > > > > +       if (PCI_PID(pcr) == PID_5261) {
> > > > > > > > +               val = rtsx_pci_readl(pcr, RTSX_BIPR);
> > > > > > > > +               if (val & SD_WRITE_PROTECT) {
> > > > > > > > +                       pcr->extra_caps &=
> > > > > > > ~EXTRA_CAPS_SD_EXPRESS;
> > > > > > > > +                       mmc->caps2 &=
> > ~(MMC_CAP2_SD_EXP
> > > |
> > > > > > > > + MMC_CAP2_SD_EXP_1_2V);
> > > > > > >
> > > > > > > This looks a bit weird to me. For a write protected card you
> > > > > > > want to disable the SD_EXPRESS support, right?
> > > > > > >
> > > > > > Right. If end user insert a write protected SD express card, I
> > > > > > will disable
> > > > > SD_EXPRESS support.
> > > > > > If host switch to SD EXPRESS mode, the card will be recognized
> > > > > > as a writable PCIe/NVMe device, I think this is not end user's purpose.
> > > > >
> > > > > Hmm.
> > > > >
> > > > > Falling back to use the legacy SD interface is probably not what
> > > > > the user expects either.
> > > > >
> > > > > Note that the physical write protect switch/pin isn't mandatory to
> > > > > support and it doesn't even exist for all formats of SD cards. In
> > > > > the mmc core, we are defaulting to make the card write enabled, if
> > > > > the switch isn't supported by the host driver. Additionally,
> > > > > nothing prevents the end user from mounting the filesystem in
> > > > > read-only mode, if
> > > that is preferred.
> > > > >
> > > > > >
> > > > > > > Is there no mechanism to support read-only PCIe/NVMe based
> > > > > > > storage
> > > > > devices?
> > > > > > > If that is the case, maybe it's simply better to not support
> > > > > > > the readonly option at all for SD express cards?
> > > > > > >
> > > > > > I think there's no mechanism to support read-only PCIe/NVMe
> > > > > > based storage
> > > > > devices.
> > > > >
> > > > > I have looped in Christoph, maybe he can give us his opinion on this.
> > > > >
> > > > > > But different venders may have different opinions. This is only
> > > > > > Realtek's
> > > > > opinion.
> > > > >
> > > > > I understand. However, the most important point for me, is that we
> > > > > don't end up in a situation where each mmc host handles this
> > > > > differently. We should strive towards a consistent behavior.
> > > > >
> > > > > At this point I tend to prefer to default to ignore the write
> > > > > protect switch for SD express, unless we can find a way to
> > > > > properly support
> > > it.
> > > > >
> > > > For information security purpose, some companies or business users
> > > > set their
> > > notebook SD as "read only".
> > > > Because a lot of "read only" requirements from those companies or
> > > > business
> > > users, notebook vendor controls reader write protect pin to achieve it.
> > > > Notebook BIOS might have option to choose "read only" or not.
> > > > This is why we think write protect is more important than speed.
> > >
> > > I understand that it may be used, in some way or the other to provide
> > > a hint to the operating system to mount it in read-only mode.
> > >
> > > Although, if there were a real security feature involved, the internal
> > > FW of the SD card would also monitor the switch, to support read-only
> > > mode. As I understand it, that's not the common case.
> > >
> > > > If you prefer to consistent behavior, I can ignore the write protect
> > > > switch for
> > > SD express.
> > >
> > > At this point, I prefer if you would ignore the write protect switch
> > > in the SD controller driver.
> > >
> > I will ignore write protect switch in V3.
> >
> Sorry I ignore the HW design.
> The reader has two mechanism for mode selection (SD Legacy or SD Express). One is SW (MMC driver) and another is HW.
> We use HW mechanism when system exit S3 or S4.
> HW mechanism selects mode when chip is power on.
> Here is an example for HW mechanism.
> 1. Reader in SD Legacy mode ->
> 2. SD Express card insert ->
> 3. MMC driver selects the SD Express mode ->
> 4. SD Express initial and use NVMe driver and NVMe disk mount ->
> 5. system goes to S4 ->
> 6. system exits S4 ->
> 7. HW selects SD Express mode ->
> 8. SD Express still uses NVMe driver and disk keeps the same
> Therefore, after S4, disk is still keep the same.
>
> Because of HW mechanism selects SD legacy mode when write protect.
> If driver can't select SD legacy mode when write protect, disk might unmount and than mount after S3/S4.
> Here is an example for write protect.
> 1. Reader in SD Legacy mode ->
> 2. SD Express card insert with write protect ->
> 3. MMC driver selects the SD Express mode ->
> 4. SD Express initial and use NVMe driver and NVMe disk mount ->
> 5. system goes to S4 ->
> 6. system exits S4 ->
> 7. Because write protect, HW selects SD legacy mode ->
> 8. linux detect HW change, use MMC driver and NVMe disk unmount ->
> 9. MMC driver selects the SD Express mode ->
> 10. SD Express initial and use NVMe driver and NVMe disk mount
>
> If driver can select SD legacy mode when write protect, disk can keep the same after S3/S4.
> Here is an example for write protect.
> 1. Reader in SD Legacy mode ->
> 2. SD Express card insert with write protect ->
> 3. MMC driver selects the SD legacy mode and disk mount ->
> 5. system goes to S4 ->
> 6. system exits S4 ->
> 7. Because write protect, HW selects SD legacy mode ->
> 8. MMC driver selects the SD legacy mode and disk keeps the same.
> If I ignore the write protect switch in mmc host driver, behavior of SW will not be consistent with HW.

Alright, let's keep the code monitoring the write protect switch then.
However, please add a comment in the code that it's needed because the
HW reads it when resuming from S3/S4 (and then picks SD legacy
interface if it's set in read-only mode).

[...]

Kind regards
Uffe