| Message ID | 20250407152759.25160-3-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: core: Add support for graceful host removal for eMMC/SD | expand |
> @@ -2187,11 +2198,12 @@ static int mmc_shutdown(struct mmc_host *host) > int err = 0; > > /* > - * In a specific case for poweroff notify, we need to resume the card > - * before we can shutdown it properly. > + * If the card remains suspended at this point and it was done by using > + * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow > + * us to send the preferred poweroff-notification cmd at shutdown. > */ > if (mmc_can_poweroff_notify(host->card) && > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > + !mmc_host_can_poweroff_notify(host, true)) Ooookay, I think I got this logic now. I think it makes sense to make it more explicit in the comment, though: "This is then the case when the card is able to handle poweroff notifications in general but the host could not initiate those for suspend." Something like this?
On Tue, 8 Apr 2025 at 10:09, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > > @@ -2187,11 +2198,12 @@ static int mmc_shutdown(struct mmc_host *host) > > int err = 0; > > > > /* > > - * In a specific case for poweroff notify, we need to resume the card > > - * before we can shutdown it properly. > > + * If the card remains suspended at this point and it was done by using > > + * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow > > + * us to send the preferred poweroff-notification cmd at shutdown. > > */ > > if (mmc_can_poweroff_notify(host->card) && > > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > > + !mmc_host_can_poweroff_notify(host, true)) > > Ooookay, I think I got this logic now. I think it makes sense to make it > more explicit in the comment, though: > > "This is then the case when the card is able to handle poweroff > notifications in general but the host could not initiate those for > suspend." > > Something like this? Well, in my opinion I think this would become a bit too much comments in the code. The rather long function-names "mmc_can_poweroff_notify" (that will change to mmc_card_can_poweroff_notify with your series) and "mmc_host_can_poweroff_notify" are rather self-explanatory, don't you think? Kind regards Uffe
> The rather long function-names "mmc_can_poweroff_notify" (that will > change to mmc_card_can_poweroff_notify with your series) and > "mmc_host_can_poweroff_notify" are rather self-explanatory, don't you > think? Well, you are the boss here, but frankly, I don't think it is obvious enough. I had to look twice and very closely to understand the logic. Not because of the function name, but for the reason why 'is_suspend' is true despite being in _shutdown(). Adrian was wondering about it the first time, too. So, I honestly think the comment is for a maintainer -> superfluous for a part-time-MMC-core-hacker -> helpful to remember for someone new to the code -> essential Something like this.
On Tue, 8 Apr 2025 at 17:07, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > > The rather long function-names "mmc_can_poweroff_notify" (that will > > change to mmc_card_can_poweroff_notify with your series) and > > "mmc_host_can_poweroff_notify" are rather self-explanatory, don't you > > think? > > Well, you are the boss here, but frankly, I don't think it is obvious > enough. I had to look twice and very closely to understand the logic. > Not because of the function name, but for the reason why 'is_suspend' is > true despite being in _shutdown(). Adrian was wondering about it the > first time, too. So, I honestly think the comment is > > for a maintainer -> superfluous > for a part-time-MMC-core-hacker -> helpful to remember > for someone new to the code -> essential > > Something like this. > I understand what you are saying and I agree. However, the problem is that your concern applies to a lot more code in the mmc core, but this condition. Don't get me wrong, I don't mind useful comments and good documentation, but perhaps what we are really missing is a general mmc documentation that describes how the core is working and in particular the power-management part of it. Unfortunately, I don't think I will have the bandwidth currently to work on this. That said, I am going to apply the $subject patch as is - but feel free to send a patch on top if you want to add and improve any further comments in the code. I would be happy to apply it! Kind regards Uffe
> I understand what you are saying and I agree. However, the problem is > that your concern applies to a lot more code in the mmc core, but this > condition. We can easily agree on that :) > Don't get me wrong, I don't mind useful comments and good > documentation, but perhaps what we are really missing is a general mmc > documentation that describes how the core is working and in particular > the power-management part of it. That would be the ideal solution, no doubt. > Unfortunately, I don't think I will have the bandwidth currently to > work on this. Same here. Plus, I don't have a complete understanding of it. Obtaining this understanding and then write some docs about my findings would be awesome, of course. But -EBUSY, too... > That said, I am going to apply the $subject patch as is - but feel I still think that having the comment is better than not having it, but I accept your decision and will still be happy that we finally solved the power-off-notification issue \o/
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 3424bc9e20c5..ee65c5b85f95 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -2014,6 +2014,18 @@ static bool mmc_can_poweroff_notify(const struct mmc_card *card) (card->ext_csd.power_off_notification == EXT_CSD_POWER_ON); } +static bool mmc_host_can_poweroff_notify(const struct mmc_host *host, + bool is_suspend) +{ + if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) + return true; + + if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND && is_suspend) + return true; + + return !is_suspend; +} + static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type) { unsigned int timeout = card->ext_csd.generic_cmd6_time; @@ -2124,8 +2136,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) goto out; if (mmc_can_poweroff_notify(host->card) && - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend || - (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND))) + mmc_host_can_poweroff_notify(host, is_suspend)) err = mmc_poweroff_notify(host->card, notify_type); else if (mmc_can_sleep(host->card)) err = mmc_sleep(host); @@ -2187,11 +2198,12 @@ static int mmc_shutdown(struct mmc_host *host) int err = 0; /* - * In a specific case for poweroff notify, we need to resume the card - * before we can shutdown it properly. + * If the card remains suspended at this point and it was done by using + * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow + * us to send the preferred poweroff-notification cmd at shutdown. */ if (mmc_can_poweroff_notify(host->card) && - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) + !mmc_host_can_poweroff_notify(host, true)) err = _mmc_resume(host); if (!err)