| Message ID | 20250220074429.2906141-5-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: handle undervoltage events and prevent eMMC corruption | expand |
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. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.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? > { > unsigned int notify_type; > int err = 0; > @@ -2115,8 +2115,6 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend, > else > notify_type = EXT_CSD_POWER_OFF_LONG; > > - mmc_claim_host(host); > - > if (mmc_card_suspended(host->card)) > goto out; > > @@ -2140,7 +2138,18 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend, > mmc_card_set_suspended(host->card); > } > out: > + return err; > +} > + > +static int _mmc_suspend(struct mmc_host *host, bool is_suspend, > + bool is_undervoltage) > +{ > + int err; > + > + mmc_claim_host(host); > + err = __mmc_suspend(host, is_suspend, is_undervoltage); > mmc_release_host(host); > + > return err; > } > > @@ -2160,6 +2169,20 @@ static int mmc_suspend(struct mmc_host *host) > return err; > } > > +static int __mmc_resume(struct mmc_host *host) > +{ > + int err; > + > + if (!mmc_card_suspended(host->card)) > + return 0; > + > + mmc_power_up(host, host->card->ocr); > + err = mmc_init_card(host, host->card->ocr, host->card); > + mmc_card_clr_suspended(host->card); > + > + return err; > +} > + > /* > * This function tries to determine if the same card is still present > * and, if so, restore all state to it. > @@ -2169,16 +2192,9 @@ static int _mmc_resume(struct mmc_host *host) > int err = 0; > > mmc_claim_host(host); > - > - if (!mmc_card_suspended(host->card)) > - goto out; > - > - mmc_power_up(host, host->card->ocr); > - err = mmc_init_card(host, host->card->ocr, host->card); > - mmc_card_clr_suspended(host->card); > - > -out: > + err = __mmc_resume(host); > mmc_release_host(host); > + > return err; > } > > @@ -2189,6 +2205,9 @@ static int mmc_shutdown(struct mmc_host *host) > { > int err = 0; > > + if (host->undervoltage) > + return 0; > + Probably deserves a comment. > /* > * 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? > + > + 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. > + > + err = __mmc_suspend(host, false, true); > + } > + > + if (err) > + pr_err("%s: Undervoltage emergency stop failed\n", > + mmc_hostname(host)); > + > + mmc_card_set_removed(host->card); > + > +out: > + mmc_release_host(host); > + > + return err; > +} > + > static const struct mmc_bus_ops mmc_ops = { > .remove = mmc_remove, > .detect = mmc_detect, > @@ -2292,6 +2346,7 @@ static const struct mmc_bus_ops mmc_ops = { > .hw_reset = _mmc_hw_reset, > .cache_enabled = _mmc_cache_enabled, > .flush_cache = _mmc_flush_cache, > + .handle_undervoltage = _mmc_handle_undervoltage, > }; > > /*
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@pengutronix.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" > > @@ -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.
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@pengutronix.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?
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) { unsigned int notify_type; int err = 0; @@ -2115,8 +2115,6 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend, else notify_type = EXT_CSD_POWER_OFF_LONG; - mmc_claim_host(host); - if (mmc_card_suspended(host->card)) goto out; @@ -2140,7 +2138,18 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend, mmc_card_set_suspended(host->card); } out: + return err; +} + +static int _mmc_suspend(struct mmc_host *host, bool is_suspend, + bool is_undervoltage) +{ + int err; + + mmc_claim_host(host); + err = __mmc_suspend(host, is_suspend, is_undervoltage); mmc_release_host(host); + return err; } @@ -2160,6 +2169,20 @@ static int mmc_suspend(struct mmc_host *host) return err; } +static int __mmc_resume(struct mmc_host *host) +{ + int err; + + if (!mmc_card_suspended(host->card)) + return 0; + + mmc_power_up(host, host->card->ocr); + err = mmc_init_card(host, host->card->ocr, host->card); + mmc_card_clr_suspended(host->card); + + return err; +} + /* * This function tries to determine if the same card is still present * and, if so, restore all state to it. @@ -2169,16 +2192,9 @@ static int _mmc_resume(struct mmc_host *host) int err = 0; mmc_claim_host(host); - - if (!mmc_card_suspended(host->card)) - goto out; - - mmc_power_up(host, host->card->ocr); - err = mmc_init_card(host, host->card->ocr, host->card); - mmc_card_clr_suspended(host->card); - -out: + err = __mmc_resume(host); mmc_release_host(host); + return err; } @@ -2189,6 +2205,9 @@ static int mmc_shutdown(struct mmc_host *host) { int err = 0; + if (host->undervoltage) + return 0; + /* * 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); + + if (!err) { + err = mmc_interrupt_hpi(card); + if (err) + pr_err("%s: Interrupt HPI failed, error %d\n", + mmc_hostname(host), err); + + err = __mmc_suspend(host, false, true); + } + + if (err) + pr_err("%s: Undervoltage emergency stop failed\n", + mmc_hostname(host)); + + mmc_card_set_removed(host->card); + +out: + mmc_release_host(host); + + return err; +} + static const struct mmc_bus_ops mmc_ops = { .remove = mmc_remove, .detect = mmc_detect, @@ -2292,6 +2346,7 @@ static const struct mmc_bus_ops mmc_ops = { .hw_reset = _mmc_hw_reset, .cache_enabled = _mmc_cache_enabled, .flush_cache = _mmc_flush_cache, + .handle_undervoltage = _mmc_handle_undervoltage, }; /*
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. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/mmc/core/mmc.c | 81 +++++++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 13 deletions(-)