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

[Jyri Sarha <jsarha@ti.com>]

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