linux-kernel - Re: [PATCH v2 4/6] mmc: core: add undervoltage handler for MMC/eMMC devices

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: <4bdc8cef-60cc-466e-986e-46cf23d96556@arm.com>
Date: Thu, 20 Feb 2025 11:22:45 +0000
From: Christian Loehle <christian.loehle@....com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Ulf Hansson <ulf.hansson@...aro.org>, kernel@...gutronix.de,
 linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Mark Brown <broonie@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Søren Andersen <san@...v.dk>
Subject: Re: [PATCH v2 4/6] mmc: core: add undervoltage handler for MMC/eMMC
 devices

On 2/20/25 10:56, Oleksij Rempel wrote:
> On Thu, Feb 20, 2025 at 10:47:01AM +0000, Christian Loehle wrote:
>> On 2/20/25 07:44, Oleksij Rempel wrote:
>>> Introduce _mmc_handle_undervoltage() to handle undervoltage events for
>>> MMC/eMMC devices. This function interrupts ongoing operations using HPI
>>> and performs a controlled suspend. After completing the sequence, the card
>>> is marked as removed to prevent further interactions.
>>>
>>> To support this, introduce __mmc_suspend() and __mmc_resume() as internal
>>> helpers that omit mmc_claim_host()/mmc_release_host(), allowing them to be
>>> called when the host is already claimed. Update mmc_shutdown() to skip the
>>> normal shutdown sequence if the host is flagged as undervoltage to avoid
>>> repeating of the shutdown procedure.
>>
>> "of" can be removed here.
>>
>> Given that this introduces large parts of the mmc handling IMO this commit
>> deserves a lot more explanation of what steps exactly do for which cards
>> and why.
> 
> ack
> 
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
>>> ---
>>>  drivers/mmc/core/mmc.c | 81 +++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 68 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 86b608843232..e626213e7a52 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -2104,8 +2104,8 @@ static int _mmc_flush_cache(struct mmc_host *host)
>>>  	return err;
>>>  }
>>>  
>>> -static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
>>> -			bool is_undervoltage)
>>> +static int __mmc_suspend(struct mmc_host *host, bool is_suspend,
>>> +			 bool is_undervoltage)
>>
>> The is_undervoltage doesn't do anything? Did you forget something here?
> 
> This was done in the previous patch "mmc: core: refactor _mmc_suspend()
> for undervoltage handling"

Sorry!

> 
>>> @@ -2189,6 +2205,9 @@ static int mmc_shutdown(struct mmc_host *host)
>>>  {
>>>  	int err = 0;
>>>  
>>> +	if (host->undervoltage)
>>> +		return 0;
>>> +
>>
>> Probably deserves a comment.
> 
> ack
> 
>>>  	/*
>>>  	 * In a specific case for poweroff notify, we need to resume the card
>>>  	 * before we can shutdown it properly.
>>> @@ -2280,6 +2299,41 @@ static int _mmc_hw_reset(struct mmc_host *host)
>>>  	return mmc_init_card(host, card->ocr, card);
>>>  }
>>>  
>>> +static int _mmc_handle_undervoltage(struct mmc_host *host)
>>> +{
>>> +	struct mmc_card *card = host->card;
>>> +	int err = 0;
>>> +
>>> +	mmc_claim_host(host);
>>> +
>>> +	if (!host->card)
>>> +		goto out;
>>> +
>>> +	if (mmc_can_poweroff_notify(host->card) &&
>>> +		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
>>> +		err = __mmc_resume(host);
>>
>> I'm not sure I follow this.
>> Why would we power-up a card that currently doesn't have power when we
>> know we are about to powerfail it?
> 
> It is part of the mmc_shutdown, but it is not used on my HW. So, can be
> skip it.
> 
>>> +
>>> +	if (!err) {
>>> +		err = mmc_interrupt_hpi(card);
>>> +		if (err)
>>> +			pr_err("%s: Interrupt HPI failed, error %d\n",
>>> +				mmc_hostname(host), err);
>>
>> There's no point in calling this for SD but I don't see why it currently
>> wouldn't be called for SD.
> 
> I tried to keep budget low, until we agree that it is the way to go.
> After this patch stack is accepted, i can try to request more time to
> add and test the SD handler.
If you're not implementing this for now, why not just drop the undervoltage
event at patch 1/6 if host doesn't have an eMMC attached?