linux-kernel - Re: [PATCH V1] 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: <48df0005-34d1-4bac-9517-16dc6018aa85@intel.com>
Date: Mon, 11 Nov 2024 10:11:27 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Sarthak Garg <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, 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_sachgupt@...cinc.com,
 quic_bhaskarv@...cinc.com, quic_narepall@...cinc.com, kernel@...cinc.com
Subject: Re: [PATCH V1] mmc: sdhci-msm: Ensure SD card power isn't ON when
 card removed

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.

>  
>  	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)) {
		...


> +		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)) {
		...


> +		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;