答复: [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.

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