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

The amba bus, amba drivers and a vast amount of platform drivers which
enables runtime PM, don't invoke a pm_runtime_get_sync() while probing
their devices.

Instead, once they have turned on their PM resourses during ->probe()
and are ready to handle I/O, these invokes pm_runtime_set_active() to
synchronize its state towards the runtime PM core.

From a runtime PM point of view this behavior is perfectly acceptable,
but we encounter probe failures if their corresponding devices resides
in the generic PM domain. The issues are observed for those devices,
which requires its PM domain to stay powered during ->probe() since
that's not being controlled.

While using the generic OF-based PM domain look-up, a device's PM
domain will be attached during the probe sequence. For this path, let's
fix the probe failures, by simply power on the PM domain right after
when it's been attached to the device.

The generic PM domain stays powered until all of its devices becomes
runtime PM enabled and runtime PM suspended.

The old SOCs which makes use of the generic PM domain but don't use the
generic OF-based PM domain look-up, will not be affected from this
change.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 1 +
 1 file changed, 1 insertion(+)

On Mon, Nov 17, 2014 at 04:19:10PM +0100, Ulf Hansson wrote:
> The amba bus, amba drivers and a vast amount of platform drivers which
> enables runtime PM, don't invoke a pm_runtime_get_sync() while probing
> their devices.
> 
> Instead, once they have turned on their PM resourses during ->probe()
> and are ready to handle I/O, these invokes pm_runtime_set_active() to
> synchronize its state towards the runtime PM core.

The above is misleading for amba.  The code sequence is:

                pm_runtime_get_noresume(dev);
                pm_runtime_set_active(dev);
                pm_runtime_enable(dev);

                ret = pcdrv->probe(pcdev, id);

AMBA drivers should never call pm_runtime_set_active(), as the runtime PM
state has already been initialised by the bus code.  Platform drivers are
different; the platform code provides zero help for runtime PM.

The sequence used by AMBA bus code is the sequence which was used by PCI
(as per commit f3ec4f87d607) at the time the runtime PM support was
written for AMBA.  PCI assumes that unbound devices are already powered
up, and as far as I'm aware, that's also true of AMBA devices as well.
I have yet to have access to a platform where this isn't true, neither
has anyone reported that such a platform exists.

Ulf Hansson <ulf.hansson@linaro.org> writes:

> The amba bus, amba drivers and a vast amount of platform drivers which
> enables runtime PM, don't invoke a pm_runtime_get_sync() while probing
> their devices.
>
> Instead, once they have turned on their PM resourses during ->probe()
> and are ready to handle I/O, these invokes pm_runtime_set_active() to
> synchronize its state towards the runtime PM core.
>
> From a runtime PM point of view this behavior is perfectly acceptable,

In the context of PM domains that can be dynamically powered on/off, I'm
not so sure it's perfectly acceptable anymore.

Why doesn't the bus do a _get_sync() instead of a _get_noresume()
followed by a _set_active().  

By using the _get_noresume() you're bypassing the paths that would bring
up your PM domain.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 17 Nov 2014, Kevin Hilman wrote:

> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
> > The amba bus, amba drivers and a vast amount of platform drivers which
> > enables runtime PM, don't invoke a pm_runtime_get_sync() while probing
> > their devices.
> >
> > Instead, once they have turned on their PM resourses during ->probe()
> > and are ready to handle I/O, these invokes pm_runtime_set_active() to
> > synchronize its state towards the runtime PM core.
> >
> > From a runtime PM point of view this behavior is perfectly acceptable,
> 
> In the context of PM domains that can be dynamically powered on/off, I'm
> not so sure it's perfectly acceptable anymore.
> 
> Why doesn't the bus do a _get_sync() instead of a _get_noresume()
> followed by a _set_active().  
> 
> By using the _get_noresume() you're bypassing the paths that would bring
> up your PM domain.

This probably comes about because devices that are part of a power
domain need special treatment.  Before the driver's probe routine gets
called, the bus ought to resume the entire power domain.  For that,
some sort of _get_sync() is needed.

For devices that aren't part of a power domain, things are simpler.  
The bus does _get_noresume() to make sure the device won't be runtime 
suspended while the probe routine is running.  It doesn't do 
_get_sync(), because that would end up calling the driver's 
runtime_resume routine before the driver was bound to the device.  (The 
bus could prevent that from happening by taking special precautions,
like PCI does, but in general it's a nuisance.)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Nov 17, 2014 at 11:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 17 Nov 2014, Kevin Hilman wrote:
>
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>> > The amba bus, amba drivers and a vast amount of platform drivers which
>> > enables runtime PM, don't invoke a pm_runtime_get_sync() while probing
>> > their devices.
>> >
>> > Instead, once they have turned on their PM resourses during ->probe()
>> > and are ready to handle I/O, these invokes pm_runtime_set_active() to
>> > synchronize its state towards the runtime PM core.
>> >
>> > From a runtime PM point of view this behavior is perfectly acceptable,
>>
>> In the context of PM domains that can be dynamically powered on/off, I'm
>> not so sure it's perfectly acceptable anymore.
>>
>> Why doesn't the bus do a _get_sync() instead of a _get_noresume()
>> followed by a _set_active().
>>
>> By using the _get_noresume() you're bypassing the paths that would bring
>> up your PM domain.
>
> This probably comes about because devices that are part of a power
> domain need special treatment.  Before the driver's probe routine gets
> called, the bus ought to resume the entire power domain.  For that,
> some sort of _get_sync() is needed.
>
> For devices that aren't part of a power domain, things are simpler.
> The bus does _get_noresume() to make sure the device won't be runtime
> suspended while the probe routine is running.  It doesn't do
> _get_sync(), because that would end up calling the driver's
> runtime_resume routine before the driver was bound to the device.  (The
> bus could prevent that from happening by taking special precautions,
> like PCI does, but in general it's a nuisance.)

That's why I think we need some new call that would mean "make sure the
device is powered" which would properly handle power domain and bus, but
ignore all driver stuff since it may not be initialized yet. And similar
call for asking to put device and maybe domain in powered down state in
case probing failed.

Thanks.

On Mon, 17 Nov 2014, Dmitry Torokhov wrote:

> > For devices that aren't part of a power domain, things are simpler.
> > The bus does _get_noresume() to make sure the device won't be runtime
> > suspended while the probe routine is running.  It doesn't do
> > _get_sync(), because that would end up calling the driver's
> > runtime_resume routine before the driver was bound to the device.  (The
> > bus could prevent that from happening by taking special precautions,
> > like PCI does, but in general it's a nuisance.)
> 
> That's why I think we need some new call that would mean "make sure the
> device is powered" which would properly handle power domain and bus, but
> ignore all driver stuff since it may not be initialized yet. And similar
> call for asking to put device and maybe domain in powered down state in
> case probing failed.

I can't imagine how such a call would work.

The PM core invokes the subsystem's runtime_suspend/resume callback, 
and then the subsystem's routine is responsible for invoking the 
driver's callback (or _not_ invoking it, in this case).

Thus, the PM core has no way to tell the subsystem's callback not to
invoke the driver's routine, and adding a new runtime PM call wouldn't
change that.  You'd have to add a new pair of callbacks instead, which 
IMO would be a tremendous waste.

Furthermore, the subsystem already _knows_ when the driver gets probed, 
because probing works in the same sort of way: The subsystem's probe 
routine gets invoked, and it is responsible for invoking the driver's 
probe routine.  Therefore the PM core doesn't _need_ to provide this 
extra information to the subsystem.  Rather, the subsystem just needs 
to keep track of the information it already has available.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Nov 17, 2014 at 02:54:53PM -0500, Alan Stern wrote:
> On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> 
> > > For devices that aren't part of a power domain, things are simpler.
> > > The bus does _get_noresume() to make sure the device won't be runtime
> > > suspended while the probe routine is running.  It doesn't do
> > > _get_sync(), because that would end up calling the driver's
> > > runtime_resume routine before the driver was bound to the device.  (The
> > > bus could prevent that from happening by taking special precautions,
> > > like PCI does, but in general it's a nuisance.)
> > 
> > That's why I think we need some new call that would mean "make sure the
> > device is powered" which would properly handle power domain and bus, but
> > ignore all driver stuff since it may not be initialized yet. And similar
> > call for asking to put device and maybe domain in powered down state in
> > case probing failed.
> 
> I can't imagine how such a call would work.
> 
> The PM core invokes the subsystem's runtime_suspend/resume callback, 
> and then the subsystem's routine is responsible for invoking the 
> driver's callback (or _not_ invoking it, in this case).
> 
> Thus, the PM core has no way to tell the subsystem's callback not to
> invoke the driver's routine, and adding a new runtime PM call wouldn't
> change that.  You'd have to add a new pair of callbacks instead, which 
> IMO would be a tremendous waste.
> 
> Furthermore, the subsystem already _knows_ when the driver gets probed, 
> because probing works in the same sort of way: The subsystem's probe 
> routine gets invoked, and it is responsible for invoking the driver's 
> probe routine.  Therefore the PM core doesn't _need_ to provide this 
> extra information to the subsystem.  Rather, the subsystem just needs 
> to keep track of the information it already has available.

You are missing concept of power domains in this picture. True,
subsystem knows when it probes but power domain does not. Subsystem has
no knowledge of power domain (devices in the same subsystem can come
from different domains).

We need to have either subsystem or device core to indicate to power
management core that we do not need "full" runtime resume, but rather a
"partial" one since driver is not ready yet.

We would not need new callbacks here I think, we just need to be able to
select appropriate set of callbacks, depending on the binding state.

Thanks.

On Mon, 17 Nov 2014, Dmitry Torokhov wrote:

> On Mon, Nov 17, 2014 at 02:54:53PM -0500, Alan Stern wrote:
> > On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> > 
> > > > For devices that aren't part of a power domain, things are simpler.
> > > > The bus does _get_noresume() to make sure the device won't be runtime
> > > > suspended while the probe routine is running.  It doesn't do
> > > > _get_sync(), because that would end up calling the driver's
> > > > runtime_resume routine before the driver was bound to the device.  (The
> > > > bus could prevent that from happening by taking special precautions,
> > > > like PCI does, but in general it's a nuisance.)
> > > 
> > > That's why I think we need some new call that would mean "make sure the
> > > device is powered" which would properly handle power domain and bus, but
> > > ignore all driver stuff since it may not be initialized yet. And similar
> > > call for asking to put device and maybe domain in powered down state in
> > > case probing failed.
> > 
> > I can't imagine how such a call would work.
> > 
> > The PM core invokes the subsystem's runtime_suspend/resume callback, 
> > and then the subsystem's routine is responsible for invoking the 
> > driver's callback (or _not_ invoking it, in this case).
> > 
> > Thus, the PM core has no way to tell the subsystem's callback not to
> > invoke the driver's routine, and adding a new runtime PM call wouldn't
> > change that.  You'd have to add a new pair of callbacks instead, which 
> > IMO would be a tremendous waste.
> > 
> > Furthermore, the subsystem already _knows_ when the driver gets probed, 
> > because probing works in the same sort of way: The subsystem's probe 
> > routine gets invoked, and it is responsible for invoking the driver's 
> > probe routine.  Therefore the PM core doesn't _need_ to provide this 
> > extra information to the subsystem.  Rather, the subsystem just needs 
> > to keep track of the information it already has available.
> 
> You are missing concept of power domains in this picture. True,
> subsystem knows when it probes but power domain does not. Subsystem has
> no knowledge of power domain (devices in the same subsystem can come
> from different domains).

Okay, I take your point.

> We need to have either subsystem or device core to indicate to power
> management core that we do not need "full" runtime resume, but rather a
> "partial" one since driver is not ready yet.
> 
> We would not need new callbacks here I think, we just need to be able to
> select appropriate set of callbacks, depending on the binding state.

When the runtime PM core invokes a power domain's callback routine,
what does the domain's routine usually do?  Does it go ahead and invoke
the driver's callback?  Or does it try to invoke the subsystem's
callback?

Obviously this depends on how the power domain code is written.  But
suppose every power domain would always use the same strategy as the PM
core: Invoke the subsystem's callback if there is one; otherwise invoke
the driver's callback.

Then there wouldn't be a problem.  Even when a runtime-resume went via
the power domain, the subsystem would still be able to protect the
not-yet-bound driver from being called.

(... Unless the subsystem itself was incapable of doing this the right 
way.  But subsystems can be fixed.)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Nov 17, 2014 at 03:49:14PM -0500, Alan Stern wrote:
> On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> 
> > On Mon, Nov 17, 2014 at 02:54:53PM -0500, Alan Stern wrote:
> > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> > > 
> > > > > For devices that aren't part of a power domain, things are simpler.
> > > > > The bus does _get_noresume() to make sure the device won't be runtime
> > > > > suspended while the probe routine is running.  It doesn't do
> > > > > _get_sync(), because that would end up calling the driver's
> > > > > runtime_resume routine before the driver was bound to the device.  (The
> > > > > bus could prevent that from happening by taking special precautions,
> > > > > like PCI does, but in general it's a nuisance.)
> > > > 
> > > > That's why I think we need some new call that would mean "make sure the
> > > > device is powered" which would properly handle power domain and bus, but
> > > > ignore all driver stuff since it may not be initialized yet. And similar
> > > > call for asking to put device and maybe domain in powered down state in
> > > > case probing failed.
> > > 
> > > I can't imagine how such a call would work.
> > > 
> > > The PM core invokes the subsystem's runtime_suspend/resume callback, 
> > > and then the subsystem's routine is responsible for invoking the 
> > > driver's callback (or _not_ invoking it, in this case).
> > > 
> > > Thus, the PM core has no way to tell the subsystem's callback not to
> > > invoke the driver's routine, and adding a new runtime PM call wouldn't
> > > change that.  You'd have to add a new pair of callbacks instead, which 
> > > IMO would be a tremendous waste.
> > > 
> > > Furthermore, the subsystem already _knows_ when the driver gets probed, 
> > > because probing works in the same sort of way: The subsystem's probe 
> > > routine gets invoked, and it is responsible for invoking the driver's 
> > > probe routine.  Therefore the PM core doesn't _need_ to provide this 
> > > extra information to the subsystem.  Rather, the subsystem just needs 
> > > to keep track of the information it already has available.
> > 
> > You are missing concept of power domains in this picture. True,
> > subsystem knows when it probes but power domain does not. Subsystem has
> > no knowledge of power domain (devices in the same subsystem can come
> > from different domains).
> 
> Okay, I take your point.
> 
> > We need to have either subsystem or device core to indicate to power
> > management core that we do not need "full" runtime resume, but rather a
> > "partial" one since driver is not ready yet.
> > 
> > We would not need new callbacks here I think, we just need to be able to
> > select appropriate set of callbacks, depending on the binding state.
> 
> When the runtime PM core invokes a power domain's callback routine,
> what does the domain's routine usually do?  Does it go ahead and invoke
> the driver's callback?  Or does it try to invoke the subsystem's
> callback?
> 
> Obviously this depends on how the power domain code is written.  But
> suppose every power domain would always use the same strategy as the PM
> core: Invoke the subsystem's callback if there is one; otherwise invoke
> the driver's callback.
> 
> Then there wouldn't be a problem.  Even when a runtime-resume went via
> the power domain, the subsystem would still be able to protect the
> not-yet-bound driver from being called.
> 
> (... Unless the subsystem itself was incapable of doing this the right 
> way.  But subsystems can be fixed.)

The genpd code currently starts by powering on the domain (if it is not
on already) and then does "device restore" which is:

/**
 * pm_genpd_default_restore_state - Default PM domians "restore device state".
 * @dev: Device to handle.
 */
static int pm_genpd_default_restore_state(struct device *dev)
{
	int (*cb)(struct device *__dev);

	cb = dev_gpd_data(dev)->ops.restore_state;
	if (cb)
		return cb(dev);

	if (dev->type && dev->type->pm)
		cb = dev->type->pm->runtime_resume;
	else if (dev->class && dev->class->pm)
		cb = dev->class->pm->runtime_resume;
	else if (dev->bus && dev->bus->pm)
		cb = dev->bus->pm->runtime_resume;
	else
		cb = NULL;

	if (!cb && dev->driver && dev->driver->pm)
		cb = dev->driver->pm->runtime_resume;

	return cb ? cb(dev) : 0;
}

So I guess bus (or class or type) can take care of it. Except buses
usually call pm_generic_runtime_resume() which ends up fetching driver's
callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?

Thanks.

On Mon, 17 Nov 2014, Dmitry Torokhov wrote:

> > When the runtime PM core invokes a power domain's callback routine,
> > what does the domain's routine usually do?  Does it go ahead and invoke
> > the driver's callback?  Or does it try to invoke the subsystem's
> > callback?
> > 
> > Obviously this depends on how the power domain code is written.  But
> > suppose every power domain would always use the same strategy as the PM
> > core: Invoke the subsystem's callback if there is one; otherwise invoke
> > the driver's callback.
> > 
> > Then there wouldn't be a problem.  Even when a runtime-resume went via
> > the power domain, the subsystem would still be able to protect the
> > not-yet-bound driver from being called.
> > 
> > (... Unless the subsystem itself was incapable of doing this the right 
> > way.  But subsystems can be fixed.)
> 
> The genpd code currently starts by powering on the domain (if it is not
> on already) and then does "device restore" which is:
> 
> /**
>  * pm_genpd_default_restore_state - Default PM domians "restore device state".
>  * @dev: Device to handle.
>  */
> static int pm_genpd_default_restore_state(struct device *dev)
> {
> 	int (*cb)(struct device *__dev);
> 
> 	cb = dev_gpd_data(dev)->ops.restore_state;
> 	if (cb)
> 		return cb(dev);
> 
> 	if (dev->type && dev->type->pm)
> 		cb = dev->type->pm->runtime_resume;
> 	else if (dev->class && dev->class->pm)
> 		cb = dev->class->pm->runtime_resume;
> 	else if (dev->bus && dev->bus->pm)
> 		cb = dev->bus->pm->runtime_resume;
> 	else
> 		cb = NULL;
> 
> 	if (!cb && dev->driver && dev->driver->pm)
> 		cb = dev->driver->pm->runtime_resume;
> 
> 	return cb ? cb(dev) : 0;
> }
> 
> So I guess bus (or class or type) can take care of it.

The bus could.  I don't think the class or type knows when a driver is 
being probed.

>  Except buses
> usually call pm_generic_runtime_resume() which ends up fetching driver's
> callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?

No, the bus subsystem needs to be smarter.  It shouldn't call 
pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
the driver has already been unbound from the device.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Nov 17, 2014 at 04:44:56PM -0500, Alan Stern wrote:
> On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> 
> > > When the runtime PM core invokes a power domain's callback routine,
> > > what does the domain's routine usually do?  Does it go ahead and invoke
> > > the driver's callback?  Or does it try to invoke the subsystem's
> > > callback?
> > > 
> > > Obviously this depends on how the power domain code is written.  But
> > > suppose every power domain would always use the same strategy as the PM
> > > core: Invoke the subsystem's callback if there is one; otherwise invoke
> > > the driver's callback.
> > > 
> > > Then there wouldn't be a problem.  Even when a runtime-resume went via
> > > the power domain, the subsystem would still be able to protect the
> > > not-yet-bound driver from being called.
> > > 
> > > (... Unless the subsystem itself was incapable of doing this the right 
> > > way.  But subsystems can be fixed.)
> > 
> > The genpd code currently starts by powering on the domain (if it is not
> > on already) and then does "device restore" which is:
> > 
> > /**
> >  * pm_genpd_default_restore_state - Default PM domians "restore device state".
> >  * @dev: Device to handle.
> >  */
> > static int pm_genpd_default_restore_state(struct device *dev)
> > {
> > 	int (*cb)(struct device *__dev);
> > 
> > 	cb = dev_gpd_data(dev)->ops.restore_state;
> > 	if (cb)
> > 		return cb(dev);
> > 
> > 	if (dev->type && dev->type->pm)
> > 		cb = dev->type->pm->runtime_resume;
> > 	else if (dev->class && dev->class->pm)
> > 		cb = dev->class->pm->runtime_resume;
> > 	else if (dev->bus && dev->bus->pm)
> > 		cb = dev->bus->pm->runtime_resume;
> > 	else
> > 		cb = NULL;
> > 
> > 	if (!cb && dev->driver && dev->driver->pm)
> > 		cb = dev->driver->pm->runtime_resume;
> > 
> > 	return cb ? cb(dev) : 0;
> > }
> > 
> > So I guess bus (or class or type) can take care of it.
> 
> The bus could.  I don't think the class or type knows when a driver is 
> being probed.
> 
> >  Except buses
> > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> 
> No, the bus subsystem needs to be smarter.  It shouldn't call 
> pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> the driver has already been unbound from the device.

But that code wold be exactly the same for all buses, right? So why
can't pm_generic_runtime_resume() be smarter?

However, is it allowed to call pm_runtime_get_sync() on devices that
didn't issue pm_runtime_enable()?

Thanks.

On Mon, 17 Nov 2014, Dmitry Torokhov wrote:

> > >  Except buses
> > > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> > 
> > No, the bus subsystem needs to be smarter.  It shouldn't call 
> > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> > the driver has already been unbound from the device.
> 
> But that code wold be exactly the same for all buses, right? So why
> can't pm_generic_runtime_resume() be smarter?

It would not be the same for all buses.  Each bus will have its own way 
of recognizing whether or not a driver has been probed (i.e., by 
checking some field in the bus-specific part of the device structure).

> However, is it allowed to call pm_runtime_get_sync() on devices that
> didn't issue pm_runtime_enable()?

Yes.  But the bus has to issue pm_runtime_enable() before probing the 
driver, because the driver will expect runtime PM to work properly 
while its probe routine runs.  For example, the probe routine might 
want to leave the device in a runtime-suspended state.  It can't do 
that if the device isn't enabled for runtime PM.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote:
> On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> 
> > > >  Except buses
> > > > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> > > 
> > > No, the bus subsystem needs to be smarter.  It shouldn't call 
> > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> > > the driver has already been unbound from the device.
> > 
> > But that code wold be exactly the same for all buses, right? So why
> > can't pm_generic_runtime_resume() be smarter?
> 
> It would not be the same for all buses.  Each bus will have its own way 
> of recognizing whether or not a driver has been probed (i.e., by 
> checking some field in the bus-specific part of the device structure).
> 
> > However, is it allowed to call pm_runtime_get_sync() on devices that
> > didn't issue pm_runtime_enable()?
> 
> Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> driver, because the driver will expect runtime PM to work properly 
> while its probe routine runs.  For example, the probe routine might 
> want to leave the device in a runtime-suspended state.  It can't do 
> that if the device isn't enabled for runtime PM.

That means that runtime PM will be enabled for all devices on given bus
while up till now drivers were deciding if their devices should be
runtime-pm-managed or not. I do not think we are quite ready for this.

Thanks.

On Tue, Nov 18, 2014 at 12:28:26AM +0100, Rafael J. Wysocki wrote:
> On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote:
> > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote:
> > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> > > 
> > > > > >  Except buses
> > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> > > > > 
> > > > > No, the bus subsystem needs to be smarter.  It shouldn't call 
> > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> > > > > the driver has already been unbound from the device.
> > > > 
> > > > But that code wold be exactly the same for all buses, right? So why
> > > > can't pm_generic_runtime_resume() be smarter?
> > > 
> > > It would not be the same for all buses.  Each bus will have its own way 
> > > of recognizing whether or not a driver has been probed (i.e., by 
> > > checking some field in the bus-specific part of the device structure).
> > > 
> > > > However, is it allowed to call pm_runtime_get_sync() on devices that
> > > > didn't issue pm_runtime_enable()?
> > > 
> > > Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> > > driver, because the driver will expect runtime PM to work properly 
> > > while its probe routine runs.  For example, the probe routine might 
> > > want to leave the device in a runtime-suspended state.  It can't do 
> > > that if the device isn't enabled for runtime PM.
> > 
> > That means that runtime PM will be enabled for all devices on given bus
> > while up till now drivers were deciding if their devices should be
> > runtime-pm-managed or not.
> 
> That's not the case for PCI drivers.
> 
> > I do not think we are quite ready for this.
> 
> We have to do that if power domains are in use, however, because if at least
> one device in a power domain in enabled for runtime PM, that will affect the
> other devices in that domain.
> 
> We could make a rule to keep a domail always up if at least one device in it
> has runtime PM disabled, but that is equivalent to enabling runtime PM for
> that device, powering the domain up and bumping up the device's usage counter.

What will driver will see if it tries to check pm_runtime_active()?
Would not it get unexpected result if the driver did not call
pm_runtime_enable() on it's device?

On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote:
> On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote:
> > On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> > 
> > > > >  Except buses
> > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> > > > 
> > > > No, the bus subsystem needs to be smarter.  It shouldn't call 
> > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> > > > the driver has already been unbound from the device.
> > > 
> > > But that code wold be exactly the same for all buses, right? So why
> > > can't pm_generic_runtime_resume() be smarter?
> > 
> > It would not be the same for all buses.  Each bus will have its own way 
> > of recognizing whether or not a driver has been probed (i.e., by 
> > checking some field in the bus-specific part of the device structure).
> > 
> > > However, is it allowed to call pm_runtime_get_sync() on devices that
> > > didn't issue pm_runtime_enable()?
> > 
> > Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> > driver, because the driver will expect runtime PM to work properly 
> > while its probe routine runs.  For example, the probe routine might 
> > want to leave the device in a runtime-suspended state.  It can't do 
> > that if the device isn't enabled for runtime PM.
> 
> That means that runtime PM will be enabled for all devices on given bus
> while up till now drivers were deciding if their devices should be
> runtime-pm-managed or not.

That's not the case for PCI drivers.

> I do not think we are quite ready for this.

We have to do that if power domains are in use, however, because if at least
one device in a power domain in enabled for runtime PM, that will affect the
other devices in that domain.

We could make a rule to keep a domail always up if at least one device in it
has runtime PM disabled, but that is equivalent to enabling runtime PM for
that device, powering the domain up and bumping up the device's usage counter.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday, November 17, 2014 03:26:04 PM Dmitry Torokhov wrote:
> On Tue, Nov 18, 2014 at 12:28:26AM +0100, Rafael J. Wysocki wrote:
> > On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote:
> > > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote:
> > > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> > > > 
> > > > > > >  Except buses
> > > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> > > > > > 
> > > > > > No, the bus subsystem needs to be smarter.  It shouldn't call 
> > > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> > > > > > the driver has already been unbound from the device.
> > > > > 
> > > > > But that code wold be exactly the same for all buses, right? So why
> > > > > can't pm_generic_runtime_resume() be smarter?
> > > > 
> > > > It would not be the same for all buses.  Each bus will have its own way 
> > > > of recognizing whether or not a driver has been probed (i.e., by 
> > > > checking some field in the bus-specific part of the device structure).
> > > > 
> > > > > However, is it allowed to call pm_runtime_get_sync() on devices that
> > > > > didn't issue pm_runtime_enable()?
> > > > 
> > > > Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> > > > driver, because the driver will expect runtime PM to work properly 
> > > > while its probe routine runs.  For example, the probe routine might 
> > > > want to leave the device in a runtime-suspended state.  It can't do 
> > > > that if the device isn't enabled for runtime PM.
> > > 
> > > That means that runtime PM will be enabled for all devices on given bus
> > > while up till now drivers were deciding if their devices should be
> > > runtime-pm-managed or not.
> > 
> > That's not the case for PCI drivers.
> > 
> > > I do not think we are quite ready for this.
> > 
> > We have to do that if power domains are in use, however, because if at least
> > one device in a power domain in enabled for runtime PM, that will affect the
> > other devices in that domain.
> > 
> > We could make a rule to keep a domail always up if at least one device in it
> > has runtime PM disabled, but that is equivalent to enabling runtime PM for
> > that device, powering the domain up and bumping up the device's usage counter.
> 
> What will driver will see if it tries to check pm_runtime_active()?
> Would not it get unexpected result if the driver did not call
> pm_runtime_enable() on it's device?

Well, that part was supposed to depend on the bus type.  For example, it
won't be unexpected for PCI drivers, because runtime PM is always enabled
for PCI devices (although it may be blocked as I described).

A problem arises if a power domain is used along with a bus type that doesn't
enable runtime PM for devices by default and drivers are expected to do that.
That could be addressed by powering up a PM domain (and bumping up its usage
counter) when adding a device to it and keeping it up until all of the devices
in it are runtime suspended, but then it also should be turned off eventually
if there are no drivers for any of those devices.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 18, 2014 01:26:38 AM Rafael J. Wysocki wrote:
> On Monday, November 17, 2014 03:26:04 PM Dmitry Torokhov wrote:
> > On Tue, Nov 18, 2014 at 12:28:26AM +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote:
> > > > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote:
> > > > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> > > > > 
> > > > > > > >  Except buses
> > > > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's
> > > > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter?
> > > > > > > 
> > > > > > > No, the bus subsystem needs to be smarter.  It shouldn't call 
> > > > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if 
> > > > > > > the driver has already been unbound from the device.
> > > > > > 
> > > > > > But that code wold be exactly the same for all buses, right? So why
> > > > > > can't pm_generic_runtime_resume() be smarter?
> > > > > 
> > > > > It would not be the same for all buses.  Each bus will have its own way 
> > > > > of recognizing whether or not a driver has been probed (i.e., by 
> > > > > checking some field in the bus-specific part of the device structure).
> > > > > 
> > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that
> > > > > > didn't issue pm_runtime_enable()?
> > > > > 
> > > > > Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> > > > > driver, because the driver will expect runtime PM to work properly 
> > > > > while its probe routine runs.  For example, the probe routine might 
> > > > > want to leave the device in a runtime-suspended state.  It can't do 
> > > > > that if the device isn't enabled for runtime PM.
> > > > 
> > > > That means that runtime PM will be enabled for all devices on given bus
> > > > while up till now drivers were deciding if their devices should be
> > > > runtime-pm-managed or not.
> > > 
> > > That's not the case for PCI drivers.
> > > 
> > > > I do not think we are quite ready for this.
> > > 
> > > We have to do that if power domains are in use, however, because if at least
> > > one device in a power domain in enabled for runtime PM, that will affect the
> > > other devices in that domain.
> > > 
> > > We could make a rule to keep a domail always up if at least one device in it
> > > has runtime PM disabled, but that is equivalent to enabling runtime PM for
> > > that device, powering the domain up and bumping up the device's usage counter.
> > 
> > What will driver will see if it tries to check pm_runtime_active()?
> > Would not it get unexpected result if the driver did not call
> > pm_runtime_enable() on it's device?
> 
> Well, that part was supposed to depend on the bus type.  For example, it
> won't be unexpected for PCI drivers, because runtime PM is always enabled
> for PCI devices (although it may be blocked as I described).
> 
> A problem arises if a power domain is used along with a bus type that doesn't
> enable runtime PM for devices by default and drivers are expected to do that.
> That could be addressed by powering up a PM domain (and bumping up its usage
> counter) when adding a device to it and keeping it up until all of the devices
> in it are runtime suspended, but then it also should be turned off eventually
> if there are no drivers for any of those devices.

In other words, I agree with the Ulf's approach in the $subject patch, although
the changelog needs to be updated.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[...]

>> > > > >
>> > > > > It would not be the same for all buses.  Each bus will have its own way
>> > > > > of recognizing whether or not a driver has been probed (i.e., by
>> > > > > checking some field in the bus-specific part of the device structure).
>> > > > >
>> > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that
>> > > > > > didn't issue pm_runtime_enable()?
>> > > > >
>> > > > > Yes.  But the bus has to issue pm_runtime_enable() before probing the
>> > > > > driver, because the driver will expect runtime PM to work properly
>> > > > > while its probe routine runs.  For example, the probe routine might
>> > > > > want to leave the device in a runtime-suspended state.  It can't do
>> > > > > that if the device isn't enabled for runtime PM.
>> > > >
>> > > > That means that runtime PM will be enabled for all devices on given bus
>> > > > while up till now drivers were deciding if their devices should be
>> > > > runtime-pm-managed or not.
>> > >
>> > > That's not the case for PCI drivers.
>> > >
>> > > > I do not think we are quite ready for this.
>> > >
>> > > We have to do that if power domains are in use, however, because if at least
>> > > one device in a power domain in enabled for runtime PM, that will affect the
>> > > other devices in that domain.
>> > >
>> > > We could make a rule to keep a domail always up if at least one device in it
>> > > has runtime PM disabled, but that is equivalent to enabling runtime PM for
>> > > that device, powering the domain up and bumping up the device's usage counter.
>> >
>> > What will driver will see if it tries to check pm_runtime_active()?
>> > Would not it get unexpected result if the driver did not call
>> > pm_runtime_enable() on it's device?
>>
>> Well, that part was supposed to depend on the bus type.  For example, it
>> won't be unexpected for PCI drivers, because runtime PM is always enabled
>> for PCI devices (although it may be blocked as I described).
>>
>> A problem arises if a power domain is used along with a bus type that doesn't
>> enable runtime PM for devices by default and drivers are expected to do that.
>> That could be addressed by powering up a PM domain (and bumping up its usage
>> counter) when adding a device to it and keeping it up until all of the devices
>> in it are runtime suspended, but then it also should be turned off eventually
>> if there are no drivers for any of those devices.
>
> In other words, I agree with the Ulf's approach in the $subject patch, although
> the changelog needs to be updated.
>

I am having second thoughts here! The reason to why, are because of
the scenario 5) below, which I forgot about when writing/testing
$subject patch.

As you know, I have been thinking over and over about how to address
the issues $subject patch is trying to solve.

From the discussions we have had around this topic, I would like to
summarize the situation describing the current existing scenarios that
I know about. It would be nice to reach a consensus on how to cope
with them, especially since we keep forgetting to consider at least
one of them during our discussions.

Note that some minor variations may exist for each scenario, but let's
focus on the concepts of how runtime PM is managed during ->probe().
The scenarios applies to those drivers/buses for which devices may
have an attached generic PM domain.

Scenario 1), a platform driver without runtime PM callbacks.
->probe()
- do some initialization
- pm_runtime_enable()
- pm_runtime_get_sync()
- probe the device
- pm_runtime_put()

Scenario 2), platform driver with runtime PM callbacks.
->probe()
- do some initialization
- fetch handles to runtime PM resources
- pm_runtime_enable()
- pm_runtime_get_sync()
- enable its runtime PM resources, in some cases it's done by also
requiring its runtime PM resume callback to be invoked
- probe the device
- pm_runtime_put()

Scenario 3), platform driver with runtime PM callbacks.
->probe()
- do some initialization
- fetch handles to runtime PM resources
- enable its runtime PM resources
- pm_runtime_get_noresume()
- pm_runtime_set_active()
- pm_runtime_enable()
- probe the device
- pm_runtime_put()

Scenario 4), amba bus and amba driver, both have runtime PM callbacks.
->amba bus probe()
- fetch handles to runtime PM resources
- enable its runtime PM resources
- pm_runtime_get_noresume()
- pm_runtime_set_active()
- pm_runtime_enable()
  ->amba driver probe()
  - do some initialization
  - fetch handles to runtime PM resources
  - enable its runtime PM resources
  - probe the device
  - pm_runtime_put()

Scenario 5), a platform driver with/without runtime PM callbacks.
->probe()
- do some initialization
- may fetch handles to runtime PM resources
- pm_runtime_enable()


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.


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.

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

