| 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:
<DM8PR02MB78787D61019A2056D7843DF2F565A@DM8PR02MB7878.namprd02.prod.outlook.com>
Date: Mon, 26 May 2025 08:26:10 +0000
From: Sarthak Garg <sartgarg@....qualcomm.com>
To: Adrian Hunter <adrian.hunter@...el.com>,
"Sarthak Garg (QUIC)"
<quic_sartgarg@...cinc.com>,
Ulf Hansson <ulf.hansson@...aro.org>
CC: "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Can Guo
(QUIC)" <quic_cang@...cinc.com>,
"Bao D. Nguyen (QUIC)"
<quic_nguyenb@...cinc.com>,
"Ram Prakash Gupta (QUIC)"
<quic_rampraka@...cinc.com>,
"Pradeep Pragallapati (QUIC)"
<quic_pragalla@...cinc.com>,
"Sayali Lokhande (QUIC)"
<quic_sayalil@...cinc.com>,
"Nitin Rawat (QUIC)" <quic_nitirawa@...cinc.com>,
"Sachin Gupta (QUIC)" <quic_sachgupt@...cinc.com>,
"Bhaskar Valaboju (QUIC)"
<quic_bhaskarv@...cinc.com>,
"Naveen Kumar Goud Arepalli (QUIC)"
<quic_narepall@...cinc.com>,
kernel <kernel@...cinc.com>
Subject: RE: [PATCH V1] mmc: sdhci-msm: Ensure SD card power isn't ON when
card removed
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@...el.com>
> Sent: Monday, November 11, 2024 1:41 PM
> To: Sarthak Garg (QUIC) <quic_sartgarg@...cinc.com>; Ulf Hansson
> <ulf.hansson@...aro.org>
> Cc: linux-mmc@...r.kernel.org; linux-arm-msm@...r.kernel.org; linux-
> kernel@...r.kernel.org; Can Guo (QUIC) <quic_cang@...cinc.com>; Bao D.
> Nguyen (QUIC) <quic_nguyenb@...cinc.com>; Ram Prakash Gupta (QUIC)
> <quic_rampraka@...cinc.com>; Pradeep Pragallapati (QUIC)
> <quic_pragalla@...cinc.com>; Sayali Lokhande (QUIC)
> <quic_sayalil@...cinc.com>; Nitin Rawat (QUIC)
> <quic_nitirawa@...cinc.com>; Sachin Gupta (QUIC)
> <quic_sachgupt@...cinc.com>; Bhaskar Valaboju (QUIC)
> <quic_bhaskarv@...cinc.com>; Naveen Kumar Goud Arepalli (QUIC)
> <quic_narepall@...cinc.com>; kernel <kernel@...cinc.com>
> Subject: Re: [PATCH V1] mmc: sdhci-msm: Ensure SD card power isn't ON
> when card removed
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 5/11/24 11:35, Sarthak Garg wrote:
> > Make sure SD card power is not enabled when the card is being removed.
> > On multi-card tray designs, the same card-tray would be used for SD
> > card and SIM cards. If SD card is placed at the outermost location in
> > the tray, then SIM card may come in contact with SD card power- supply
> > while removing the tray. It may result in SIM damage.
> > So in sdhci_msm_handle_pwr_irq we skip the BUS_ON request when the
> SD
> > card is removed to be in consistent with the MGPI hardware fix to
> > prevent any damage to the SIM card in case of mult-card tray designs.
> > But we need to have a similar check in sdhci_msm_check_power_status to
> > be in consistent with the sdhci_msm_handle_pwr_irq function.
> > Also reset host->pwr and POWER_CONTROL register accordingly since we
> > are not turning ON the power actually.
> >
> > Signed-off-by: Sarthak Garg <quic_sartgarg@...cinc.com>
> > ---
> > drivers/mmc/host/sdhci-msm.c | 20 ++++++++++++++++++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-msm.c
> > b/drivers/mmc/host/sdhci-msm.c index e00208535bd1..443526c56194
> 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > @@ -1516,10 +1516,11 @@ static void
> > sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type) {
> > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > - bool done = false;
> > - u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
> > const struct sdhci_msm_offset *msm_offset =
> > msm_host->offset;
> > + struct mmc_host *mmc = host->mmc;
> > + bool done = false;
> > + u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
>
> Please don't make unrelated changes. The above 2 lines have not changed
> and should stay where they are. If you feel the need to make cosmetic
> changes, make a separate patch.
>
Reviving this as back from break.
Sure will take care.
> >
> > pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
> > mmc_hostname(host->mmc), __func__, req_type, @@
> > -1573,6 +1574,13 @@ static void sdhci_msm_check_power_status(struct
> sdhci_host *host, u32 req_type)
> > "%s: pwr_irq for req: (%d) timed out\n",
> > mmc_hostname(host->mmc), req_type);
> > }
> > +
> > + if (mmc->card && mmc->ops && mmc->ops->get_cd &&
> > + !mmc->ops->get_cd(mmc) && (req_type & REQ_BUS_ON)) {
>
> It would be tidier to have a separate fn for calling get_cd() e.g.
>
> static int get_cd(struct sdhci_host *host) {
> struct mmc_host *mmc = host->mmc;
>
> return mmc->card && mmc->ops && mmc->ops->get_cd ? mmc->ops-
> >get_cd(mmc) : 0; }
>
> and put the other check first to avoid calling ->get_cd() for no reason:
>
> if ((req_type & REQ_BUS_ON) && !get_cd(host)) {
> ...
>
>
Sure looks more cleaner.
> > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > + host->pwr = 0;
> > + }
> > +
> > pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
> > __func__, req_type); } @@ -1631,6 +1639,14 @@
> > static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> > udelay(10);
> > }
> >
> > + if (mmc->card && mmc->ops && mmc->ops->get_cd &&
> > + !mmc->ops->get_cd(mmc) && irq_status &
> > + CORE_PWRCTL_BUS_ON) {
>
> If the card is being removed, how do you know mmc->ops won't disappear
> under you? You need READ_ONCE otherwise e.g.
>
> const struct mmc_host_ops *mmc_ops = READ_ONCE(mmc->ops);
>
> so like:
>
> static int get_cd(struct sdhci_host *host) {
> struct mmc_host *mmc = host->mmc;
> const struct mmc_host_ops *mmc_ops = READ_ONCE(mmc->ops);
>
> return mmc->card && mmc_ops && mmc_ops->get_cd ? mmc_ops-
> >get_cd(mmc) : 0; }
>
>
> And again, put the other check first e.g.
>
> if ((irq_status & CORE_PWRCTL_BUS_ON) && !get_cd(host)) {
> ...
>
>
Thanks for pointing this will correct in V2.
> > + irq_ack = CORE_PWRCTL_BUS_FAIL;
> > + msm_host_writel(msm_host, irq_ack, host,
> > + msm_offset->core_pwrctl_ctl);
> > + return;
> > + }
> > +
> > /* Handle BUS ON/OFF*/
> > if (irq_status & CORE_PWRCTL_BUS_ON) {
> > pwr_state = REQ_BUS_ON;
Powered by blists - more mailing lists