Subject: [PATCH v2 2/7] PM / Domains: Support IRQ safe PM domains

From: [email-masked] (Lina Iyer)
Date: Fri, 4 Sep 2015 10:05:40 -0600

On Fri, Sep 04 2015 at 04:02 -0600, Ulf Hansson wrote:
>On 3 September 2015 at 21:58, Lina Iyer <[email-masked]> 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.
>>
>> However, not all domains can operate in IRQ safe contexts. Genpd
>> therefore, has to support both cases where the domain may or may not
>> operate in IRQ safe contexts. Configuring genpd to use an appropriate
>> lock for that domain, would allow domains that have IRQ safe devices to
>> runtime suspend and resume, in atomic context.
>>
>> To achieve domain specific locking, set the domain's ->flag to
>> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
>> should use a spinlock instead of a mutex for locking the domain. Locking
>> is abstracted through genpd_lock() and genpd_unlock() functions that use
>> the flag to determine the appropriate lock to be used for that domain.
>> Domains that have lower latency to suspend and resume and can operate
>> with IRQs disabled may now be able to save power, when the component
>> devices and sub-domains are idle at runtime.
>>
>> The restriction this imposes on the domain hierarchy is that sub-domains
>> and all devices in the IRQ safe domain's hierarchy also have to be IRQ
>> safe, so that we dont try to lock a mutex, while holding a spinlock.
>
>Moreover we can't have IRQ safe domains mixed with non-IRQ safe
>subdomains domains, right?
>
>> Non-IRQ safe domains may continue to have devices and sub-domains that
>> may or may not be IRQ safe.
>>
>> Cc: Ulf Hansson <[email-masked]>
>> Cc: Kevin Hilman <[email-masked]>
>> Cc: Rafael J. Wysocki <[email-masked]>
>> Cc: Geert Uytterhoeven <[email-masked]>
>> Cc: Krzysztof Koz?owski <[email-masked]>
>> Signed-off-by: Lina Iyer <[email-masked]>
>> ---
>>  Documentation/power/devices.txt |  11 ++-
>>  drivers/base/power/domain.c     | 212 ++++++++++++++++++++++++++++++++--------
>>  include/linux/pm_domain.h       |  11 ++-
>>  3 files changed, 189 insertions(+), 45 deletions(-)
>>
>> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
>> index 8ba6625..bde6141 100644
>> --- a/Documentation/power/devices.txt
>> +++ b/Documentation/power/devices.txt
>> @@ -607,7 +607,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 power domain.
>
>This seems like a separate change.
>
>> +
>> +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. Power domains by
>> +default, operate in process context but could have devices that are IRQ safe.
>> +Such power 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 suspended or resumed
>> +in an atomic context, may contain IRQ safe devices. Such domains may only
>> +contain IRQ safe devices or IRQ safe sub-domains.
>
>This is combination of describing the current behaviour and the
>changed behaviour after @subject patch. Could you please split this
>into separate patches?
>
Sure.

>>
>>  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 ef8d19f..9849338 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -53,6 +53,80 @@
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>>
>> +static inline int genpd_lock_nosleep(struct generic_pm_domain *genpd,
>> +                                       unsigned int subclass)
>> +       __acquires(&genpd->slock)
>> +{
>> +       unsigned long flags;
>> +
>> +       if (subclass > 0)
>> +               spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
>> +       else
>> +               spin_lock_irqsave(&genpd->slock, flags);
>> +
>> +       genpd->lock_flags = flags;
>> +       return 0;
>> +}
>> +
>> +static inline void genpd_unlock_nosleep(struct generic_pm_domain *genpd)
>> +       __releases(&genpd->slock)
>> +{
>> +       spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
>> +}
>> +
>> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
>> +                                       unsigned int subclass)
>> +       __acquires(&genpd->mlock)
>> +{
>> +       if (subclass > 0)
>> +               mutex_lock_nested(&genpd->mlock, subclass);
>> +       else
>> +               mutex_lock(&genpd->mlock);
>> +       return 0;
>> +}
>> +
>> +static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
>> +       __acquires(&genpd->mlock)
>> +{
>> +       return mutex_lock_interruptible(&genpd->mlock);
>> +}
>> +
>> +static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
>> +       __releases(&genpd->mlock)
>> +{
>> +       mutex_unlock(&genpd->mlock);
>> +}
>> +
>> +static inline int genpd_lock(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
>> +                       : genpd_lock_irq(genpd, 0);
>> +}
>> +
>> +static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, SINGLE_DEPTH_NESTING)
>> +                       : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
>> +}
>> +
>> +static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
>> +                       : genpd_lock_interruptible_irq(genpd);
>> +}
>> +
>> +static inline void genpd_unlock(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_unlock_nosleep(genpd)
>> +                       : genpd_unlock_irq(genpd);
>> +}
>
>I think all the above functions that check the genpd->irq_safe flag
>can be converted into dealing only with one type of lock, and then
>assigned as functions pointers at initialization, depending on the
>configuration.
>
>This would mean we don't need to check the genpd->irq_safe each time
>we acquire or release a lock.
>
I thought about it. Regmap does that. But that would mean, we carry
around, three function pointers - lock, lock_nested, unlock on each
genpd object.

