Re: [RFC PATCH v2] mmc: suspend MMC also when unbinding

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3350 bytes --]

Hi Ulf,

On Wed, Jan 22, 2025 at 10:28:09AM +0100, Wolfram Sang wrote:
> Hi Ulf,
> 
> slowly coming back to this one.
> 
> > > +       card->state &= ~MMC_STATE_PRESENT; // TBD: mmc_card_clear_present()
> > 
> > This is nice from a consistency point of view. Although, I don't want
> > us to use this bit as an indication to inform the bus_ops->suspend()
> > callback what to do. It seems prone to problems.
> 
> Makes sense. We can use another bit like MMC_STATE_NEEDS_SUSPEND. That
> would also...
> 
> > > +       host->bus_ops->suspend(host);
> > 
> > I think this is a step in the right direction. Somehow we need to be
> > able to call the bus_ops->suspend() before we call put_device() and
> > before we clear the host->card pointer.
> > 
> > However, we don't want to call bus_ops->suspend() in all cases from
> > mmc_remove_card(), but *only* when it gets called from
> > mmc_remove_host()->mmc_stop_host(), via the
> > "host->bus_ops->remove(host)" callback.
> > 
> > I am wondering whether we should just re-work/split-up the code a bit
> > to make this work. In principle, when mmc_remove_card() is called from
> > the path above, we should not let it call put_device(), but leave that
> > part to the caller (mmc_stop_host()) instead. Or something along those
> > lines.
> 
> ... avoid this :) Refactoring every code which calls mmc_remove_card()
> to handle the additional put_device() on its own seems intrusive to me.
> And error prone, new users might forget to do it. And that for only one
> use case where we want to do extra stuff.
> 
> Leaving out put_device() within mmc_remove_card() only for that one case
> is bad API design, of course.
> 
> So, I envision more something along this:
> 
> 	if (card->state & MMC_STATE_NEEDS_SUSPEND)
> 		mmc_suspend()

Shall I try this...

> 
> Maybe even more generic?
> 
> 	if (card->state & MMC_STATE_PREPARE_REMOVE)
> 		bus_ops->prepare_remove()

... or even this suggestion?

All the best,

   Wolfram

> 
> Or am I missing something from your suggestion?
> 
> > > +       enum mmc_pm_reason reason = mmc_card_present(host->card) ?
> > > +                                   MMC_PM_REASON_SUSPEND : MMC_PM_REASON_REMOVAL;
> > 
> > I don't think it makes sense to differentiate between a regular
> > "suspend" and a "host-removal". The point is, we don't know what will
> > happen beyond the host-removal. The platform may shutdown or the host
> > driver probes again.
> > 
> > Let's just use the same commands as we use for suspend.
> 
> Sadly, I think it makes sense because of our HW. We cannot control the
> regulators directly. PSCI handles them. And for shutdown, it will do
> "something". For normal suspend, nothing will happen because PSCI will
> not be called. This is why Shimoda-san introduced
> "full-pwr-cycle-in-suspend" back then.
> 
> The differentiation is needed a little above:
> 
> > -	    ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
> > -	     (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND)))
> > +	    (can_pwr_cycle_now || reason == MMC_PM_REASON_SHUTDOWN))
> >               err = mmc_poweroff_notify(host->card, notify_type);
> 
> Here. Poweroff notification is only valid in the POWEROFF case for us.
> 
> Thanks for your time,
> 
>    Wolfram
> 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]