Option 3)
Live we the limitation this $subject patch introduces for scenario 5).


Is there maybe any additional options, that I didn't think of?

Apologize for the long email, I hope I didn't bored you too much.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 17 Nov 2014, Dmitry Torokhov wrote:

> > > However, is it allowed to call pm_runtime_get_sync() on devices that
> > > didn't issue pm_runtime_enable()?
> > 
> > Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> > driver, because the driver will expect runtime PM to work properly 
> > while its probe routine runs.  For example, the probe routine might 
> > want to leave the device in a runtime-suspended state.  It can't do 
> > that if the device isn't enabled for runtime PM.
> 
> That means that runtime PM will be enabled for all devices on given bus
> while up till now drivers were deciding if their devices should be
> runtime-pm-managed or not. I do not think we are quite ready for this.

It's up to both the bus _and_ the driver to make this decision.

If a driver is completely runtime-PM-unaware then it will never
decrement the device's usage counter (which was incremented when the
bus called _get_noresume()), and therefore the device will never be
runtime-suspended.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 18, 2014 at 11:13:28AM -0500, Alan Stern wrote:
> On Mon, 17 Nov 2014, Dmitry Torokhov wrote:
> 
> > > > However, is it allowed to call pm_runtime_get_sync() on devices that
> > > > didn't issue pm_runtime_enable()?
> > > 
> > > Yes.  But the bus has to issue pm_runtime_enable() before probing the 
> > > driver, because the driver will expect runtime PM to work properly 
> > > while its probe routine runs.  For example, the probe routine might 
> > > want to leave the device in a runtime-suspended state.  It can't do 
> > > that if the device isn't enabled for runtime PM.
> > 
> > That means that runtime PM will be enabled for all devices on given bus
> > while up till now drivers were deciding if their devices should be
> > runtime-pm-managed or not. I do not think we are quite ready for this.
> 
> It's up to both the bus _and_ the driver to make this decision.
> 
> If a driver is completely runtime-PM-unaware then it will never
> decrement the device's usage counter (which was incremented when the
> bus called _get_noresume()), and therefore the device will never be
> runtime-suspended.