>> +
>> +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
>> +               struct generic_pm_domain *genpd)
>> +{
>> +       return dev->power.irq_safe && !genpd->irq_safe;
>> +}
>> +
>>  static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>  {
>>         struct generic_pm_domain *genpd = NULL, *gpd;
>> @@ -273,9 +347,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
>>  {
>>         int ret;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>         ret = __pm_genpd_poweron(genpd);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         return ret;
>>  }
>>
>> @@ -335,9 +409,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>>                 spin_unlock_irq(&dev->power.lock);
>>
>>                 if (!IS_ERR(genpd)) {
>> -                       mutex_lock(&genpd->lock);
>> +                       genpd_lock(genpd);
>>                         genpd->max_off_time_changed = true;
>> -                       mutex_unlock(&genpd->lock);
>> +                       genpd_unlock(genpd);
>>                 }
>>
>>                 dev = dev->parent;
>> @@ -394,8 +468,17 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>>                 if (stat > PM_QOS_FLAGS_NONE)
>>                         return -EBUSY;
>>
>> -               if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
>
>I don't remember having above line in genpd. I guess you missed to
>include the below patch within in this patchset:
>"PM / Domains: Remove dev->driver check for runtime PM".
>
I seemed to me that the change deserves its own patch.

>> +               /*
>> +                * We do not want to power off the domain if the device is
>> +                * not suspended or an IRQ safe device is part of this
>> +                * non-IRQ safe domain.
>> +                */
>> +               if (!pm_runtime_suspended(pdd->dev) ||
>> +                       irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
>>                         not_suspended++;
>> +               WARN_ONCE(irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd),
>> +                               "PM domain %s will not be powered off\n",
>> +                               genpd->name);
>>         }
>>
>>         if (not_suspended > genpd->in_progress)
>> @@ -460,9 +543,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>>
>>         genpd = container_of(work, struct generic_pm_domain, power_off_work);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>         pm_genpd_poweroff(genpd);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>  }
>>
>>  /**
>> @@ -500,17 +583,21 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>>         }
>>
>>         /*
>> -        * If power.irq_safe is set, this routine will be run with interrupts
>> -        * off, so it can't use mutexes.
>> +        * If power.irq_safe is set, this routine may be run with
>> +        * IRQ disabled, so suspend only if the power domain is
>> +        * irq_safe.
>
>Maybe elaborate on that the locking mechanism differ, which is why we
>can't proceed in some cases...
>
>>          */
>> -       if (dev->power.irq_safe)
>> +       WARN_ONCE(irq_safe_dev_in_no_sleep_domain(dev, genpd),
>> +                       "genpd %s will not be powered off\n", genpd->name);
>
>Perhaps, unless the string becomes too long, some information about *why*.
>
Ok

>> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
>>                 return 0;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>> +
>>         genpd->in_progress++;
>>         pm_genpd_poweroff(genpd);
>>         genpd->in_progress--;
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         return 0;
>>  }
>> @@ -535,15 +622,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
>>         if (IS_ERR(genpd))
>>                 return -EINVAL;
>>
>> -       /* If power.irq_safe, the PM domain is never powered off. */
>> -       if (dev->power.irq_safe) {
>> +       /*
>> +        * As we dont power off a non IRQ safe domain, which holds
>> +        * an IRQ safe device, we dont need to restore power to it.
>> +        */
>> +       if (dev->power.irq_safe && !genpd->irq_safe) {
>>                 timed = false;
>>                 goto out;
>>         }
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>         ret = __pm_genpd_poweron(genpd);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (ret)
>>                 return ret;
>> @@ -743,14 +833,14 @@ static int pm_genpd_prepare(struct device *dev)
>>         if (resume_needed(dev, genpd))
>>                 pm_runtime_resume(dev);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->prepared_count++ == 0) {
>>                 genpd->suspended_count = 0;
>>                 genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
>>         }
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (genpd->suspend_power_off) {
>>                 pm_runtime_put_noidle(dev);
>> @@ -768,12 +858,12 @@ static int pm_genpd_prepare(struct device *dev)
>>
>>         ret = pm_generic_prepare(dev);
>>         if (ret) {
>> -               mutex_lock(&genpd->lock);
>> +               genpd_lock(genpd);
>>
>>                 if (--genpd->prepared_count == 0)
>>                         genpd->suspend_power_off = false;
>>
>> -               mutex_unlock(&genpd->lock);
>> +               genpd_unlock(genpd);
>>                 pm_runtime_enable(dev);
>>         }
>>
>> @@ -1130,13 +1220,13 @@ static void pm_genpd_complete(struct device *dev)
>>         if (IS_ERR(genpd))
>>                 return;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         run_complete = !genpd->suspend_power_off;
>>         if (--genpd->prepared_count == 0)
>>                 genpd->suspend_power_off = false;
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (run_complete) {
>>                 pm_generic_complete(dev);
>> @@ -1280,11 +1370,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;
>>
>> +       if (genpd->irq_safe && !dev->power.irq_safe) {
>> +               dev_err(dev,
>> +                       "PM Domain %s is IRQ safe; device has to IRQ safe.\n",
>
>/s/; device has to IRQ safe/, but device isn't.
>
>> +                       genpd->name);
>> +               return -EINVAL;
>> +       }
>> +
>>         gpd_data = genpd_alloc_dev_data(dev, genpd, td);
>>         if (IS_ERR(gpd_data))
>>                 return PTR_ERR(gpd_data);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->prepared_count > 0) {
>>                 ret = -EAGAIN;
>> @@ -1301,7 +1398,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>>         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (ret)
>>                 genpd_free_dev_data(dev, gpd_data);
>> @@ -1345,7 +1442,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>         gpd_data = to_gpd_data(pdd);
>>         dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->prepared_count > 0) {
>>                 ret = -EAGAIN;
>> @@ -1360,14 +1457,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>
>>         list_del_init(&pdd->list_node);
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         genpd_free_dev_data(dev, gpd_data);
>>
>>         return 0;
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         dev_pm_qos_add_notifier(dev, &gpd_data->nb);
>>
>>         return ret;
>> @@ -1388,12 +1485,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) {
>
>Is this really enough? What if the subdomain is IRQ safe but its
>parent isn't, that's not allowed either, right?
>
That should be allowable. We would just not power off the parent and
expect it to be powered on always, when the subdomain powers on/off.

>> +               WARN("Sub-domain (%s) in an IRQ-safe domain (%s) has to be IRQ safe\n",
>> +                               subdomain->name, genpd->name);
>> +               return -EINVAL;
>> +       }
>> +
>>         link = kzalloc(sizeof(*link), GFP_KERNEL);
>>         if (!link)
>>                 return -ENOMEM;
>>
>> -       mutex_lock(&genpd->lock);
>> -       mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>> +       genpd_lock(genpd);
>> +       genpd_lock_nested(subdomain);
>>
>>         if (genpd->status == GPD_STATE_POWER_OFF
>>             &&  subdomain->status != GPD_STATE_POWER_OFF) {
>> @@ -1416,8 +1524,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>                 genpd_sd_counter_inc(genpd);
>>
>>   out:
>> -       mutex_unlock(&subdomain->lock);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(subdomain);
>> +       genpd_unlock(genpd);
>>         if (ret)
>>                 kfree(link);
>>         return ret;
>> @@ -1466,13 +1574,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
>>                 return -EINVAL;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         list_for_each_entry(link, &genpd->master_links, master_node) {
>>                 if (link->slave != subdomain)
>>                         continue;
>>
>> -               mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>> +               genpd_lock_nested(subdomain);
>>
>>                 list_del(&link->master_node);
>>                 list_del(&link->slave_node);
>> @@ -1480,13 +1588,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>>                 if (subdomain->status != GPD_STATE_POWER_OFF)
>>                         genpd_sd_counter_dec(genpd);
>>
>> -               mutex_unlock(&subdomain->lock);
>> +               genpd_unlock(subdomain);
>>
>>                 ret = 0;
>>                 break;
>>         }
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         return ret;
>>  }
>> @@ -1510,11 +1618,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>>         if (IS_ERR_OR_NULL(genpd) || state < 0)
>>                 return -EINVAL;
>>
>> +       if (genpd->irq_safe) {
>> +               WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
>> +                               genpd->name);
>> +               return -EINVAL;
>> +       }
>> +
>>         cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>>         if (!cpuidle_data)
>>                 return -ENOMEM;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->cpuidle_data) {
>>                 ret = -EEXIST;
>> @@ -1541,7 +1655,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>>         genpd_recalc_cpu_exit_latency(genpd);
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         return ret;
>>
>>   err:
>> @@ -1578,7 +1692,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
>>         if (IS_ERR_OR_NULL(genpd))
>>                 return -EINVAL;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         cpuidle_data = genpd->cpuidle_data;
>>         if (!cpuidle_data) {
>> @@ -1596,7 +1710,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
>>         kfree(cpuidle_data);
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         return ret;
>>  }
>>
>> @@ -1657,6 +1771,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
>>         return cb ? cb(dev) : 0;
>>  }
>>
>> +static void genpd_lock_init(struct generic_pm_domain *genpd)
>> +{
>> +       if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>> +               spin_lock_init(&genpd->slock);
>> +               genpd->irq_safe = true;
>> +       } else {
>> +               mutex_init(&genpd->mlock);
>> +               genpd->irq_safe = false;
>> +       }
>
>As I stated earlier around having function pointers, this would be the
>place where to assign them I guess.
>
>> +}
>> +
>>  /**
>>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>>   * @genpd: PM domain object to initialize.
>> @@ -1672,7 +1797,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>         INIT_LIST_HEAD(&genpd->master_links);
>>         INIT_LIST_HEAD(&genpd->slave_links);
>>         INIT_LIST_HEAD(&genpd->dev_list);
>> -       mutex_init(&genpd->lock);
>> +       genpd_lock_init(genpd);
>>         genpd->gov = gov;
>>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>>         genpd->in_progress = 0;
>> @@ -2067,7 +2192,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>>         struct gpd_link *link;
>>         int ret;
>>
>> -       ret = mutex_lock_interruptible(&genpd->lock);
>> +       ret = genpd_lock_interruptible(genpd);
>>         if (ret)
>>                 return -ERESTARTSYS;
>>
>> @@ -2087,7 +2212,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
>>         }
>>
>>         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
>> -               kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
>> +               kobj_path = kobject_get_path(&pm_data->dev->kobj,
>> +                               genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
>>                 if (kobj_path == NULL)
>>                         continue;
>>
>> @@ -2098,7 +2224,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>>
>>         seq_puts(s, "\n");
>>  exit:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         return 0;
>>  }
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index b2725e6..dc7cb53 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -16,9 +16,11 @@
>>  #include <linux/of.h>
>>  #include <linux/notifier.h>
>>  #include <linux/cpuidle.h>
>> +#include <linux/spinlock.h>
>>
>>  /* Defines used for the flags field in the struct generic_pm_domain */
>>  #define GENPD_FLAG_PM_CLK      (1U << 0) /* PM domain uses PM clk */
>> +#define GENPD_FLAG_IRQ_SAFE    (1U << 1) /* PM domain operates in atomic */
>>
>>  enum gpd_status {
>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>> @@ -49,7 +51,6 @@ struct generic_pm_domain {
>>         struct list_head master_links;  /* Links with PM domain as a master */
>>         struct list_head slave_links;   /* Links with PM domain as a slave */
>>         struct list_head dev_list;      /* List of devices */
>> -       struct mutex lock;
>>         struct dev_power_governor *gov;
>>         struct work_struct power_off_work;
>>         const char *name;
>> @@ -74,6 +75,14 @@ struct generic_pm_domain {
>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>                            struct device *dev);
>>         unsigned int flags;             /* Bit field of configs for genpd */
>> +       bool irq_safe;
>
>If you convert to functions pointers, perhaps you can remove the
>irq_safe bool here, and instead only use the flags field.
>
Answered earlier.

Another point to consider is that, this facilitates the use of
__acquires() and __releases(), which need a lock object and not a
pointer.

>> +       union {
>> +               struct mutex mlock;
>> +               struct {
>> +                       spinlock_t slock;
>> +                       unsigned long lock_flags;
>> +               };
>> +       };
>>  };
>>
>>  static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
>> --
>> 2.1.4
>>
>
>Kind regards
>Uffe