[PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Jyri Sarha]

Do not lock parent of irq safe device to enabled state if the parent
is also irq safe.

Before this patch the pm_runtime_irq_safe() always called
pm_runtime_get_sync() for the parent, locking the parent to enabled
state and for sure making any parent irq_safe. This is hardly optimal
if the parent device PM code is also irq_safe.

After this patch the PM runtime core synchronously enables any
irq_safe parents of an irq_safe device when pm_runtime_get_sync() is
called. The parents are checked asyncronously after pm_runtime_put()
call.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/base/power/runtime.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 67c7938..800ca1e 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -541,7 +541,8 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	}
 
 	/* Maybe the parent is now able to suspend. */
-	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
+	if (parent && !parent->power.ignore_children && 
+	    (!dev->power.irq_safe || parent->power.irq_safe)) {
 		spin_unlock(&dev->power.lock);
 
 		spin_lock(&parent->power.lock);
@@ -706,7 +707,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
 		 * parent is permanently resumed.
 		 */
 		parent = dev->parent;
-		if (dev->power.irq_safe)
+		if (dev->power.irq_safe && !parent->power.irq_safe)
 			goto skip_parent;
 		spin_unlock(&dev->power.lock);
 
@@ -755,7 +756,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
 		rpm_idle(dev, RPM_ASYNC);
 
  out:
-	if (parent && !dev->power.irq_safe) {
+	if (parent && (!dev->power.irq_safe || parent->power.irq_safe)) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_runtime_put(parent);
@@ -1269,7 +1270,7 @@ EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
  */
 void pm_runtime_irq_safe(struct device *dev)
 {
-	if (dev->parent)
+	if (dev->parent && !dev->parent->power.irq_safe)
 		pm_runtime_get_sync(dev->parent);
 	spin_lock_irq(&dev->power.lock);
 	dev->power.irq_safe = 1;
@@ -1399,7 +1400,7 @@ void pm_runtime_remove(struct device *dev)
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
 		pm_runtime_set_suspended(dev);
-	if (dev->power.irq_safe && dev->parent)
+	if (dev->power.irq_safe && dev->parent && !dev->parent->power.irq_safe)
 		pm_runtime_put(dev->parent);
 }
 #endif
-- 
1.7.9.5

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Rafael J. Wysocki]

On Tuesday, December 02, 2014 10:19:03 PM Jyri Sarha wrote:
> Do not lock parent of irq safe device to enabled state if the parent
> is also irq safe.
> 
> Before this patch the pm_runtime_irq_safe() always called
> pm_runtime_get_sync() for the parent, locking the parent to enabled
> state and for sure making any parent irq_safe. This is hardly optimal
> if the parent device PM code is also irq_safe.
> 
> After this patch the PM runtime core synchronously enables any
> irq_safe parents of an irq_safe device when pm_runtime_get_sync() is
> called. The parents are checked asyncronously after pm_runtime_put()
> call.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/base/power/runtime.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 67c7938..800ca1e 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -541,7 +541,8 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  	}
>  
>  	/* Maybe the parent is now able to suspend. */
> -	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
> +	if (parent && !parent->power.ignore_children && 
> +	    (!dev->power.irq_safe || parent->power.irq_safe)) {
>  		spin_unlock(&dev->power.lock);
>  
>  		spin_lock(&parent->power.lock);
> @@ -706,7 +707,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
>  		 * parent is permanently resumed.
>  		 */
>  		parent = dev->parent;
> -		if (dev->power.irq_safe)
> +		if (dev->power.irq_safe && !parent->power.irq_safe)

Of course, you haven't read the comment above these two lines, because
otherwise you wouldn't have submitted this patch at all.

I'm not applying it.

>  			goto skip_parent;
>  		spin_unlock(&dev->power.lock);
>  
> @@ -755,7 +756,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
>  		rpm_idle(dev, RPM_ASYNC);
>  
>   out:
> -	if (parent && !dev->power.irq_safe) {
> +	if (parent && (!dev->power.irq_safe || parent->power.irq_safe)) {
>  		spin_unlock_irq(&dev->power.lock);
>  
>  		pm_runtime_put(parent);
> @@ -1269,7 +1270,7 @@ EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
>   */
>  void pm_runtime_irq_safe(struct device *dev)
>  {
> -	if (dev->parent)
> +	if (dev->parent && !dev->parent->power.irq_safe)
>  		pm_runtime_get_sync(dev->parent);
>  	spin_lock_irq(&dev->power.lock);
>  	dev->power.irq_safe = 1;
> @@ -1399,7 +1400,7 @@ void pm_runtime_remove(struct device *dev)
>  	/* Change the status back to 'suspended' to match the initial status. */
>  	if (dev->power.runtime_status == RPM_ACTIVE)
>  		pm_runtime_set_suspended(dev);
> -	if (dev->power.irq_safe && dev->parent)
> +	if (dev->power.irq_safe && dev->parent && !dev->parent->power.irq_safe)
>  		pm_runtime_put(dev->parent);
>  }
>  #endif
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Jyri Sarha]

On 12/02/2014 11:54 PM, Rafael J. Wysocki wrote:
> On Tuesday, December 02, 2014 10:19:03 PM Jyri Sarha wrote:
>> Do not lock parent of irq safe device to enabled state if the parent
>> is also irq safe.
>>
>> Before this patch the pm_runtime_irq_safe() always called
>> pm_runtime_get_sync() for the parent, locking the parent to enabled
>> state and for sure making any parent irq_safe. This is hardly optimal
>> if the parent device PM code is also irq_safe.
>>
>> After this patch the PM runtime core synchronously enables any
>> irq_safe parents of an irq_safe device when pm_runtime_get_sync() is
>> called. The parents are checked asyncronously after pm_runtime_put()
>> call.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>   drivers/base/power/runtime.c |   11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 67c7938..800ca1e 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -541,7 +541,8 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>   	}
>>
>>   	/* Maybe the parent is now able to suspend. */
>> -	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
>> +	if (parent && !parent->power.ignore_children &&
>> +	    (!dev->power.irq_safe || parent->power.irq_safe)) {
>>   		spin_unlock(&dev->power.lock);
>>
>>   		spin_lock(&parent->power.lock);
>> @@ -706,7 +707,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
>>   		 * parent is permanently resumed.
>>   		 */
>>   		parent = dev->parent;
>> -		if (dev->power.irq_safe)
>> +		if (dev->power.irq_safe && !parent->power.irq_safe)
>
> Of course, you haven't read the comment above these two lines, because
> otherwise you wouldn't have submitted this patch at all.
>
> I'm not applying it.

Is there something seriously wrong with the code part too? Or is this 
just a matter of updating the comment (sorry that I missed that)?

If the thing I am doing somehow fundamentally flawed, I 	apologize. 
However, this was an RFC patch after all and the problem I am trying to 
solve is real.

>
>>   			goto skip_parent;
>>   		spin_unlock(&dev->power.lock);
>>
>> @@ -755,7 +756,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
>>   		rpm_idle(dev, RPM_ASYNC);
>>
>>    out:
>> -	if (parent && !dev->power.irq_safe) {
>> +	if (parent && (!dev->power.irq_safe || parent->power.irq_safe)) {
>>   		spin_unlock_irq(&dev->power.lock);
>>
>>   		pm_runtime_put(parent);
>> @@ -1269,7 +1270,7 @@ EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
>>    */
>>   void pm_runtime_irq_safe(struct device *dev)
>>   {
>> -	if (dev->parent)
>> +	if (dev->parent && !dev->parent->power.irq_safe)
>>   		pm_runtime_get_sync(dev->parent);
>>   	spin_lock_irq(&dev->power.lock);
>>   	dev->power.irq_safe = 1;
>> @@ -1399,7 +1400,7 @@ void pm_runtime_remove(struct device *dev)
>>   	/* Change the status back to 'suspended' to match the initial status. */
>>   	if (dev->power.runtime_status == RPM_ACTIVE)
>>   		pm_runtime_set_suspended(dev);
>> -	if (dev->power.irq_safe && dev->parent)
>> +	if (dev->power.irq_safe && dev->parent && !dev->parent->power.irq_safe)
>>   		pm_runtime_put(dev->parent);
>>   }
>>   #endif
>>
>

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Rafael J. Wysocki]

On Tuesday, December 02, 2014 11:46:21 PM Jyri Sarha wrote:
> On 12/02/2014 11:54 PM, Rafael J. Wysocki wrote:
> > On Tuesday, December 02, 2014 10:19:03 PM Jyri Sarha wrote:
> >> Do not lock parent of irq safe device to enabled state if the parent
> >> is also irq safe.
> >>
> >> Before this patch the pm_runtime_irq_safe() always called
> >> pm_runtime_get_sync() for the parent, locking the parent to enabled
> >> state and for sure making any parent irq_safe. This is hardly optimal
> >> if the parent device PM code is also irq_safe.
> >>
> >> After this patch the PM runtime core synchronously enables any
> >> irq_safe parents of an irq_safe device when pm_runtime_get_sync() is
> >> called. The parents are checked asyncronously after pm_runtime_put()
> >> call.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >>   drivers/base/power/runtime.c |   11 ++++++-----
> >>   1 file changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> >> index 67c7938..800ca1e 100644
> >> --- a/drivers/base/power/runtime.c
> >> +++ b/drivers/base/power/runtime.c
> >> @@ -541,7 +541,8 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >>   	}
> >>
> >>   	/* Maybe the parent is now able to suspend. */
> >> -	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
> >> +	if (parent && !parent->power.ignore_children &&
> >> +	    (!dev->power.irq_safe || parent->power.irq_safe)) {
> >>   		spin_unlock(&dev->power.lock);
> >>
> >>   		spin_lock(&parent->power.lock);
> >> @@ -706,7 +707,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
> >>   		 * parent is permanently resumed.
> >>   		 */
> >>   		parent = dev->parent;
> >> -		if (dev->power.irq_safe)
> >> +		if (dev->power.irq_safe && !parent->power.irq_safe)
> >
> > Of course, you haven't read the comment above these two lines, because
> > otherwise you wouldn't have submitted this patch at all.
> >
> > I'm not applying it.
> 
> Is there something seriously wrong with the code part too? Or is this 
> just a matter of updating the comment (sorry that I missed that)?

First, this isn't the only comment you've missed.

Second, what if the parent sets its irq_safe after the child does?

> If the thing I am doing somehow fundamentally flawed, I 	apologize. 
> However, this was an RFC patch after all and the problem I am trying to 
> solve is real.

Well, the whole irq_safe concept if somewhat rough and fragile (for example,
it doesn't play well with bus type callbacks that may sleep) and I'm not
sure if making it more fragile is a good idea.

Rafael

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Tomi Valkeinen]

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

On 03/12/14 01:11, Rafael J. Wysocki wrote:

> Second, what if the parent sets its irq_safe after the child does?

Then the behavior becomes the same as it is currently.

But isn't the probe of the parent always ran before the child? And the
only sane place (?) to set irq_safe is in probe, after
pm_runtime_enable. If so, it should never happen in practice that the
parent's irq_safe is called after the child's.

>> If the thing I am doing somehow fundamentally flawed, I 	apologize. 
>> However, this was an RFC patch after all and the problem I am trying to 
>> solve is real.
> 
> Well, the whole irq_safe concept if somewhat rough and fragile (for example,
> it doesn't play well with bus type callbacks that may sleep) and I'm not

I don't know enough of the PM to understand what you mean with above,
but isn't it a bug to set a device irq-safe if it may need sleeping
callbacks? Or do you mean that the device driver may not know if such
sleeping callbacks happen or not?

> sure if making it more fragile is a good idea.

Is there something we can do to fix it? Conceptually I don't see any
issues with it, but I'm probably only looking at it from the platform
device point of view.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Rafael J. Wysocki]

On Wednesday, December 03, 2014 09:45:43 AM Tomi Valkeinen wrote:
> 
> --pNvmjWc6tlSGwtKdGsBmAsBWQX9kn2ndC
> Content-Type: text/plain; charset=utf-8
> Content-Transfer-Encoding: quoted-printable
> 
> On 03/12/14 01:11, Rafael J. Wysocki wrote:
> 
> > Second, what if the parent sets its irq_safe after the child does?
> 
> Then the behavior becomes the same as it is currently.
> 
> But isn't the probe of the parent always ran before the child? And the
> only sane place (?) to set irq_safe is in probe, after
> pm_runtime_enable. If so, it should never happen in practice that the
> parent's irq_safe is called after the child's.

Probably not, but that's an additional assumption that we don't have to make
today.

> >> If the thing I am doing somehow fundamentally flawed, I 	apologize.=20
> >> However, this was an RFC patch after all and the problem I am trying t=
> o=20
> >> solve is real.
> >=20
> > Well, the whole irq_safe concept if somewhat rough and fragile (for exa=
> mple,
> > it doesn't play well with bus type callbacks that may sleep) and I'm no=
> t
> 
> I don't know enough of the PM to understand what you mean with above,
> but isn't it a bug to set a device irq-safe if it may need sleeping
> callbacks? Or do you mean that the device driver may not know if such
> sleeping callbacks happen or not?

The latter.

For example, if the device lands in the ACPI PM domain, irq_safe should not
be set for it, but whether or not that happens depends on the configuration
of the system.

> > sure if making it more fragile is a good idea.
> 
> Is there something we can do to fix it? Conceptually I don't see any
> issues with it, but I'm probably only looking at it from the platform
> device point of view.

I'm sure it is fixable, but I'm not sure what's the best way to fix it.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Alan Stern]

On Tue, 2 Dec 2014, Jyri Sarha wrote:

> Is there something seriously wrong with the code part too? Or is this 
> just a matter of updating the comment (sorry that I missed that)?
> 
> If the thing I am doing somehow fundamentally flawed, I 	apologize. 
> However, this was an RFC patch after all and the problem I am trying to 
> solve is real.

What exactly _is_ the problem you are trying to solve?

When I first wrote the irq_safe stuff, I considered doing something 
like what you want.  But there was no demand for it at the time, and 
leaving it out seemed simpler and safer.

However, if you have a genuine use case then I don't object to allowing
parents of irq-safe devices to enter runtime suspend, provided the
parents are also irq-safe.

Alan Stern

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Tomi Valkeinen]

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

On 03/12/14 17:22, Alan Stern wrote:
> On Tue, 2 Dec 2014, Jyri Sarha wrote:
> 
>> Is there something seriously wrong with the code part too? Or is this 
>> just a matter of updating the comment (sorry that I missed that)?
>>
>> If the thing I am doing somehow fundamentally flawed, I 	apologize. 
>> However, this was an RFC patch after all and the problem I am trying to 
>> solve is real.
> 
> What exactly _is_ the problem you are trying to solve?
> 
> When I first wrote the irq_safe stuff, I considered doing something 
> like what you want.  But there was no demand for it at the time, and 
> leaving it out seemed simpler and safer.
> 
> However, if you have a genuine use case then I don't object to allowing
> parents of irq-safe devices to enter runtime suspend, provided the
> parents are also irq-safe.

OMAP Display subsystem hardware has one main module (dss) and multiple
submodules (dispc being the important one here). The submodules require
the main module to be enabled and configured to work. These are modeled
as a dss parent platform device, and a number of child platform devices.

The DRM driver for OMAP (omapdrm uses the dispc device, and uses runtime
PM to control dispc's power state. Unfortunately on some cases omapdrm
wants to access the hardware from atomic context, and needs to enable
the dispc first.

omapdrm could be changed not to touch the hardware from atomic context,
but that is a big change, requiring rewrite of its core logic. I hope we
get that done some day, as it would be a performance boost also. But as
for today, omapdrm will crash in some cases when it happens to call
runtime_get from atomic context.

So... If what's proposed in this patch is messy and risky, I think we
should skip it and try to change omapdrm as soon as we manage to. If so,
I'd still be interested to hear what problems this might cause, as we're
planning to apply it to our internal tree to fix the issue with omapdrm.

But as the functionality in this patch sounded sane and something which
could be usable for others also, we wanted to try this approach.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Alan Stern]

On Wed, 3 Dec 2014, Tomi Valkeinen wrote:

> On 03/12/14 17:22, Alan Stern wrote:
> > On Tue, 2 Dec 2014, Jyri Sarha wrote:
> > 
> >> Is there something seriously wrong with the code part too? Or is this 
> >> just a matter of updating the comment (sorry that I missed that)?
> >>
> >> If the thing I am doing somehow fundamentally flawed, I 	apologize. 
> >> However, this was an RFC patch after all and the problem I am trying to 
> >> solve is real.
> > 
> > What exactly _is_ the problem you are trying to solve?
> > 
> > When I first wrote the irq_safe stuff, I considered doing something 
> > like what you want.  But there was no demand for it at the time, and 
> > leaving it out seemed simpler and safer.
> > 
> > However, if you have a genuine use case then I don't object to allowing
> > parents of irq-safe devices to enter runtime suspend, provided the
> > parents are also irq-safe.
> 
> OMAP Display subsystem hardware has one main module (dss) and multiple
> submodules (dispc being the important one here). The submodules require
> the main module to be enabled and configured to work. These are modeled
> as a dss parent platform device, and a number of child platform devices.

Although you didn't say so, it sounds like you want the dss device to 
go into runtime suspend whenever it isn't needed by any of the child 
devices.

> The DRM driver for OMAP (omapdrm uses the dispc device, and uses runtime
> PM to control dispc's power state. Unfortunately on some cases omapdrm
> wants to access the hardware from atomic context, and needs to enable
> the dispc first.
> 
> omapdrm could be changed not to touch the hardware from atomic context,
> but that is a big change, requiring rewrite of its core logic. I hope we
> get that done some day, as it would be a performance boost also. But as
> for today, omapdrm will crash in some cases when it happens to call
> runtime_get from atomic context.
> 
> So... If what's proposed in this patch is messy and risky, I think we
> should skip it and try to change omapdrm as soon as we manage to. If so,
> I'd still be interested to hear what problems this might cause, as we're
> planning to apply it to our internal tree to fix the issue with omapdrm.
> 
> But as the functionality in this patch sounded sane and something which
> could be usable for others also, we wanted to try this approach.

It seems reasonable to me.  But Rafael is the person you need to 
convince.

Alan Stern

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Rafael J. Wysocki]

On Wednesday, December 03, 2014 05:53:52 PM Tomi Valkeinen wrote:
> 
> --9XMi52Vjv8MSaMHnoh2EDME1sPQ9gtiND
> Content-Type: text/plain; charset=windows-1252
> Content-Transfer-Encoding: quoted-printable
> 
> On 03/12/14 17:22, Alan Stern wrote:
> > On Tue, 2 Dec 2014, Jyri Sarha wrote:
> >=20
> >> Is there something seriously wrong with the code part too? Or is this =
> 
> >> just a matter of updating the comment (sorry that I missed that)?
> >>
> >> If the thing I am doing somehow fundamentally flawed, I 	apologize.=20
> >> However, this was an RFC patch after all and the problem I am trying t=
> o=20
> >> solve is real.
> >=20
> > What exactly _is_ the problem you are trying to solve?
> >=20
> > When I first wrote the irq_safe stuff, I considered doing something=20
> > like what you want.  But there was no demand for it at the time, and=20
> > leaving it out seemed simpler and safer.
> >=20
> > However, if you have a genuine use case then I don't object to allowing=
> 
> > parents of irq-safe devices to enter runtime suspend, provided the
> > parents are also irq-safe.
> 
> OMAP Display subsystem hardware has one main module (dss) and multiple
> submodules (dispc being the important one here). The submodules require
> the main module to be enabled and configured to work. These are modeled
> as a dss parent platform device, and a number of child platform devices.
> 
> The DRM driver for OMAP (omapdrm uses the dispc device, and uses runtime
> PM to control dispc's power state. Unfortunately on some cases omapdrm
> wants to access the hardware from atomic context, and needs to enable
> the dispc first.
> 
> omapdrm could be changed not to touch the hardware from atomic context,
> but that is a big change, requiring rewrite of its core logic. I hope we
> get that done some day, as it would be a performance boost also. But as
> for today, omapdrm will crash in some cases when it happens to call
> runtime_get from atomic context.
> 
> So... If what's proposed in this patch is messy and risky, I think we
> should skip it and try to change omapdrm as soon as we manage to. If so,
> I'd still be interested to hear what problems this might cause, as we're
> planning to apply it to our internal tree to fix the issue with omapdrm.

It is sort of fragile.

Currently, irq_safe causes PM callbacks to be executed with interrupts off.
That may potentially lead to the execution of quite big chunks of code
with interrupts off (imagine a suspended device, with a suspended parent
and a suspended grand parent, all with irq_safe set).

In your patch that won't actually happen, because the pm_runtime_put(parent)
is still executed with interrupts on, but I'm wondering if that's really OK.

> But as the functionality in this patch sounded sane and something which
> could be usable for others also, we wanted to try this approach.

To me, it requires quite a bit of consideration.

Definitely, I don't want to make changes that will work for platform devices
only and that will require some special conditions to be met to work.

Anyway, can we get back to that next week?  We're likely to have a merge window
in a few days ...

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RFC] PM / Runtime: Better support for nested irq_safe drivers

[Jyri Sarha]

On 12/04/2014 01:11 AM, Rafael J. Wysocki wrote:
> On Wednesday, December 03, 2014 05:53:52 PM Tomi Valkeinen wrote:
>>
>> --9XMi52Vjv8MSaMHnoh2EDME1sPQ9gtiND
>> Content-Type: text/plain; charset=windows-1252
>> Content-Transfer-Encoding: quoted-printable
>>
>> On 03/12/14 17:22, Alan Stern wrote:
>>> On Tue, 2 Dec 2014, Jyri Sarha wrote:
>>> =20
>>>> Is there something seriously wrong with the code part too? Or is this =
>>
>>>> just a matter of updating the comment (sorry that I missed that)?
>>>>
>>>> If the thing I am doing somehow fundamentally flawed, I 	apologize.=20
>>>> However, this was an RFC patch after all and the problem I am trying t=
>> o=20
>>>> solve is real.
>>> =20
>>> What exactly _is_ the problem you are trying to solve?
>>> =20
>>> When I first wrote the irq_safe stuff, I considered doing something=20
>>> like what you want.  But there was no demand for it at the time, and=20
>>> leaving it out seemed simpler and safer.
>>> =20
>>> However, if you have a genuine use case then I don't object to allowing=
>>
>>> parents of irq-safe devices to enter runtime suspend, provided the
>>> parents are also irq-safe.
>>
>> OMAP Display subsystem hardware has one main module (dss) and multiple
>> submodules (dispc being the important one here). The submodules require
>> the main module to be enabled and configured to work. These are modeled
>> as a dss parent platform device, and a number of child platform devices.
>>
>> The DRM driver for OMAP (omapdrm uses the dispc device, and uses runtime
>> PM to control dispc's power state. Unfortunately on some cases omapdrm
>> wants to access the hardware from atomic context, and needs to enable
>> the dispc first.
>>
>> omapdrm could be changed not to touch the hardware from atomic context,
>> but that is a big change, requiring rewrite of its core logic. I hope we
>> get that done some day, as it would be a performance boost also. But as
>> for today, omapdrm will crash in some cases when it happens to call
>> runtime_get from atomic context.
>>
>> So... If what's proposed in this patch is messy and risky, I think we
>> should skip it and try to change omapdrm as soon as we manage to. If so,
>> I'd still be interested to hear what problems this might cause, as we're
>> planning to apply it to our internal tree to fix the issue with omapdrm.
>
> It is sort of fragile.
>
> Currently, irq_safe causes PM callbacks to be executed with interrupts off.
> That may potentially lead to the execution of quite big chunks of code
> with interrupts off (imagine a suspended device, with a suspended parent
> and a suspended grand parent, all with irq_safe set).
>
> In your patch that won't actually happen, because the pm_runtime_put(parent)
> is still executed with interrupts on, but I'm wondering if that's really OK.
>

That was my choice. I decided there should be no hurry on suspending the 
parents and grandparents (currently they are all locked to enabled state 
anyway). I considered swift execution of suspend more important.

>> But as the functionality in this patch sounded sane and something which
>> could be usable for others also, we wanted to try this approach.
>
> To me, it requires quite a bit of consideration.
>
> Definitely, I don't want to make changes that will work for platform devices
> only and that will require some special conditions to be met to work.
>
> Anyway, can we get back to that next week?  We're likely to have a merge window
> in a few days ...
>

I'll send a new version of the patch with all comment updated according 
the the code change. I don't expect you to merge it. Just for the record 
I want to make the patch complete. Maybe someone else is struggling with 
the same issue and finds the patch useful.

Best regards,
Jyri

[PATCH RFC v2] PM / Runtime: Better support for nested irq_safe drivers

[Jyri Sarha]

Do not lock parent of irq safe device to enabled state if the parent
is also irq safe.

Before this patch the pm_runtime_irq_safe() always called
pm_runtime_get_sync() for the parent, locking the parent to enabled
state and for sure making any parent irq_safe. This is hardly optimal
if the parent device PM code is also irq_safe.

After this patch the PM runtime core synchronously enables any
irq_safe parents of an irq_safe device when pm_runtime_get_sync() is
called. The parents are checked asyncronously after pm_runtime_put()
call.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
I sent this just for the record and for the sake of the convesation. I
do not expect this patch to get applied as I do not undrestand all the
implication it may have. I can only say that it appears to work well
in our environment (OMAPDSS on omap4+).

 drivers/base/power/runtime.c |   45 +++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 67c7938..6d8802a 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -406,13 +406,14 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
  * or wait for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC
  * flags. If the RPM_ASYNC flag is set then queue a suspend request;
  * otherwise run the ->runtime_suspend() callback directly. When
- * ->runtime_suspend succeeded, if a deferred resume was requested while
- * the callback was running then carry it out, otherwise send an idle
- * notification for its parent (if the suspend succeeded and both
- * ignore_children of parent->power and irq_safe of dev->power are not set).
- * If ->runtime_suspend failed with -EAGAIN or -EBUSY, and if the RPM_AUTO
- * flag is set and the next autosuspend-delay expiration time is in the
- * future, schedule another autosuspend attempt.
+ * ->runtime_suspend succeeded, if a deferred resume was requested
+ * while the callback was running then carry it out, otherwise send an
+ * idle notification for its parent (if the suspend succeeded,
+ * ignore_children of parent->power is not set and irq_safe of
+ * parent->power is set or irq_safe of dev->power is not set).  If
+ * ->runtime_suspend failed with -EAGAIN or -EBUSY, and if the
+ * RPM_AUTO flag is set and the next autosuspend-delay expiration time
+ * is in the future, schedule another autosuspend attempt.
  *
  * This function must be called under dev->power.lock with interrupts disabled.
  */
@@ -541,7 +542,8 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	}
 
 	/* Maybe the parent is now able to suspend. */
-	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
+	if (parent && !parent->power.ignore_children &&
+	    (!dev->power.irq_safe || parent->power.irq_safe)) {
 		spin_unlock(&dev->power.lock);
 
 		spin_lock(&parent->power.lock);
@@ -701,12 +703,13 @@ static int rpm_resume(struct device *dev, int rpmflags)
 
 	if (!parent && dev->parent) {
 		/*
-		 * Increment the parent's usage counter and resume it if
-		 * necessary.  Not needed if dev is irq-safe; then the
-		 * parent is permanently resumed.
+		 * Increment the parent's usage counter and resume it
+		 * if necessary.  Not needed if dev is irq-safe and
+		 * its paret is not; then the parent is permanently
+		 * resumed.
 		 */
 		parent = dev->parent;
-		if (dev->power.irq_safe)
+		if (dev->power.irq_safe && !parent->power.irq_safe)
 			goto skip_parent;
 		spin_unlock(&dev->power.lock);
 
@@ -755,7 +758,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
 		rpm_idle(dev, RPM_ASYNC);
 
  out:
-	if (parent && !dev->power.irq_safe) {
+	if (parent && (!dev->power.irq_safe || parent->power.irq_safe)) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_runtime_put(parent);
@@ -1261,15 +1264,17 @@ EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
  * @dev: Device to handle
  *
  * Set the power.irq_safe flag, which tells the PM core that the
- * ->runtime_suspend() and ->runtime_resume() callbacks for this device should
- * always be invoked with the spinlock held and interrupts disabled.  It also
- * causes the parent's usage counter to be permanently incremented, preventing
- * the parent from runtime suspending -- otherwise an irq-safe child might have
- * to wait for a non-irq-safe parent.
+ * ->runtime_suspend() and ->runtime_resume() callbacks for this
+ * device should always be invoked with the spinlock held and
+ * interrupts disabled.  It also causes the parent's usage counter to
+ * be permanently incremented if the parent's power.irq_safe flag is
+ * not set, preventing a non-irq-safe parent from runtime suspending
+ * -- otherwise an irq-safe child might have to wait for a
+ * non-irq-safe parent.
  */
 void pm_runtime_irq_safe(struct device *dev)
 {
-	if (dev->parent)
+	if (dev->parent && !dev->parent->power.irq_safe)
 		pm_runtime_get_sync(dev->parent);
 	spin_lock_irq(&dev->power.lock);
 	dev->power.irq_safe = 1;
@@ -1399,7 +1404,7 @@ void pm_runtime_remove(struct device *dev)
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
 		pm_runtime_set_suspended(dev);
-	if (dev->power.irq_safe && dev->parent)
+	if (dev->power.irq_safe && dev->parent && !dev->parent->power.irq_safe)
 		pm_runtime_put(dev->parent);
 }
 #endif
-- 
1.7.9.5