[PATCH RFC 2/3] PM / Domains: Support atomic PM domains

[Ulf Hansson]

[...]

>>> @@ -387,7 +452,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain
>>> *genpd)
>>>                         return -EBUSY;
>>>
>>>                 if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
>>> -                   || pdd->dev->power.irq_safe))
>>> +                       || (pdd->dev->power.irq_safe &&
>>> !genpd->irq_safe)))
>>
>>
>> This deserves a comment in the code.
>>
> This check should simplify to
>                 if (!pm_runtime_suspended(pdd->dev))
>
> The other connditions seem unnecessary just to determine if the devices are
> suspended.
>
> Do you see any problem with that?

I am not sure, need to think a bit more about it. Anyway, I wouldn't
do the "simplification" in $subject patch, since it's a related to
different topic.

[...]

>>> @@ -478,12 +543,8 @@ static int pm_genpd_runtime_suspend(struct device
>>> *dev)
>>>         if (IS_ERR(genpd))
>>>                 return -EINVAL;
>>>
>>> -       /*
>>> -        * We can't allow to power off the PM domain if it holds an
>>> irq_safe
>>> -        * device. That's beacuse we use mutexes to protect data while
>>> power
>>> -        * off and on the PM domain, thus we can't execute in atomic
>>> context.
>>> -        */
>>> -       if (dev->power.irq_safe)
>>> +       /* We can't allow to power off a domain that is also not irq
>>> safe. */
>>
>>
>> I wouldn't mind to keep some more pieces of the earlier comment, since
>> it explains a bit more of the *why*. Could you try to rephrase this
>> comment in that regard?
>>
>>> +       if (dev->power.irq_safe && !genpd->irq_safe)
>>
>>
>> This looks correct...
>>
> Apparently not the best place to check this. We should still allow
> runtime suspend of the device, irrespective of whether the domain is IRQ
> safe or not and this check here is incorrect.

You are absolutely correct!

That change is actually done as a part of the patch I posted [1],
which $subject patch is based upon.

Earlier we could only invoke the ->stop|start() callbacks for an IRQ
safe device. According to the changes made in [1], we are now able to
also invoke the ->runtime_suspend|resume() callbacks. Yet another
positive "side effect". I will cook a v4 and post a new version to fix
this.

>
>>>                 return -EBUSY;
>>>
>>>         stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;
>>> @@ -500,11 +561,19 @@ static int pm_genpd_runtime_suspend(struct device
>>> *dev)
>>>                 return ret;
>>>         }
>>>
>>> -       mutex_lock(&genpd->lock);
>>> +       /*
>>> +        * If power.irq_safe is set, this routine will be run with
>>> interrupts
>>> +        * off, so suspend only if the power domain is irq_safe.
>>> +        */
>>> +       if (dev->power.irq_safe && !genpd->irq_safe)
>>> +               return 0;
>>
>>
>> ... but this doesn't. You have already returned -EBUSY above for this
>> case.
>>
> This is probably more appropriate, as we want to skip locking the
> domain, because it may be in atomic context and the domain may use
> mutexes to lock.

Yep.

>
> I feel like, we miss out an opportunity here to power off the domain, if
> device is IRQ safe while the domain is not, but the function is called
> from a process context. I am not sure how to check that correctly, yet.

You may be able to power off the PM domain for some cases, yes. But...
if you do that, how would you then be able to power on the PM domain
from atomic context via pm_genpd_runtime_resume()?

[...]

[1]
http://www.spinics.net/lists/arm-kernel/msg424994.html

Kind regards
Uffe