OK. Another question then: pm_runtime_get_noresume() does literally this:

	atomic_inc(&dev->power.usage_count);

So who is responsible for actually waking up parent device and/or power
domain? Is it simply missing because we did not really have PM domains
before?

Thanks.

On Tue, 18 Nov 2014, Dmitry Torokhov wrote:

> OK. Another question then: pm_runtime_get_noresume() does literally this:
> 
> 	atomic_inc(&dev->power.usage_count);
> 
> So who is responsible for actually waking up parent device and/or power
> domain? Is it simply missing because we did not really have PM domains
> before?

Ths bus is responsible for making sure that all the standard resources
are available -- that is, all the resources that would be needed by a
normal device on that bus.  Anything beyond that (such as
special-purpose clocks) has to be set up by the driver.

Thus the bus would insure that the device was powered, its parent was
resumed, and the usual clock inputs were enabled.  And of course, one
mechanism for doing this is to runtime-resume the power domain.

Often the bus doesn't have to do anything special to resume the
device's parent.  This is because the device gets probed when it is
registered, which happens when it is discovered, and the discovery is
done by the parent's driver as part of its normal activity.  Since the
parent has to be powered up to carry out this normal activity, nothing 
more is needed.

(Of course, devices can get probed at other times too, not just when
they are discovered.  For example, a device might get probed when a
driver module is loaded, and that can occur at any time.  So in general
it might indeed be necessary for the bus to wake up the parent before
calling the driver's probe routine.)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> 
> > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > 
> > 	atomic_inc(&dev->power.usage_count);
> > 
> > So who is responsible for actually waking up parent device and/or power
> > domain? Is it simply missing because we did not really have PM domains
> > before?
> 
> Ths bus is responsible for making sure that all the standard resources
> are available -- that is, all the resources that would be needed by a
> normal device on that bus.  Anything beyond that (such as
> special-purpose clocks) has to be set up by the driver.
> 
> Thus the bus would insure that the device was powered, its parent was
> resumed, and the usual clock inputs were enabled.  And of course, one
> mechanism for doing this is to runtime-resume the power domain.

This does not sound like anything bus-specific. Can we move powering on
the domain before probing into the driver core, similarly to the default
pin selection by pinctrl? I do not see why we want to have every
individual bus to implement this. I guess right now we can't because we
assign the domain to device in individual bus's probe function, but I do
not think this is proper place for that either: bus and pm domain are
orthogonal concepts IMO.

> 
> Often the bus doesn't have to do anything special to resume the
> device's parent.  This is because the device gets probed when it is
> registered, which happens when it is discovered, and the discovery is
> done by the parent's driver as part of its normal activity.  Since the
> parent has to be powered up to carry out this normal activity, nothing 
> more is needed.

I think this only true for buses that support discovery.

> 
> (Of course, devices can get probed at other times too, not just when
> they are discovered.  For example, a device might get probed when a
> driver module is loaded, and that can occur at any time.  So in general
> it might indeed be necessary for the bus to wake up the parent before
> calling the driver's probe routine.)
>

Thanks.

On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > 
> > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > 
> > > > 	atomic_inc(&dev->power.usage_count);
> > > > 
> > > > So who is responsible for actually waking up parent device and/or power
> > > > domain? Is it simply missing because we did not really have PM domains
> > > > before?
> > > 
> > > Ths bus is responsible for making sure that all the standard resources
> > > are available -- that is, all the resources that would be needed by a
> > > normal device on that bus.  Anything beyond that (such as
> > > special-purpose clocks) has to be set up by the driver.
> > > 
> > > Thus the bus would insure that the device was powered, its parent was
> > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > mechanism for doing this is to runtime-resume the power domain.
> > 
> > This does not sound like anything bus-specific. Can we move powering on
> > the domain before probing into the driver core, similarly to the default
> > pin selection by pinctrl?
> 
> We could do that for genpd if devices were added to domains before registering
> (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> 
> Note that this would only be the case for genpd, not for the ACPI PM domain
> in particular, for example.  The reason why is that the ACPI PM domain cannot
> be used along with bus types that provide non-trivial PM callbacks, so pretty
> much the bus type's ->probe needs to decide whether or not to use it.

In genpd code there is a notion of providers that match devices and
domains. Can we do the same for ACPI and stuff all that knowledge into
it's "provider"?

IOW why ACPI is that special?

On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > 
> > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > 
> > > 	atomic_inc(&dev->power.usage_count);
> > > 
> > > So who is responsible for actually waking up parent device and/or power
> > > domain? Is it simply missing because we did not really have PM domains
> > > before?
> > 
> > Ths bus is responsible for making sure that all the standard resources
> > are available -- that is, all the resources that would be needed by a
> > normal device on that bus.  Anything beyond that (such as
> > special-purpose clocks) has to be set up by the driver.
> > 
> > Thus the bus would insure that the device was powered, its parent was
> > resumed, and the usual clock inputs were enabled.  And of course, one
> > mechanism for doing this is to runtime-resume the power domain.
> 
> This does not sound like anything bus-specific. Can we move powering on
> the domain before probing into the driver core, similarly to the default
> pin selection by pinctrl?

We could do that for genpd if devices were added to domains before registering
(those devices).  Otherwise, there's no guarantee that all has been set up yet.

Note that this would only be the case for genpd, not for the ACPI PM domain
in particular, for example.  The reason why is that the ACPI PM domain cannot
be used along with bus types that provide non-trivial PM callbacks, so pretty
much the bus type's ->probe needs to decide whether or not to use it.

> I do not see why we want to have every
> individual bus to implement this. I guess right now we can't because we
> assign the domain to device in individual bus's probe function, but I do
> not think this is proper place for that either: bus and pm domain are
> orthogonal concepts IMO.

For genpd, yes.  Not necessarily for other types of PM domains.  [A "PM domain"
is just an override for bus type PM callbacks in general, genpd is only one
use case here.]

I think that the the general DT bindings for genpd are the source of the whole
complication here.  Before, devices were added to domains in the board-specific
code before registration, but now that can be done automatically if the
appropriate bindings are present in the DT.

> > Often the bus doesn't have to do anything special to resume the
> > device's parent.  This is because the device gets probed when it is
> > registered, which happens when it is discovered, and the discovery is
> > done by the parent's driver as part of its normal activity.  Since the
> > parent has to be powered up to carry out this normal activity, nothing 
> > more is needed.
> 
> I think this only true for buses that support discovery.

It may not be the case for the platform bus type or other "root kind" bus
types where devices may not have a functional parent.  Otherwise it usually
is the case.

Anyway, that really depends on the bus type.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 18, 2014 03:05:08 PM Ulf Hansson wrote:
> [...]
> 
> >> > > > >
> >> > > > > It would not be the same for all buses.  Each bus will have its own way
> >> > > > > of recognizing whether or not a driver has been probed (i.e., by
> >> > > > > checking some field in the bus-specific part of the device structure).
> >> > > > >
> >> > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that
> >> > > > > > didn't issue pm_runtime_enable()?
> >> > > > >
> >> > > > > Yes.  But the bus has to issue pm_runtime_enable() before probing the
> >> > > > > driver, because the driver will expect runtime PM to work properly
> >> > > > > while its probe routine runs.  For example, the probe routine might
> >> > > > > want to leave the device in a runtime-suspended state.  It can't do
> >> > > > > that if the device isn't enabled for runtime PM.
> >> > > >
> >> > > > That means that runtime PM will be enabled for all devices on given bus
> >> > > > while up till now drivers were deciding if their devices should be
> >> > > > runtime-pm-managed or not.
> >> > >
> >> > > That's not the case for PCI drivers.
> >> > >
> >> > > > I do not think we are quite ready for this.
> >> > >
> >> > > We have to do that if power domains are in use, however, because if at least
> >> > > one device in a power domain in enabled for runtime PM, that will affect the
> >> > > other devices in that domain.
> >> > >
> >> > > We could make a rule to keep a domail always up if at least one device in it
> >> > > has runtime PM disabled, but that is equivalent to enabling runtime PM for
> >> > > that device, powering the domain up and bumping up the device's usage counter.
> >> >
> >> > What will driver will see if it tries to check pm_runtime_active()?
> >> > Would not it get unexpected result if the driver did not call
> >> > pm_runtime_enable() on it's device?
> >>
> >> Well, that part was supposed to depend on the bus type.  For example, it
> >> won't be unexpected for PCI drivers, because runtime PM is always enabled
> >> for PCI devices (although it may be blocked as I described).
> >>
> >> A problem arises if a power domain is used along with a bus type that doesn't
> >> enable runtime PM for devices by default and drivers are expected to do that.
> >> That could be addressed by powering up a PM domain (and bumping up its usage
> >> counter) when adding a device to it and keeping it up until all of the devices
> >> in it are runtime suspended, but then it also should be turned off eventually
> >> if there are no drivers for any of those devices.
> >
> > In other words, I agree with the Ulf's approach in the $subject patch, although
> > the changelog needs to be updated.
> >
> 
> I am having second thoughts here! The reason to why, are because of
> the scenario 5) below, which I forgot about when writing/testing
> $subject patch.
> 
> As you know, I have been thinking over and over about how to address
> the issues $subject patch is trying to solve.
> 
> From the discussions we have had around this topic, I would like to
> summarize the situation describing the current existing scenarios that
> I know about. It would be nice to reach a consensus on how to cope
> with them, especially since we keep forgetting to consider at least
> one of them during our discussions.
> 
> Note that some minor variations may exist for each scenario, but let's
> focus on the concepts of how runtime PM is managed during ->probe().
> The scenarios applies to those drivers/buses for which devices may
> have an attached generic PM domain.
> 
> Scenario 1), a platform driver without runtime PM callbacks.
> ->probe()
> - do some initialization
> - pm_runtime_enable()
> - pm_runtime_get_sync()
> - probe the device
> - pm_runtime_put()
> 
> Scenario 2), platform driver with runtime PM callbacks.
> ->probe()
> - do some initialization
> - fetch handles to runtime PM resources
> - pm_runtime_enable()
> - pm_runtime_get_sync()
> - enable its runtime PM resources, in some cases it's done by also
> requiring its runtime PM resume callback to be invoked
> - probe the device
> - pm_runtime_put()
> 
> Scenario 3), platform driver with runtime PM callbacks.
> ->probe()
> - do some initialization
> - fetch handles to runtime PM resources
> - enable its runtime PM resources
> - pm_runtime_get_noresume()
> - pm_runtime_set_active()
> - pm_runtime_enable()
> - probe the device
> - pm_runtime_put()
> 
> Scenario 4), amba bus and amba driver, both have runtime PM callbacks.
> ->amba bus probe()
> - fetch handles to runtime PM resources
> - enable its runtime PM resources
> - pm_runtime_get_noresume()
> - pm_runtime_set_active()
> - pm_runtime_enable()
>   ->amba driver probe()
>   - do some initialization
>   - fetch handles to runtime PM resources
>   - enable its runtime PM resources
>   - probe the device
>   - pm_runtime_put()
> 
> 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?

