[PATCH] PM / Domains: Power on the PM domain right after attach completes

[Rafael J. Wysocki]

On Thursday, November 20, 2014 11:13:01 AM Ulf Hansson wrote:
> On 20 November 2014 01:35, Rafael J. Wysocki <rjw at rjwysocki.net> wrote:
> > On Wednesday, November 19, 2014 09:54:00 AM Ulf Hansson wrote:
> >> [...]
> >>
> >> >>
> >> >> Scenario 5), a platform driver with/without runtime PM callbacks.
> >> >> ->probe()
> >> >> - do some initialization
> >> >> - may fetch handles to runtime PM resources
> >> >> - pm_runtime_enable()
> >> >
> >> > Well, and now how the driver knows if the device is "on" before accessing it?
> >>
> >> In this case the driver don't need to access the device during
> >> ->probe(). That's postponed until sometime later.
> >
> > If this is a platform driver, it rather does need to access the device,
> > precisely because it doesn't know what power state the device is in otherwise.
> > See below.
> >
> >> >> Note 1)
> >> >> Scenario 1) and 2), both relies on the approach to power on the PM
> >> >> domain by using pm_runtime_get_sync(). That approach didn't work when
> >> >> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by
> >> >> the below patch, so that's good!
> >> >> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled"
> >> >>
> >> >> Note 2)
> >> >> Scenario 3) and 4) use the same principles for managing runtime PM.
> >> >> These scenarios needs a way to power on the generic PM domain prior
> >> >> probing the device. The call to pm_runtime_set_active(), prevents an
> >> >> already powered PM domain from power off until after probe, but that's
> >> >> not enough.
> >> >>
> >> >> Note 3)
> >> >> The $subject patch, tried to address the issues for scenario 3) and
> >> >> 4). It does so, but will affect scenario 5) which was working nicely
> >> >> before. In scenario 5), the $subject patch will cause the generic PM
> >> >> domain to potentially stay powered after ->probe() even if the device
> >> >> is runtime PM suspended.
> >> >
> >> > Why would it?  If the device is runtime-suspended, the domain will know
> >> > that, because its callbacks will be used for that.  At least, that's
> >> > what I'd expect to happen, so is there a problem here?
> >>
> >> Genpd do knows about the device but it doesn’t get a "notification" to
> >> power off. There are no issues whatsoever for driver.
> >
> > Except that the driver is arguably buggy.
> >
> >> This is a somewhat special case. Let's go through an example.
> >>
> >> 1. The PM domain is initially in powered off state.
> >> 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM
> >> domain gets attached to the device.
> >> 3. $subject patch causes the PM domain to power on.
> >> 4. A driver ->probe() sequence start, following the Scenario 5).
> >> 5. The device is initially in runtime PM suspended state and it will
> >> remain so during ->probe().
> >
> > But is it physically suspended?
> >
> > The runtime PM status of the device after ->probe is required to reflect its
> > real state if runtime PM is enabled.  If that's not the case, it is a bug.
> 
> Agree.
> 
> While I was searching for drivers that behave as in scenario 5), they
> tend to register some subsystem specific callbacks and don't access
> the device until some of those callbacks are invoked.
> 
> At least that was my interpretation of their ->probe() methods, but
> it's not always easy to tell how those callbacks are being used for
> each subsystem.
> 
> >
> > Now, for platform drivers, the driver can't really assume anything in
> > particular about the current power state of the device at ->probe time,
> > because different platforms including devices handled by that driver may
> > behave differently.
> >
> > A good example would be two platforms A and B where the same device X is in a
> > power domain such that A boots with the domain (physically) "on", while B boots
> > with the domain "off".  If the driver for X assumes anything about the initial
> > power state of the device, it may not work on either A or B.
> 
> I get your point and agree!
> 
> >
> >> 6. The pm_request_idle() invoked after really_probe() in driver core,
> >> won't trigger a runtime PM suspend callback to be invoked. In other
> >> words, genpd don't get a "notification" that it may power off.
> >>
> >> In this state, genpd will either power off from the late_initcall,
> >> genpd_poweroff_unused() (depending on when the driver was probed) or
> >> wait until some device's runtime PM suspend callback is invoked at any
> >> later point.
> >
> > Which sounds OK to me, so why is it a problem?
> 
> The late_initcall doesn't work for modules.
> 
> Also, suppose the PM domain only holds this "inactive device" that was
> probed as in scenario 5). Then, you could end up having the PM domain
> powered, even if it isn't needed.
> 
> Anyway, I can live with this. It's likely the driver that behave as in
> scenario 5) that should be fixed as you stated.
> 
> >
> >> >> I see three options going forward.
> >> >>
> >> >> Option 1)
> >> >> Convert scenario 3) and 4) into using the pm_runtime_get_sync()
> >> >> approach. There are no theoretical obstacles to do this, but pure
> >> >> practical. There are a lot of drivers that needs to be converted and
> >> >> we also need to teach driver authors future wise to not use
> >> >> pm_runtime_set_active() in this manner.
> >> >
> >> > I'd say we need to do something like this anyway.  That is, standardize on
> >> > *one* approach.  I'm actually not sure what approach is the most useful,
> >> > but the pm_runtime_get_sync() one seems to be the most popular to me.
> >> >
> >> >> Option 2)
> >> >> Add some kind of get/put API for PM domains. The buses invokes it to
> >> >> control the power to the PM domain. From what I understand, that's
> >> >> also what Dmitry think is needed!?
> >> >> Anyway, that somehow means to proceed with the approach I took in the
> >> >> below patchset.
> >> >> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
> >> >> http://marc.info/?t=141320907000003&r=1&w=2
> >> >
> >> > I don't like that.  The API is already quite complicated in my view and
> >> > adding even more complexity to it is not going to help in the long run.
> >>
> >> I absolutely agree that we shouldn't add unnecessary APIs and keep
> >> APIs as simple as possible.
> >>
> >> In that context, I think the effect from proceeding with Option 2)
> >> also means there are no need for the below APIs any more.
> >> pm_genpd_poweron()
> >> pm_genpd_name_poweron() (requires some additional work though)
> >> pm_genpd_poweroff_unused()
> >> pm_genpd_dev_need_restore()
> >>
> >> I guess you figured out that I am in favour of Option 2). :-)
> >> Especially since it cover all scenarios and we don't have to go a fix
> >> a vast amount of drivers.
> >
> > Adding more callbacks to struct dev_pm_domain just in order to handle some
> > genpd-specific use case is out of the question.  If you find a way within
> > genpd to address this the way you like, I'm open for ideas.  Otherwise,
> > let's just do what I said.
> >
> >> >
> >> >> Option 3)
> >> >> Live we the limitation this $subject patch introduces for scenario 5).
> >> >
> >> > I'd say, 3) for now and 1) going forward.
> >>
> >> This could work!
> >>
> >> The hardest part is to know when we should revert $subject patch, to
> >> fix the regression introduced for scenario 5).
> >
> > Well, I don't think so.
> >
> > First of all, there is a (very) limited number of platforms using genpd today
> > and it should be perfectly possible to go through the platform drivers used
> > by them and see which of those drivers are affected.  Fixing them should not
> > be too hard either.
> 
> Considering that scenario 5) probably exists because of buggy drivers
> and that the related issues is very limited, I am perfectly fine with
> your proposal!
> 
> Will you queue this patch then?

I will, but I'll change the first line of the changelog to read: "Vast amount
of platform drivers enables runtime PM, [...]" (ie. leave out AMBA from there).

Rafael