Re: [RFC] platform: detach from PM domains on shutdown

[Rafael J. Wysocki]

On Monday, May 28, 2018 10:01:09 AM CEST Peng Fan wrote:
> Hi, Rafael & Uffe
> 
> > -----Original Message-----
> > From: Peng Fan
> > Sent: 2018å5æ18æ 16:53
> > To: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Ulf Hansson
> > <ulf.hansson@xxxxxxxxxx>; Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>;
> > Fabio Estevam <fabio.estevam@xxxxxxx>; Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> > <linux-kernel@xxxxxxxxxxxxxxx>; Linux PM <linux-pm@xxxxxxxxxxxxxxx>;
> > dl-linux-imx <linux-imx@xxxxxxx>
> > Subject: RE: [RFC] platform: detach from PM domains on shutdown
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> > > Sent: 2018å5æ18æ 15:55
> > > To: Peng Fan <peng.fan@xxxxxxx>
> > > Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Ulf Hansson
> > > <ulf.hansson@xxxxxxxxxx>; Rafael J. Wysocki
> > > <rafael.j.wysocki@xxxxxxxxx>; Fabio Estevam <fabio.estevam@xxxxxxx>;
> > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Linux Kernel Mailing
> > > List <linux-kernel@xxxxxxxxxxxxxxx>; Linux PM
> > > <linux-pm@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
> > > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > >
> > > On Thursday, May 17, 2018 2:37:31 PM CEST Peng Fan wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: rjwysocki@xxxxxxxxx [mailto:rjwysocki@xxxxxxxxx] On Behalf
> > > > > Of Rafael J. Wysocki
> > > > > Sent: 2018å5æ17æ 16:01
> > > > > To: Peng Fan <peng.fan@xxxxxxx>
> > > > > Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Ulf Hansson
> > > > > <ulf.hansson@xxxxxxxxxx>; Rafael J. Wysocki
> > > > > <rafael.j.wysocki@xxxxxxxxx>; Fabio Estevam
> > > > > <fabio.estevam@xxxxxxx>; Greg Kroah-Hartman
> > > > > <gregkh@xxxxxxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> > > > > <linux-kernel@xxxxxxxxxxxxxxx>; Linux PM
> > > > > <linux-pm@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
> > > > > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > > >
> > > > > On Thu, May 17, 2018 at 4:33 AM, Peng Fan <peng.fan@xxxxxxx> wrote:
> > > > > >
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: rjwysocki@xxxxxxxxx [mailto:rjwysocki@xxxxxxxxx] On
> > > > > >> Behalf Of Rafael J. Wysocki
> > > > > >> Sent: 2018å5æ17æ 5:35
> > > > > >> To: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > > > > >> Cc: Peng Fan <peng.fan@xxxxxxx>; Rafael J. Wysocki
> > > > > >> <rafael.j.wysocki@xxxxxxxxx>; Fabio Estevam
> > > > > >> <fabio.estevam@xxxxxxx>; Greg Kroah-Hartman
> > > > > >> <gregkh@xxxxxxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> > > > > >> <linux-kernel@xxxxxxxxxxxxxxx>; Linux PM
> > > > > >> <linux-pm@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
> > > > > >> Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > > > >>
> > > > > >> On Wed, May 16, 2018 at 11:52 AM, Ulf Hansson
> > > > > >> <ulf.hansson@xxxxxxxxxx>
> > > > > >> wrote:
> > > > > >> > On 15 May 2018 at 11:01, Peng Fan <peng.fan@xxxxxxx> wrote:
> > > > > >> >> When reboot Linux, the PM domains attached to a device are
> > > > > >> >> not shutdown. To SoCs which relys on reset the whole SoC,
> > > > > >> >> there is no need to shutdown PM domains, but to Linux
> > > > > >> >> running in a virtual machine with devices pass-through, we
> > > > > >> >> could not reset the
> > > whole SoC.
> > > > > >> >> Currently we need Linux to shutdown its PM domains when reboot.
> > > > > >> >
> > > > > >> > I am not sure I understand exactly why the PM domain needs to
> > > > > >> > be shutdown for these cases, could you please elaborate a bit on that.
> > > > > >> >
> > > > > >> > BTW, what platform are you running on and also what PM
> > > > > >> > domains are being
> > > > > >> used?
> > > > > >> >
> > > > > >> > Anyway, it seems like there may be need for certain cases,
> > > > > >> > but certainly not all - especially since it may slow down the
> > > > > >> > shutdown process, when not needed.
> > > > > >> >
> > > > > >> > Can we make this runtime configurable, via sysfs or whatever
> > > > > >> > that makes
> > > > > >> sense!?
> > > > > >> >
> > > > > >> >>
> > > > > >> >> commit 2d30bb0b3889 ("platform: Do not detach from PM
> > > > > >> >> domains on shutdown"), removes what this patch tries to add,
> > > > > >> >> because of a
> > > warning.
> > > > > >> >> commit e79aee49bcf9 ("PM: Avoid false-positive warnings in
> > > > > >> >> dev_pm_domain_set()") already fixes the false alarm warning.
> > > > > >> >> So let's detach the power domain to shutdown PM domains
> > > > > >> >> after driver
> > > > > shutdown.
> > > > > >> >>
> > > > > >> >> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > > >> >> ---
> > > > > >> >>
> > > > > >> >> I do not find a better place to shutdown power domain when
> > > > > >> >> reboot Linux, so add back the line that commit 2d30bb0b3889
> > > > > >> >> removes, because it is a false alarm warning as commit
> > > > > >> >> e79aee49bcf9
> > > describes.
> > > > > >> >>
> > > > > >> >>  drivers/base/platform.c | 1 +
> > > > > >> >>  1 file changed, 1 insertion(+)
> > > > > >> >>
> > > > > >> >> diff --git a/drivers/base/platform.c
> > > > > >> >> b/drivers/base/platform.c index 8075ddc70a17..a5929f24dc3c
> > > > > >> >> 100644
> > > > > >> >> --- a/drivers/base/platform.c
> > > > > >> >> +++ b/drivers/base/platform.c
> > > > > >> >> @@ -616,6 +616,7 @@ static void platform_drv_shutdown(struct
> > > > > >> >> device
> > > > > >> >> *_dev)
> > > > > >> >>
> > > > > >> >>         if (drv->shutdown)
> > > > > >> >>                 drv->shutdown(dev);
> > > > > >> >> +       dev_pm_domain_detach(_dev, true);
> > > > > >> >
> > > > > >> > This would somewhat work, but only for platform devices. To
> > > > > >> > make this fully work, we need to call dev_pm_domain_detach()
> > > > > >> > from amba, spi, etc as well.
> > > > > >> >
> > > > > >> > Perhaps another option to manage this more generally, an
> > > > > >> > without having detach devices, could be to extend the struct
> > > > > >> > dev_pm_domain with a new callback, "->shutdown()" and then
> > > > > >> > make the driver core call it from device_shutdown().
> > > > > >>
> > > > > >> I'm sensing a possible ordering slippery slope with this (it
> > > > > >> will only work if all of the drivers/bus types etc do the right
> > > > > >> thing in their
> > > > > >> ->shutdown callbacks so nothing depends on the domain going
> > forward).
> > > > > >>
> > > > > >> > Typically, for genpd, I would probably count the number of
> > > > > >> > calls being made to ->shutdown() per PM domain, then when it
> > > > > >> > reaches the number of attached devices to it, allow to power off it.
> > > > > >> >
> > > > > >> > Let's see what Rafael thinks about it.
> > > > > >>
> > > > > >> I'm not sure about the use case.  The hypervisor should be able
> > > > > >> to take care of turning power domains off on the client OS
> > > > > >> reboot in theory.  If the client OS leaving the hypervisor
> > > > > >> needs to worry about what state it leaves behind, the design of
> > > > > >> the hypervisor is sort of
> > > > > questionable IMO.
> > > > > >
> > > > > > This is valid concern. But moving the power domain logic into
> > > > > > hypervisor mostly micro-kernel design will introduce more
> > > > > > complexity and
> > > > > make certification harder.
> > > > > > Currently, Let Linux shutdown it's power domain is the easiest
> > > > > > way to me and make things work well after reboot.
> > > > >
> > > > > Well, to put it bluntly, if your hypervisor depends on the guest
> > > > > to do the right thing on exit, it doesn't do its job.  I wouldn't
> > > > > have certified it for you if that was my decision.
> > > >
> > > > It is guest os not work well after guest os reboot. The hypervisor
> > > > is not
> > > affected.
> > > >
> > > > Thinking another case without hypervisor, M4 core run RTOS, A35 Core
> > > > run Linux, when Linux rebooting, RTOS should not be affected. After
> > > > Linux reboot itself, because its power domain is not paired with
> > > open/shutdown, some devices not function well.
> > >
> > > The question boils down to whether or not devices should be detached
> > > from PM domains on shutdown IMO.
> > >
> > > They are detached from PM domains on driver removal, so I guess one
> > > answer is "yes, in analogy with that".  However, the point about
> > > performace brought up by Ulf seems to be valid too.
> > >
> > > In any case, the change should be made for all of the bus types using
> > > PM domains, not just one.
> > 
> > Understand, it will increase shutdown time. How about shutdown the power
> > domain in platform_driver->shutdown, let the driver handle it's power domain
> > sthudown by itself?
> > Then no need common framework change.
> 
> Do you have more suggestions on how to handle this the power domain shutdown?

I think you could add a platform syscore_shutdown hook to turn all power
domains off.