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