| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2cc743fae59d44b68effe8b51152cfe3@realtek.com>
Date: Wed, 28 Jan 2026 03:06:28 +0000
From: Ricky WU <ricky_wu@...ltek.com>
To: Ulf Hansson <ulf.hansson@...aro.org>,
Matthew Schwartz
<matthew.schwartz@...ux.dev>
CC: "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] mmc: rtsx_pci_sdmmc: implement sdmmc_card_busy function
> > >
> > > Hi Matthew,
> > >
> > > Thanks for working on this patch.
> > >
> > > We’ve tested this change on our platforms and can confirm that
> > > adding the
> > > card_busy() callback does resolve the “cannot verify signal voltage
> > > switch” issue for us 👍.
> > >
> > > That said, while reviewing the change we noticed a potential
> > > redundancy in the existing driver logic. In sdmmc_switch_voltage()
> > > we already perform explicit DAT line stabilization checks via
> > > sd_wait_voltage_stable_1() and sd_wait_voltage_stable_2().
> > >
> > > Once card_busy() is implemented and used by the MMC core during the
> > > voltage-switch verification phase, these two stabilization steps
> > > appear to be partially overlapping with what the core now validates
> > > via card_busy(). In our testing, with card_busy() present, the
> > > stable_1 /
> > > stable_2 logic no longer seems strictly necessary and could likely
> > > be simplified or removed with some adjustment.
> > >
> > > From a process point of view, we’re not sure which approach you’d prefer:
> > >
> > > Land your patch as-is first, and then we can follow up with a
> > > separate cleanup/modification patch to adjust
> > > sdmmc_switch_voltage(), or
> > >
> > > We can prepare an additional patch that builds on top of yours and
> > > share it with you for review, so the changes can be aligned together.
> > >
> > > Please let us know which option you think makes more sense for
> > > upstream,
> > or
> > > if you’d prefer a different approach.
> > >
> > > Thanks again for the fix and for looking into this driver.
> > >
> > > Best regards,
> > > Ricky
> > >
> > > > rtsx_pci_sdmmc does not have an sdmmc_card_busy function, so any
> > voltage
> > > > switches cause a kernel warning, "mmc0: cannot verify signal
> > > > voltage
> > switch."
> > > >
> > > > Copy the sdmmc_card_busy function from rtsx_pci_usb to
> > > > rtsx_pci_sdmmc
> > to
> > > > fix this.
> > > >
> > > > Fixes: ff984e57d36e ("mmc: Add realtek pcie sdmmc host driver")
> > > > Signed-off-by: Matthew Schwartz <matthew.schwartz@...ux.dev>
> >
> > I have applied this for fixes and by adding a stable tag. I am also
> > adding Ricky's reviewed/tested-by tag, according to the above.
> >
> > Let's deal with the potential improvement on-top, as agreed.
> >
> > Kind regards
> > Uffe
> >
> >
>
Hi All,
Just a gentle ping on the patch below.
This change is intended as a follow-up cleanup on top of Matthew’s
card_busy() patch, which has already been merged. I wanted to check
whether this additional adjustment to the voltage-switch path looks OK,
or if there are any comments or concerns from the maintainers.
We’re happy to rework or rebase the patch as needed.
> Hi Matthew Ulf,
>
> based on card_busy() patch, we’ve prepared a small follow-up change that
> simplifies the voltage-switch handling in sdmmc_switch_voltage().
>
> sd_wait_voltage_stable_1() and sd_wait_voltage_stable_2() become largely
> redundant.
>
> In our testing, removing these two helpers and keeping only the minimal
> clock-control handling (forcing the SD clock to stop before switching and
> cleaning up on error) works correctly and still conforms to the expected SD
> voltage switch behavior. The core-level checks via card_busy() now cover the
> DAT line verification that was previously duplicated in the driver.
>
> The attached diff removes sd_wait_voltage_stable_1() and
> sd_wait_voltage_stable_2(), and simplifies sdmmc_switch_voltage()
> accordingly.
>
> Ricky
>
> ---
> drivers/mmc/host/rtsx_pci_sdmmc.c | 88 +++----------------------------
> 1 file changed, 6 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 4db3328f46df..8dfbc62f165b 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -1181,79 +1181,6 @@ static int sdmmc_get_cd(struct mmc_host *mmc)
> return cd;
> }
>
> -static int sd_wait_voltage_stable_1(struct realtek_pci_sdmmc *host) -{
> - struct rtsx_pcr *pcr = host->pcr;
> - int err;
> - u8 stat;
> -
> - /* Reference to Signal Voltage Switch Sequence in SD spec.
> - * Wait for a period of time so that the card can drive SD_CMD and
> - * SD_DAT[3:0] to low after sending back CMD11 response.
> - */
> - mdelay(1);
> -
> - /* SD_CMD, SD_DAT[3:0] should be driven to low by card;
> - * If either one of SD_CMD,SD_DAT[3:0] is not low,
> - * abort the voltage switch sequence;
> - */
> - err = rtsx_pci_read_register(pcr, SD_BUS_STAT, &stat);
> - if (err < 0)
> - return err;
> -
> - if (stat & (SD_CMD_STATUS | SD_DAT3_STATUS | SD_DAT2_STATUS |
> - SD_DAT1_STATUS | SD_DAT0_STATUS))
> - return -EINVAL;
> -
> - /* Stop toggle SD clock */
> - err = rtsx_pci_write_register(pcr, SD_BUS_STAT,
> - 0xFF, SD_CLK_FORCE_STOP);
> - if (err < 0)
> - return err;
> -
> - return 0;
> -}
> -
> -static int sd_wait_voltage_stable_2(struct realtek_pci_sdmmc *host) -{
> - struct rtsx_pcr *pcr = host->pcr;
> - int err;
> - u8 stat, mask, val;
> -
> - /* Wait 1.8V output of voltage regulator in card stable */
> - msleep(50);
> -
> - /* Toggle SD clock again */
> - err = rtsx_pci_write_register(pcr, SD_BUS_STAT, 0xFF,
> SD_CLK_TOGGLE_EN);
> - if (err < 0)
> - return err;
> -
> - /* Wait for a period of time so that the card can drive
> - * SD_DAT[3:0] to high at 1.8V
> - */
> - msleep(20);
> -
> - /* SD_CMD, SD_DAT[3:0] should be pulled high by host */
> - err = rtsx_pci_read_register(pcr, SD_BUS_STAT, &stat);
> - if (err < 0)
> - return err;
> -
> - mask = SD_CMD_STATUS | SD_DAT3_STATUS | SD_DAT2_STATUS |
> - SD_DAT1_STATUS | SD_DAT0_STATUS;
> - val = SD_CMD_STATUS | SD_DAT3_STATUS | SD_DAT2_STATUS |
> - SD_DAT1_STATUS | SD_DAT0_STATUS;
> - if ((stat & mask) != val) {
> - dev_dbg(sdmmc_dev(host),
> - "%s: SD_BUS_STAT = 0x%x\n", __func__, stat);
> - rtsx_pci_write_register(pcr, SD_BUS_STAT,
> - SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0);
> - rtsx_pci_write_register(pcr, CARD_CLK_EN, 0xFF, 0);
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios
> *ios) {
> struct realtek_pci_sdmmc *host = mmc_priv(mmc); @@ -1281,7 +1208,9
> @@ static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios
> *ios)
> voltage = OUTPUT_1V8;
>
> if (voltage == OUTPUT_1V8) {
> - err = sd_wait_voltage_stable_1(host);
> + /* Stop toggle SD clock */
> + err = rtsx_pci_write_register(pcr, SD_BUS_STAT,
> + 0xFF, SD_CLK_FORCE_STOP);
> if (err < 0)
> goto out;
> }
> @@ -1290,16 +1219,11 @@ static int sdmmc_switch_voltage(struct
> mmc_host *mmc, struct mmc_ios *ios)
> if (err < 0)
> goto out;
>
> - if (voltage == OUTPUT_1V8) {
> - err = sd_wait_voltage_stable_2(host);
> - if (err < 0)
> - goto out;
> - }
> -
> out:
> /* Stop toggle SD clock in idle */
> - err = rtsx_pci_write_register(pcr, SD_BUS_STAT,
> - SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0);
> + if (err < 0)
> + rtsx_pci_write_register(pcr, SD_BUS_STAT,
> + SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0);
>
> mutex_unlock(&pcr->pcr_mutex);
>
> --
> 2.34.1
>
> > > > ---
> > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 41
> > > > +++++++++++++++++++++++++++++++
> > > > 1 file changed, 41 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > index dc2587ff8519..4db3328f46df 100644
> > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > @@ -1306,6 +1306,46 @@ static int sdmmc_switch_voltage(struct
> > mmc_host
> > > > *mmc, struct mmc_ios *ios)
> > > > return err;
> > > > }
> > > >
> > > > +static int sdmmc_card_busy(struct mmc_host *mmc) {
> > > > + struct realtek_pci_sdmmc *host = mmc_priv(mmc);
> > > > + struct rtsx_pcr *pcr = host->pcr;
> > > > + int err;
> > > > + u8 stat;
> > > > + u8 mask = SD_DAT3_STATUS | SD_DAT2_STATUS |
> > SD_DAT1_STATUS
> > > > + | SD_DAT0_STATUS;
> > > > +
> > > > + mutex_lock(&pcr->pcr_mutex);
> > > > +
> > > > + rtsx_pci_start_run(pcr);
> > > > +
> > > > + err = rtsx_pci_write_register(pcr, SD_BUS_STAT,
> > > > + SD_CLK_TOGGLE_EN |
> > > > SD_CLK_FORCE_STOP,
> > > > + SD_CLK_TOGGLE_EN);
> > > > + if (err)
> > > > + goto out;
> > > > +
> > > > + mdelay(1);
> > > > +
> > > > + err = rtsx_pci_read_register(pcr, SD_BUS_STAT, &stat);
> > > > + if (err)
> > > > + goto out;
> > > > +
> > > > + err = rtsx_pci_write_register(pcr, SD_BUS_STAT,
> > > > + SD_CLK_TOGGLE_EN |
> > > > +SD_CLK_FORCE_STOP, 0);
> > > > +out:
> > > > + mutex_unlock(&pcr->pcr_mutex);
> > > > +
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + /* check if any pin between dat[0:3] is low */
> > > > + if ((stat & mask) != mask)
> > > > + return 1;
> > > > + else
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > {
> > > > struct realtek_pci_sdmmc *host = mmc_priv(mmc); @@
> -1418,6
> > > > +1458,7 @@ static const struct mmc_host_ops realtek_pci_sdmmc_ops
> > > > += {
> > > > .get_ro = sdmmc_get_ro,
> > > > .get_cd = sdmmc_get_cd,
> > > > .start_signal_voltage_switch = sdmmc_switch_voltage,
> > > > + .card_busy = sdmmc_card_busy,
> > > > .execute_tuning = sdmmc_execute_tuning,
> > > > .init_sd_express = sdmmc_init_sd_express, };
> > > > --
> > > > 2.52.0
> > >
Powered by blists - more mailing lists