linux-kernel - Re: [PATCH V2] mmc: sdhci-msm: Ensure SD card power isn't ON when card removed

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: <61cffc10-3e05-4331-b967-6c01cc03d072@oss.qualcomm.com>
Date: Sat, 21 Jun 2025 12:59:07 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Sarthak Garg <quic_sartgarg@...cinc.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>
Cc: linux-mmc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_cang@...cinc.com,
        quic_nguyenb@...cinc.com, quic_rampraka@...cinc.com,
        quic_pragalla@...cinc.com, quic_sayalil@...cinc.com,
        quic_nitirawa@...cinc.com, quic_bhaskarv@...cinc.com,
        kernel@....qualcomm.com
Subject: Re: [PATCH V2] mmc: sdhci-msm: Ensure SD card power isn't ON when
 card removed

On 6/20/25 11:03 AM, 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.

This is very difficult to parse. How about:

Many mobile phones feature multi-card tray designs, where the same
tray is used for both SD and SIM cards. If the SD card is placed
at the outermost location in the tray, the SIM card may come in
contact with SD card power-supply while removing the tray, possibly
resulting in SIM damage.

To prevent that, make sure the SD card is really inserted by reading
the Card Detect pin state. If it's not, turn off the power in
sdhci_msm_check_power_status() and set the BUS_FAIL power state on the
controller as part of pwr_irq handling.


(Now I don't know if this is a good fix as far as logic goes, but I'm
simply looking at the patch)

> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@...cinc.com>
> ---
> Changes from v1:
> As per Adrian Hunter's comment :
> - Removed unrelated changes
> - Created a separate function get_cd for cleaner code
> - Used READ_ONCE when getting mmc->ops to handle card removal cases
> - Reordered if check conditions
> ---
>  drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index bf91cb96a0ea..97a895d839c9 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1566,6 +1566,14 @@ static inline void sdhci_msm_complete_pwr_irq_wait(
>  	wake_up(&msm_host->pwr_irq_wait);
>  }
>  
> +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);

What do you need the READ_ONCE for?> +
> +	return mmc_ops && mmc->ops->get_cd ? mmc->ops->get_cd(mmc) : 0;

I think this op will always exist for our driver, since we call:

sdhci_msm_probe()
 -> sdhci_pltfm_init()
    -> sdhci_alloc_host()

which assigns:

host->mmc_host_ops = sdhci_ops;
mmc->ops = &host->mmc_host_ops;

which contains:

.get_cd         = sdhci_get_cd,

there's some more layers to this matryoshka, so I'm not a 100% sure

> +}
> +
>  /*
>   * sdhci_msm_check_power_status API should be called when registers writes
>   * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
> @@ -1579,6 +1587,7 @@ 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);
> +	struct mmc_host *mmc = host->mmc;
>  	bool done = false;
>  	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
>  	const struct sdhci_msm_offset *msm_offset =
> @@ -1636,6 +1645,12 @@ 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 ((req_type & REQ_BUS_ON) && mmc->card && !get_cd(host)) {
> +		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);
>  }
> @@ -1694,6 +1709,13 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		udelay(10);
>  	}
>  
> +	if ((irq_status & CORE_PWRCTL_BUS_ON) && mmc->card && !get_cd(host)) {
> +		irq_ack = CORE_PWRCTL_BUS_FAIL;
> +		msm_host_writel(msm_host, irq_ack, host,
> +				msm_offset->core_pwrctl_ctl);

Since you're dropping out if this function, you can pass the parameter
directly to msm_host_writel

Konrad