[PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Geert Uytterhoeven]

Hi Lina,

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer at linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.

[...]

Thanks for your patch!

> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -600,7 +600,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
>  into a low-power state together at the same time by turning off the shared
>  power resource.  Of course, they also need to be put into the full-power state
>  together, by turning the shared power resource on.  A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another domain.

"... another power domain"?

> +Devices, by default, operate in process context and if a device can operate in
> +IRQ safe context, has to be explicitly set as IRQ safe. PM domains by default,

This isn't exactly your fault, but this section is the only one inside this
document that talks about "power domains". All other sections talk about
"PM domains".
You're adding a paragraph about "PM domains" inside this section.

> +operate in process context but could have devices that are IRQ safe. Such PM
> +domains cannot be powered on/off during runtime PM. On the other hand, an IRQ
> +safe PM domain that can be powered on/off and suspend or resumed in an atomic
> +context, may contain IRQ safe devices. Such domain may only contain only IRQ
> +safe devices or IRQ safe sub-domains.
>
>  Support for power domains is provided through the pm_domain field of struct
>  device.  This field is a pointer to an object of type struct dev_pm_domain,
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e9b7cfb..42ffb8b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -50,6 +50,74 @@
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> +static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       if (unlikely(subclass > 0))

In general, using "unlikely" is frowned upon.
And in this case, the compiler already knows the exact value of subclass,
as it's passed as a constant from one of the functions below.

> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->mlock)
> +{
> +       if (unlikely(subclass > 0))

Same here.

> @@ -1277,11 +1356,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>                 return -EINVAL;
>
> +       /* Devices in an IRQ safe PM domain have to be IRQ safe too */

Do you really need this comment? It's identical to the error message below.

> +       if (genpd->irq_safe && !dev->power.irq_safe) {
> +               dev_err(dev,
> +                       "Devices in an IRQ safe domain have to be IRQ safe.\n");

> @@ -1385,12 +1471,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>             || genpd == subdomain)
>                 return -EINVAL;
>
> +       /*
> +        * If the domain can be powered on/off in an IRQ safe
> +        * context, ensure that the subdomain can also be
> +        * powered on/off in that context.
> +        */
> +       if (genpd->irq_safe && !subdomain->irq_safe) {
> +               WARN("Incompatible sub-domain %s of IRQ safe domain %s\n",
> +                               subdomain->name, genpd->name);

If you change the error message to "Subdomains in an IRQ safe domain have
to be IRQ safe", it's (a) consistent with the one for devices, and (b) you
can remove the comment.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds