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

[冯锐]

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

> > 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.
> >
> "will not work" mean fail and will not hang interface. I will add "host->eject =
> true" in the end of init_sd_express(), so that set_ios() will do nothing just
> return.
> 
> > Kind regards
> > Uffe
> >
> > ------Please consider the environment before printing this e-mail.