linux-kernel - RE: [EXT] Re: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer completion before reset

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <DU2PR04MB8567DB3B818280EED81A36B2EDA0A@DU2PR04MB8567.eurprd04.prod.outlook.com>
Date: Wed, 10 Dec 2025 11:18:06 +0000
From: Luke Wang <ziniu.wang_1@....com>
To: Ulf Hansson <ulf.hansson@...aro.org>, Bough Chen <haibo.chen@....com>
CC: "adrian.hunter@...el.com" <adrian.hunter@...el.com>, "shawnguo@...nel.org"
	<shawnguo@...nel.org>, "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>, "festevam@...il.com"
	<festevam@...il.com>, "linux-mmc@...r.kernel.org"
	<linux-mmc@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	dl-S32 <S32@....com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer
 completion before reset



> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@...aro.org>
> Sent: Tuesday, December 9, 2025 9:48 PM
> To: Bough Chen <haibo.chen@....com>; Luke Wang
> <ziniu.wang_1@....com>
> Cc: adrian.hunter@...el.com; shawnguo@...nel.org;
> s.hauer@...gutronix.de; kernel@...gutronix.de; festevam@...il.com;
> linux-mmc@...r.kernel.org; imx@...ts.linux.dev; dl-S32 <S32@....com>;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: [EXT] Re: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer
> completion before reset
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On Fri, 5 Dec 2025 at 09:12, Bough Chen <haibo.chen@....com> wrote:
> >
> > > -----Original Message-----
> > > From: Luke Wang <ziniu.wang_1@....com>
> > > Sent: 2025年12月5日 15:17
> > > To: adrian.hunter@...el.com; Bough Chen <haibo.chen@....com>;
> > > ulf.hansson@...aro.org
> > > Cc: shawnguo@...nel.org; s.hauer@...gutronix.de;
> kernel@...gutronix.de;
> > > festevam@...il.com; linux-mmc@...r.kernel.org; imx@...ts.linux.dev;
> dl-S32
> > > <S32@....com>; linux-arm-kernel@...ts.infradead.org;
> > > linux-kernel@...r.kernel.org
> > > Subject: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer
> completion
> > > before reset
> > >
> > > From: Luke Wang <ziniu.wang_1@....com>
> > >
> > > On IMX7ULP platforms, certain SD cards (e.g. Kingston Canvas Go! Plus)
> cause
> > > system hangs and reboots during manual tuning. These cards exhibit large
> gaps
> > > (~16us) between tuning command response and data transmission.
> > > When cmd CRC errors occur during tuning, the code assumes data errors
> even
> > > tuning data hasn't been fully received and then reset host data circuit.
> > >
> > > Per IMX7ULP reference manual, reset operations (RESET_DATA/ALL) need
> to
> > > make sure no active data transfers. Previously, resetting while data was in-
> flight
> > > would clear data circuit, including ADMA/SDMA address, causing data to
> be
> > > transmitted to incorrect memory address. This patch adds polling for data
> > > transfer completion before executing resets.
> > >
> > > Signed-off-by: Luke Wang <ziniu.wang_1@....com>
> > > ---
> > > v2: amend commit message
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index a7a5df673b0f..affde1936510 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -1453,6 +1453,21 @@ static void esdhc_set_uhs_signaling(struct
> > > sdhci_host *host, unsigned timing)
> > >
> > >  static void esdhc_reset(struct sdhci_host *host, u8 mask)  {
> > > +     u32 present_state;
> > > +     int ret;
> > > +
> > > +     /*
> > > +      * For data or full reset, ensure any active data transfer completes
> > > +      * before resetting to avoid system hang.
> > > +      */
> > > +     if (mask & (SDHCI_RESET_DATA | SDHCI_RESET_ALL)) {
> > > +             ret = readl_poll_timeout_atomic(host->ioaddr + ESDHC_PRSSTAT,
> > > present_state,
> >
> > I'm okay with the patch, but I find one thing here:
> >
> > I notice you use _atomic here, I guess you want to cover the case when the
> reset function is called in hardware irq handler sdhci_irq().
> > I'm not sure whether it is suitable to add delay in hardware iqr handler, I
> find there is also udelay in sdhci_reset(), sdhci_reset can also be called in
> hardware irq handler when there is cmd_crc/data_crc error.
> > Adrian/Ulf, do you notice this issue before? Any comments?
> 
> You are right. In general it's certainly not preferred to poll/delay
> in an atomic context. But if I understand correctly here, polling in
> atomic context should not happen that frequently, right?
> 
> Moreover, I wonder if we really need to have a polling-timeout of
> 100ms? Where did that value come from?

I referred to the 100ms timeout setting of sdhci_reset. 

Actual measurements show that during tuning, waiting for data transfer
completion typically only requires tens of microseconds. However, considering
that the tuning block size is very small, when transferring larger blocks, it
may require a longer waiting time. 

The probability of encountering CRC errors that require a reset when not in tuning
is quite low, so waiting for some additional time may not cause significant impact.

> 
> In any case, please add a define to specify the timeout and make sure
> the define-name has the suffix corresponding to the unit (_US,
> _MS,...)
> 

Sure, will do in next version

Thanks,
Luke Wang

> >
> > Regards
> > Haibo Chen
> >
> >
> > > +                                             !(present_state & SDHCI_DATA_INHIBIT), 2,
> > > 100000);
> > > +             if (ret == -ETIMEDOUT)
> > > +                     dev_warn(mmc_dev(host->mmc),
> > > +                              "timeout waiting for data transfer completion\n");
> > > +     }
> > > +
> > >       sdhci_and_cqhci_reset(host, mask);
> > >
> > >       sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> > > --
> > > 2.34.1
> >
> 
> Kind regards
> Uffe