[...]

quoted

quoted

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

[...]

quoted

quoted

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

quoted

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

quoted

quoted

                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