> 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?

> 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.

> Option 3)
> Live we the limitation this $subject patch introduces for scenario 5).

I'd say, 3) for now and 1) going forward.

> Is there maybe any additional options, that I didn't think of?
> 
> Apologize for the long email, I hope I didn't bored you too much.

That's fine, thanks for taking the time to lay out all of the cases, that's
always helpful.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote:
> > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote:
> > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > > > > 
> > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > > > > 
> > > > > > > 	atomic_inc(&dev->power.usage_count);
> > > > > > > 
> > > > > > > So who is responsible for actually waking up parent device and/or power
> > > > > > > domain? Is it simply missing because we did not really have PM domains
> > > > > > > before?
> > > > > > 
> > > > > > Ths bus is responsible for making sure that all the standard resources
> > > > > > are available -- that is, all the resources that would be needed by a
> > > > > > normal device on that bus.  Anything beyond that (such as
> > > > > > special-purpose clocks) has to be set up by the driver.
> > > > > > 
> > > > > > Thus the bus would insure that the device was powered, its parent was
> > > > > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > > > > mechanism for doing this is to runtime-resume the power domain.
> > > > > 
> > > > > This does not sound like anything bus-specific. Can we move powering on
> > > > > the domain before probing into the driver core, similarly to the default
> > > > > pin selection by pinctrl?
> > > > 
> > > > We could do that for genpd if devices were added to domains before registering
> > > > (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> > > > 
> > > > Note that this would only be the case for genpd, not for the ACPI PM domain
> > > > in particular, for example.  The reason why is that the ACPI PM domain cannot
> > > > be used along with bus types that provide non-trivial PM callbacks, so pretty
> > > > much the bus type's ->probe needs to decide whether or not to use it.
> > > 
> > > In genpd code there is a notion of providers that match devices and
> > > domains. Can we do the same for ACPI and stuff all that knowledge into
> > > it's "provider"?
> > 
> > It is in ACPI like that too, but not in the form of the ACPI PM domain.
> > 
> > > IOW why ACPI is that special?
> > 
> > The ACPI PM domain is there specifically for bus types that don't provide
> > non-trivial PM callbacks to avoid duplication of code (if it didn't exist,
> > all of the bus types in question would need to provide callbacks with
> > optional ACPI handling in them).  That's all about it.
> > 
> > And there are bus types that provide non-trivial PM callbacks *and* use
> > ACPI in them, like PCI, and that is more interleaved with the native PM
> > in there.  For those bus types we can't add devices to the ACPI PM domain
> > just because they have ACPI companion objects.
> > 
> > I'm not really sure why it is important here, though.  We're talking about
> > genpd, aren't we?
> > 
> > I just wanted to indicate that the PM domains concept is not only about
> > handling power domains and not all of its use cases can be shoehorned into
> > the same scheme.
> 
> And by the way, things worked just fine for the ACPI PM domain before commit
> 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device)
> which put the ACPI PM domain and genpd into one bag, which was a mistake,
> because they are different things.
>

