Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1600999061-13669-1-git-send-email-rui_feng@realsil.com.cn>
 <CAPDyKFrnkF3mU5PJsy0VtEjPSToktSsRRtyMvQF97vymc+rY5A@mail.gmail.com>
 <dd210290eef6467cbffca8cbaddb8b84@realsil.com.cn> <CAPDyKFqwsJaYrXMVabR7qui6yqr4FAHfYq1ghfsf0HtRSZpGGw@mail.gmail.com>
 <ba3c68fea4614434838a0a8cbc0e892a@realsil.com.cn>
In-Reply-To: <ba3c68fea4614434838a0a8cbc0e892a@realsil.com.cn>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Tue, 27 Oct 2020 13:54:46 +0100
Message-ID: <CAPDyKFrDLJtDkkWsSENLDu2xLqptkjDk94YxYfkfW7UPBoG+bg@mail.gmail.com>
Subject: Re: [PATCH 3/3] mmc: rtsx: Add SD Express mode support for RTS5261
To:     =?UTF-8?B?5Yav6ZSQ?= <rui_feng@realsil.com.cn>
Cc:     Christoph Hellwig <hch@lst.de>, Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 26 Oct 2020 at 09:22, =E5=86=AF=E9=94=90 <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, =E5=86=AF=E9=94=90 <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 =3D host->pcr;
> > > > > +       struct mmc_host *mmc =3D host->mmc;
> > > > >         int err;
> > > > > +       u32 val;
> > > > >
> > > > >         if (host->power_state =3D=3D 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) =3D=3D PID_5261) {
> > > > > +               val =3D rtsx_pci_readl(pcr, RTSX_BIPR);
> > > > > +               if (val & SD_WRITE_PROTECT) {
> > > > > +                       pcr->extra_caps &=3D
> > > > ~EXTRA_CAPS_SD_EXPRESS;
> > > > > +                       mmc->caps2 &=3D ~(MMC_CAP2_SD_EXP |
> > > > > + MMC_CAP2_SD_EXP_1_2V);
> > > >
> > > > This looks a bit weird to me. For a write protected card you want t=
o
> > > > disable the SD_EXPRESS support, right?
> > > >
> > > Right. If end user insert a write protected SD express card, I will d=
isable
> > 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 us=
er
> > expects either.
> >
> > Note that the physical write protect switch/pin isn't mandatory to supp=
ort 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 supporte=
d 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 sto=
rage
> > 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 Realt=
ek'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 shou=
ld strive
> > towards a consistent behavior.
> >
> > At this point I tend to prefer to default to ignore the write protect s=
witch for SD
> > express, unless we can find a way to properly support it.
> >
> For information security purpose, some companies or business users set th=
eir notebook SD as "read only".
> Because a lot of "read only" requirements from those companies or busines=
s 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 swit=
ch 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 "no=
t
> > work"?
> >
> > Do you mean that rtsx_pci_card_exclusive_check() that is called early i=
n
> > sdmmc_set_ios() will fail and then make it bail out? Then, could you pl=
ease add
> > a comment about that in the code?
> >
> In init_sd_express() driver sets 0xFF54 bit0=3D1 and 0xFF55 bit4=3D0, the=
n RTS5261 will switch MCU and enter SD EXPRESS mode.
> After that RTS5261 can't receive any CMD from PCIe, so mmc_power_off() wi=
ll 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