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

[Ulf Hansson <ulf.hansson@linaro.org>]

On Mon, 26 Oct 2020 at 09:22, 冯锐 <rui_feng@realsil.com.cn> 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@realsil.com.cn> wrote:
> > >
> > > >
> > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote:
> > > > >
> > > > > From: Rui Feng <rui_feng@realsil.com.cn>
> > > > >
> > > > > 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@realsil.com.cn>
> > > > > ---
> > > > >  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.

According to Christoph, it should be possible to support read-only
mode via PCIe/NVMe. You may need to add some tweaks to support this in
the PCIe controller driver, but I can't advise you how to exactly do
this.

Perhaps you need to read/store the state of SD write-protect pin
before switching to SD express mode, because you may not be able to
read it beyond some point?

>
> >
> > From this, I assume that my interpretations of the behavior was correct.
> >
> > Although, can you please elaborate on what you mean by that it will "not
> > work"?
> >
> > Do you mean that rtsx_pci_card_exclusive_check() that is called early in
> > sdmmc_set_ios() will fail and then make it bail out? Then, could you please add
> > a comment about that in the code?
> >
> In init_sd_express() driver sets 0xFF54 bit0=1 and 0xFF55 bit4=0, then RTS5261 will switch MCU and enter SD EXPRESS mode.
> After that RTS5261 can't receive any CMD from PCIe, so mmc_power_off() will not work.

Thanks for trying to clarify.

However, this still doesn't explain to me, what *exactly* will happen
when rtsx_pci_card_exclusive_check() is called (or any other functions
in ->set_ios()).

In principle, "will not work" could mean that the calls to the
rtsx_pci_* cardreader interface hangs - and that would not be okay (as
it could lead to that the ->remove() callback hangs). So, either you
need to put a well written comment in the code about what will happen
- or add some kind of protection against potential problems for this.

Kind regards
Uffe