Can we maybe settle on the naming then so that we do not mix them up in
the future? For me PM domain is group of devices that share certain
power constraints so that they have to be powered up and down together.
Is this definition is not correct (for genpd at least)? And what is the
proper definition for ACPI PM domain?

Thanks.

On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote:
> On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > > 
> > > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > > 
> > > > > 	atomic_inc(&dev->power.usage_count);
> > > > > 
> > > > > So who is responsible for actually waking up parent device and/or power
> > > > > domain? Is it simply missing because we did not really have PM domains
> > > > > before?
> > > > 
> > > > Ths bus is responsible for making sure that all the standard resources
> > > > are available -- that is, all the resources that would be needed by a
> > > > normal device on that bus.  Anything beyond that (such as
> > > > special-purpose clocks) has to be set up by the driver.
> > > > 
> > > > Thus the bus would insure that the device was powered, its parent was
> > > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > > mechanism for doing this is to runtime-resume the power domain.
> > > 
> > > This does not sound like anything bus-specific. Can we move powering on
> > > the domain before probing into the driver core, similarly to the default
> > > pin selection by pinctrl?
> > 
> > We could do that for genpd if devices were added to domains before registering
> > (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> > 
> > Note that this would only be the case for genpd, not for the ACPI PM domain
> > in particular, for example.  The reason why is that the ACPI PM domain cannot
> > be used along with bus types that provide non-trivial PM callbacks, so pretty
> > much the bus type's ->probe needs to decide whether or not to use it.
> 
> In genpd code there is a notion of providers that match devices and
> domains. Can we do the same for ACPI and stuff all that knowledge into
> it's "provider"?

It is in ACPI like that too, but not in the form of the ACPI PM domain.

> IOW why ACPI is that special?

The ACPI PM domain is there specifically for bus types that don't provide
non-trivial PM callbacks to avoid duplication of code (if it didn't exist,
all of the bus types in question would need to provide callbacks with
optional ACPI handling in them).  That's all about it.

And there are bus types that provide non-trivial PM callbacks *and* use
ACPI in them, like PCI, and that is more interleaved with the native PM
in there.  For those bus types we can't add devices to the ACPI PM domain
just because they have ACPI companion objects.

I'm not really sure why it is important here, though.  We're talking about
genpd, aren't we?

I just wanted to indicate that the PM domains concept is not only about
handling power domains and not all of its use cases can be shoehorned into
the same scheme.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote:
> On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote:
> > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > > > 
> > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > > > 
> > > > > > 	atomic_inc(&dev->power.usage_count);
> > > > > > 
> > > > > > So who is responsible for actually waking up parent device and/or power
> > > > > > domain? Is it simply missing because we did not really have PM domains
> > > > > > before?
> > > > > 
> > > > > Ths bus is responsible for making sure that all the standard resources
> > > > > are available -- that is, all the resources that would be needed by a
> > > > > normal device on that bus.  Anything beyond that (such as
> > > > > special-purpose clocks) has to be set up by the driver.
> > > > > 
> > > > > Thus the bus would insure that the device was powered, its parent was
> > > > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > > > mechanism for doing this is to runtime-resume the power domain.
> > > > 
> > > > This does not sound like anything bus-specific. Can we move powering on
> > > > the domain before probing into the driver core, similarly to the default
> > > > pin selection by pinctrl?
> > > 
> > > We could do that for genpd if devices were added to domains before registering
> > > (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> > > 
> > > Note that this would only be the case for genpd, not for the ACPI PM domain
> > > in particular, for example.  The reason why is that the ACPI PM domain cannot
> > > be used along with bus types that provide non-trivial PM callbacks, so pretty
> > > much the bus type's ->probe needs to decide whether or not to use it.
> > 
> > In genpd code there is a notion of providers that match devices and
> > domains. Can we do the same for ACPI and stuff all that knowledge into
> > it's "provider"?
> 
> It is in ACPI like that too, but not in the form of the ACPI PM domain.
> 
> > IOW why ACPI is that special?
> 
> The ACPI PM domain is there specifically for bus types that don't provide
> non-trivial PM callbacks to avoid duplication of code (if it didn't exist,
> all of the bus types in question would need to provide callbacks with
> optional ACPI handling in them).  That's all about it.
> 
> And there are bus types that provide non-trivial PM callbacks *and* use
> ACPI in them, like PCI, and that is more interleaved with the native PM
> in there.  For those bus types we can't add devices to the ACPI PM domain
> just because they have ACPI companion objects.
> 
> I'm not really sure why it is important here, though.  We're talking about
> genpd, aren't we?
> 
> I just wanted to indicate that the PM domains concept is not only about
> handling power domains and not all of its use cases can be shoehorned into
> the same scheme.

And by the way, things worked just fine for the ACPI PM domain before commit
46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device)
which put the ACPI PM domain and genpd into one bag, which was a mistake,
because they are different things.

Moreover, commit 207f1a2d294e (amba: Add support for attach/detach of PM
domains) is outright buggy, because the AMBA bus type provides its own
non-trivial PM callbacks, so using the ACPI PM domain with it is not
correct.

Grumble.

It looks like we need to untangle these things again and start over from square
one.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 18, 2014 at 10:58:17PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 18, 2014 01:02:29 PM Dmitry Torokhov wrote:
> > On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote:
> > > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote:
> > > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > > > > > > 
> > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > > > > > > 
> > > > > > > > > 	atomic_inc(&dev->power.usage_count);
> > > > > > > > > 
> > > > > > > > > So who is responsible for actually waking up parent device and/or power
> > > > > > > > > domain? Is it simply missing because we did not really have PM domains
> > > > > > > > > before?
> > > > > > > > 
> > > > > > > > Ths bus is responsible for making sure that all the standard resources
> > > > > > > > are available -- that is, all the resources that would be needed by a
> > > > > > > > normal device on that bus.  Anything beyond that (such as
> > > > > > > > special-purpose clocks) has to be set up by the driver.
> > > > > > > > 
> > > > > > > > Thus the bus would insure that the device was powered, its parent was
> > > > > > > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > > > > > > mechanism for doing this is to runtime-resume the power domain.
> > > > > > > 
> > > > > > > This does not sound like anything bus-specific. Can we move powering on
> > > > > > > the domain before probing into the driver core, similarly to the default
> > > > > > > pin selection by pinctrl?
> > > > > > 
> > > > > > We could do that for genpd if devices were added to domains before registering
> > > > > > (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> > > > > > 
> > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain
> > > > > > in particular, for example.  The reason why is that the ACPI PM domain cannot
> > > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty
> > > > > > much the bus type's ->probe needs to decide whether or not to use it.
> > > > > 
> > > > > In genpd code there is a notion of providers that match devices and
> > > > > domains. Can we do the same for ACPI and stuff all that knowledge into
> > > > > it's "provider"?
> > > > 
> > > > It is in ACPI like that too, but not in the form of the ACPI PM domain.
> > > > 
> > > > > IOW why ACPI is that special?
> > > > 
> > > > The ACPI PM domain is there specifically for bus types that don't provide
> > > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist,
> > > > all of the bus types in question would need to provide callbacks with
> > > > optional ACPI handling in them).  That's all about it.
> > > > 
> > > > And there are bus types that provide non-trivial PM callbacks *and* use
> > > > ACPI in them, like PCI, and that is more interleaved with the native PM
> > > > in there.  For those bus types we can't add devices to the ACPI PM domain
> > > > just because they have ACPI companion objects.
> > > > 
> > > > I'm not really sure why it is important here, though.  We're talking about
> > > > genpd, aren't we?
> > > > 
> > > > I just wanted to indicate that the PM domains concept is not only about
> > > > handling power domains and not all of its use cases can be shoehorned into
> > > > the same scheme.
> > > 
> > > And by the way, things worked just fine for the ACPI PM domain before commit
> > > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device)
> > > which put the ACPI PM domain and genpd into one bag, which was a mistake,
> > > because they are different things.
> > >
> > 
> > Can we maybe settle on the naming then so that we do not mix them up in
> > the future? For me PM domain is group of devices that share certain
> > power constraints so that they have to be powered up and down together.
> > Is this definition is not correct (for genpd at least)?
> 
> It is correct for genpd, it isn't correct for the ACPI PM domain.
> 
> > And what is the proper definition for ACPI PM domain?
> 
> I agree that the terminology is (somewhat?) confusing.
> 
> From the code perspective, using a PM domain object is a way to provide PM
> callbacks that will be executed for a subset of devices instead of or in
> addition to the bus type (or class or device type) callbacks.  Of course,
> that applies to proper power domains in particular, but it can also apply
> to broader sets of devices.  In the ACPI PM domain case this covers devices
> with ACPI power management support (or more precisely, devices with ACPI
> companion objects that can provide PM support).  In this context the word
> "domain" means as much as "area of control" (which is a proper dictionary
> definition of it AFAICS).
> 
> genpd is all about proper power domains, like you said.

