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: <32ed84b2-41f6-4c1e-a7c6-f3cbc92ae6b5@quicinc.com>
Date: Tue, 1 Jul 2025 12:22:46 +0530
From: Sarthak Garg <quic_sartgarg@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.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/21/2025 4:29 PM, Konrad Dybcio wrote:
> 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)
> 

Sure will update the commit text.

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

Yes its not needed will remove it.

>> +}
>> +
>>   /*
>>    * 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

Sure will update.

~Best regards
Sarthak