OK, thank you for explaining this. Up until now I was blissfully
oblivious to the majority of PM intricacies :)

On Tuesday, November 18, 2014 01:02:29 PM Dmitry Torokhov wrote:
> On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote:
> > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote:
> > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > > > > > 
> > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > > > > > 
> > > > > > > > 	atomic_inc(&dev->power.usage_count);
> > > > > > > > 
> > > > > > > > So who is responsible for actually waking up parent device and/or power
> > > > > > > > domain? Is it simply missing because we did not really have PM domains
> > > > > > > > before?
> > > > > > > 
> > > > > > > Ths bus is responsible for making sure that all the standard resources
> > > > > > > are available -- that is, all the resources that would be needed by a
> > > > > > > normal device on that bus.  Anything beyond that (such as
> > > > > > > special-purpose clocks) has to be set up by the driver.
> > > > > > > 
> > > > > > > Thus the bus would insure that the device was powered, its parent was
> > > > > > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > > > > > mechanism for doing this is to runtime-resume the power domain.
> > > > > > 
> > > > > > This does not sound like anything bus-specific. Can we move powering on
> > > > > > the domain before probing into the driver core, similarly to the default
> > > > > > pin selection by pinctrl?
> > > > > 
> > > > > We could do that for genpd if devices were added to domains before registering
> > > > > (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> > > > > 
> > > > > Note that this would only be the case for genpd, not for the ACPI PM domain
> > > > > in particular, for example.  The reason why is that the ACPI PM domain cannot
> > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty
> > > > > much the bus type's ->probe needs to decide whether or not to use it.
> > > > 
> > > > In genpd code there is a notion of providers that match devices and
> > > > domains. Can we do the same for ACPI and stuff all that knowledge into
> > > > it's "provider"?
> > > 
> > > It is in ACPI like that too, but not in the form of the ACPI PM domain.
> > > 
> > > > IOW why ACPI is that special?
> > > 
> > > The ACPI PM domain is there specifically for bus types that don't provide
> > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist,
> > > all of the bus types in question would need to provide callbacks with
> > > optional ACPI handling in them).  That's all about it.
> > > 
> > > And there are bus types that provide non-trivial PM callbacks *and* use
> > > ACPI in them, like PCI, and that is more interleaved with the native PM
> > > in there.  For those bus types we can't add devices to the ACPI PM domain
> > > just because they have ACPI companion objects.
> > > 
> > > I'm not really sure why it is important here, though.  We're talking about
> > > genpd, aren't we?
> > > 
> > > I just wanted to indicate that the PM domains concept is not only about
> > > handling power domains and not all of its use cases can be shoehorned into
> > > the same scheme.
> > 
> > And by the way, things worked just fine for the ACPI PM domain before commit
> > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device)
> > which put the ACPI PM domain and genpd into one bag, which was a mistake,
> > because they are different things.
> >
> 
> Can we maybe settle on the naming then so that we do not mix them up in
> the future? For me PM domain is group of devices that share certain
> power constraints so that they have to be powered up and down together.
> Is this definition is not correct (for genpd at least)?

It is correct for genpd, it isn't correct for the ACPI PM domain.

> And what is the proper definition for ACPI PM domain?

I agree that the terminology is (somewhat?) confusing.

From the code perspective, using a PM domain object is a way to provide PM
callbacks that will be executed for a subset of devices instead of or in
addition to the bus type (or class or device type) callbacks.  Of course,
that applies to proper power domains in particular, but it can also apply
to broader sets of devices.  In the ACPI PM domain case this covers devices
with ACPI power management support (or more precisely, devices with ACPI
companion objects that can provide PM support).  In this context the word
"domain" means as much as "area of control" (which is a proper dictionary
definition of it AFAICS).

genpd is all about proper power domains, like you said.

That's why I'm saying that they are different and clamping them both
together was a mistake.  I overlooked that, my bad.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 18, 2014 10:58:17 PM Rafael J. Wysocki wrote:
> On Tuesday, November 18, 2014 01:02:29 PM Dmitry Torokhov wrote:
> > On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote:
> > > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote:
> > > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote:
> > > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote:
> > > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote:
> > > > > > > > 
> > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this:
> > > > > > > > > 
> > > > > > > > > 	atomic_inc(&dev->power.usage_count);
> > > > > > > > > 
> > > > > > > > > So who is responsible for actually waking up parent device and/or power
> > > > > > > > > domain? Is it simply missing because we did not really have PM domains
> > > > > > > > > before?
> > > > > > > > 
> > > > > > > > Ths bus is responsible for making sure that all the standard resources
> > > > > > > > are available -- that is, all the resources that would be needed by a
> > > > > > > > normal device on that bus.  Anything beyond that (such as
> > > > > > > > special-purpose clocks) has to be set up by the driver.
> > > > > > > > 
> > > > > > > > Thus the bus would insure that the device was powered, its parent was
> > > > > > > > resumed, and the usual clock inputs were enabled.  And of course, one
> > > > > > > > mechanism for doing this is to runtime-resume the power domain.
> > > > > > > 
> > > > > > > This does not sound like anything bus-specific. Can we move powering on
> > > > > > > the domain before probing into the driver core, similarly to the default
> > > > > > > pin selection by pinctrl?
> > > > > > 
> > > > > > We could do that for genpd if devices were added to domains before registering
> > > > > > (those devices).  Otherwise, there's no guarantee that all has been set up yet.
> > > > > > 
> > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain
> > > > > > in particular, for example.  The reason why is that the ACPI PM domain cannot
> > > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty
> > > > > > much the bus type's ->probe needs to decide whether or not to use it.
> > > > > 
> > > > > In genpd code there is a notion of providers that match devices and
> > > > > domains. Can we do the same for ACPI and stuff all that knowledge into
> > > > > it's "provider"?
> > > > 
> > > > It is in ACPI like that too, but not in the form of the ACPI PM domain.
> > > > 
> > > > > IOW why ACPI is that special?
> > > > 
> > > > The ACPI PM domain is there specifically for bus types that don't provide
> > > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist,
> > > > all of the bus types in question would need to provide callbacks with
> > > > optional ACPI handling in them).  That's all about it.
> > > > 
> > > > And there are bus types that provide non-trivial PM callbacks *and* use
> > > > ACPI in them, like PCI, and that is more interleaved with the native PM
> > > > in there.  For those bus types we can't add devices to the ACPI PM domain
> > > > just because they have ACPI companion objects.
> > > > 
> > > > I'm not really sure why it is important here, though.  We're talking about
> > > > genpd, aren't we?
> > > > 
> > > > I just wanted to indicate that the PM domains concept is not only about
> > > > handling power domains and not all of its use cases can be shoehorned into
> > > > the same scheme.
> > > 
> > > And by the way, things worked just fine for the ACPI PM domain before commit
> > > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device)
> > > which put the ACPI PM domain and genpd into one bag, which was a mistake,
> > > because they are different things.
> > >
> > 
> > Can we maybe settle on the naming then so that we do not mix them up in
> > the future? For me PM domain is group of devices that share certain
> > power constraints so that they have to be powered up and down together.
> > Is this definition is not correct (for genpd at least)?
> 
> It is correct for genpd, it isn't correct for the ACPI PM domain.
> 
> > And what is the proper definition for ACPI PM domain?
> 
> I agree that the terminology is (somewhat?) confusing.
> 
> From the code perspective, using a PM domain object is a way to provide PM
> callbacks that will be executed for a subset of devices instead of or in
> addition to the bus type (or class or device type) callbacks.  Of course,
> that applies to proper power domains in particular, but it can also apply
> to broader sets of devices.  In the ACPI PM domain case this covers devices
> with ACPI power management support (or more precisely, devices with ACPI
> companion objects that can provide PM support).  In this context the word
> "domain" means as much as "area of control" (which is a proper dictionary
> definition of it AFAICS).
> 
> genpd is all about proper power domains, like you said.
> 
> That's why I'm saying that they are different and clamping them both
> together was a mistake.  I overlooked that, my bad.

For completeness, please let me quote the changelog of commit
564b905ab10d (PM / Domains: Rename struct dev_power_domain to struct dev_pm_domain)
dealing with this particular thing:

    The naming convention used by commit 7538e3db6e015e890825fbd9f86599b
    (PM: Add support for device power domains), which introduced the
    struct dev_power_domain type for representing device power domains,
    evidently confuses some developers who tend to think that objects
    of this type must correspond to "power domains" as defined by
    hardware, which is not the case.  Namely, at the kernel level, a
    struct dev_power_domain object can represent arbitrary set of devices
    that are mutually dependent power management-wise and need not belong
    to one hardware power domain.  To avoid that confusion, rename struct
    dev_power_domain to struct dev_pm_domain and rename the related
    pointers in struct device and struct pm_clk_notifier_block from
    pwr_domain to pm_domain.

And that should be explicitly explained in the PM documentation which I'm going
to do.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[...]

>>
>> 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.

>
>> 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.

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().
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.

>
>> 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.

>
>> 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).

>
>> Is there maybe any additional options, that I didn't think of?
>>
>> Apologize for the long email, I hope I didn't bored you too much.
>
> That's fine, thanks for taking the time to lay out all of the cases, that's
> always helpful.
>
> Rafael
>

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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.

> 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?

> >> 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.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 20 November 2014 01:35, Rafael J. Wysocki <rjw@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?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday, November 20, 2014 11:13:01 AM Ulf Hansson wrote:
> On 20 November 2014 01:35, Rafael J. Wysocki <rjw@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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html