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

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On 11.06.2015 01:13, Lina Iyer wrote:
> On Sun, Jun 07 2015 at 03:21 -0600, Krzysztof Kozlowski wrote:
>> W dniu 05.06.2015 o 07:29, Lina Iyer pisze:
>>> Power Domains currently support turning on/off only in process context.
>>> This restricts the usage of PM domains to devices and domains that
>>> could be powered on/off in irq disabled contexts as the mutexes used in
>>> GenPD allows for cpu sleep while waiting for locks.
>>
>> I can find also other use case: currently the power domain with irq_safe
>> devices is always powered on. With the patch it could be powered off (of
>> course if the driver/mach code is irq-safe).
>>
> Yes, absolutely.
> 
>>>
>>> Genpd inherently provides support for devices, domain hierarchy and can
>>> be used to represent cpu clusters like in ARM's big.Little, where, each
>>> cpu cluster is in its domain, with supporting caches and other
>>> peripheral hardware. Multiple such domains could be part of another
>>> domain. Because mutexes are used to protect and synchronize domain
>>> operations and cpu idle operations are inherently atomic, the use of
>>> genpd is not possible for runtime suspend and resume of the pm domain.
>>> Replacing the locks to spinlocks would allow cpu domain to be be powered
>>> off to save power, when all the cpus are powered off.
>>>
>>> However, not all domains can operate in irq-safe contexts and usually
>>> would need to sleep during domain operations. So genpd has to support
>>> both the cases, where the domain is or is not irq-safe. The irq-safe
>>> attribute is therefore domain specific.
>>>
>>> To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag
>>> while defining the domain. This determines if the domain should use a
>>> spinlock instead of a mutex. Locking is abstracted through
>>> genpd_lock_domain() and genpd_unlock_domain() functions that use the
>>> flag to determine the locking to be used for this domain.
>>>
>>> The restriction this imposes on the domain hierarchy is that subdomains
>>> and all devices in the hierarchy also be irq-safe. Non irq-safe domains
>>> may continue to have irq-safe devices, but not the other way around.
>>
>> So an irq-safe device can be put in irq-safe subdomain which can be a
>> child of non-irq-safe topdomain?
>>
> Yes, the container need not be irq-safe but the contained need to be
> irqsafe.

I had doubts about possibility of grabbing any lock from irq-safe
subdomain before grabbing lock from non-irq-safe parent. That would mean
that spin_lock is acquired and then mutex. But now after giving some
more thoughts I can't find such case so it seems fine.

Thanks for explanation.

> 
>>>
>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>> Cc: Kevin Hilman <khilman@linaro.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>>  drivers/base/power/domain.c | 200
>>> ++++++++++++++++++++++++++++++++++----------
>>>  include/linux/pm_domain.h   |  11 ++-
>>
>> Documentation should also be reflected.
>>
> Yes, will add.
> 
>>>  2 files changed, 164 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index dfd7595..8b89d15 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -50,6 +50,71 @@
>>>  static LIST_HEAD(gpd_list);
>>>  static DEFINE_MUTEX(gpd_list_lock);
>>>
>>> +static inline int genpd_lock_domain_noirq(struct generic_pm_domain
>>> *genpd,
>>> +                    unsigned int subclass)
>>> +    __acquires(&genpd->slock)
>>> +{
>>> +    unsigned long flags;
>>> +
>>> +    if (unlikely(subclass > 0))
>>> +        spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
>>> +    else
>>> +        spin_lock_irqsave(&genpd->slock, flags);
>>> +
>>> +    genpd->flags = flags;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static inline int genpd_unlock_domain_noirq(struct generic_pm_domain
>>> *genpd)
>>> +    __releases(&genpd->slock)
>>> +{
>>> +    spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
>>> +    return 0;
>>> +}
>>> +
>>> +static inline int genpd_lock_domain_irq(struct generic_pm_domain
>>> *genpd,
>>> +                    unsigned int subclass)
>>> +    __acquires(&genpd->mlock)
>>> +{
>>> +    if (unlikely(subclass > 0))
>>> +        mutex_lock_nested(&genpd->mlock, subclass);
>>> +    else
>>> +        mutex_lock(&genpd->mlock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static inline int genpd_lock_domain_interruptible_irq(
>>> +                struct generic_pm_domain *genpd)
>>> +    __acquires(&genpd->mlock)
>>> +{
>>> +    return mutex_lock_interruptible(&genpd->mlock);
>>> +}
>>> +
>>> +static inline int genpd_unlock_domain_irq(struct generic_pm_domain
>>> *genpd)
>>> +    __releases(&genpd->mlock)
>>> +{
>>> +    mutex_unlock(&genpd->mlock);
>>> +    return 0;
>>> +}
>>> +
>>> +#define genpd_lock_domain(genpd)                \
>>> +    (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
>>> +            : genpd_lock_domain_irq(genpd, 0))
>>> +
>>> +#define genpd_lock_domain_nested(genpd)                \
>>> +    (genpd->irq_safe ? genpd_lock_domain_noirq(genpd,
>>> SINGLE_DEPTH_NESTING)\
>>> +            : genpd_lock_domain_irq(genpd, SINGLE_DEPTH_NESTING))
>>> +
>>> +#define genpd_unlock_domain(genpd)                \
>>> +    (genpd->irq_safe ? genpd_unlock_domain_noirq(genpd)    \
>>> +            : genpd_unlock_domain_irq(genpd))
>>> +
>>> +#define genpd_lock_domain_interruptible(genpd)            \
>>> +    (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
>>> +            : genpd_lock_domain_interruptible_irq(genpd))
>>
>> Why macros? You are not using here benefits of a macro and they are
>> called just like ordinary functions.
>>
> Well, I didnt see a need for a function that might show up in the stack.
> But I have no strong preference either way.

That is actually a valid reason. It is just my personal dislike of
macros so I am fine with your idea.

> 
>> You added "domain" prefix but genpd already contains this. genod_lock(),
>> genpd_lock_nested() etc. should be sufficient, unless there is a
>> conflict, similar name planned or you plan to lock something else
>> (genpd_lock_device?).
>>
> Sigh. Yes, you are right. Its redundant. Will remove.
> 
>>
>>> +
>>>  static struct generic_pm_domain *pm_genpd_lookup_name(const char
>>> *domain_name)
>>>  {
>>>      struct generic_pm_domain *genpd = NULL, *gpd;
>>> @@ -262,9 +327,9 @@ int pm_genpd_poweron(struct generic_pm_domain
>>> *genpd)
>>>  {
>>>      int ret;
>>>
>>> -    mutex_lock(&genpd->lock);
>>> +    genpd_lock_domain(genpd);
>>>      ret = __pm_genpd_poweron(genpd);
>>> -    mutex_unlock(&genpd->lock);
>>> +    genpd_unlock_domain(genpd);
>>>      return ret;
>>>  }
>>>
>>> @@ -326,9 +391,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_domain(genpd);
>>>              genpd->max_off_time_changed = true;
>>> -            mutex_unlock(&genpd->lock);
>>> +            genpd_unlock_domain(genpd);
>>>          }
>>>
>>>          dev = dev->parent;
>>> @@ -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)))
>>>              not_suspended++;
>>>      }
>>>
>>> @@ -453,9 +518,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_domain(genpd);
>>>      pm_genpd_poweroff(genpd);
>>
>> Ipm_genpd_poweroff() calls __pm_genpd_save_device() which grabs mutex.
>> At least in next-20150604 but maybe the patches, which this depends on,
>> changed it?
>>
> Yes. Ulf's patch remvoed that call.

OK, thanks.

> 
>>
>>> -    mutex_unlock(&genpd->lock);
>>> +    genpd_unlock_domain(genpd);
>>>  }
>>>
>>>  /**
>>> @@ -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. */
>>> +    if (dev->power.irq_safe && !genpd->irq_safe)
>>>          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;
>>> +
>>> +    genpd_lock_domain(genpd);
>>> +
>>>      genpd->in_progress++;
>>>      pm_genpd_poweroff(genpd);
>>>      genpd->in_progress--;
>>> -    mutex_unlock(&genpd->lock);
>>> +    genpd_unlock_domain(genpd);
>>>
>>>      return 0;
>>>  }
>>> @@ -528,13 +597,16 @@ 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)
>>> +    /*
>>> +     * If power.irq_safe and domain is not, then
>>> +     * the PM domain is never powered off.
>>> +     */
>>> +    if (dev->power.irq_safe && !genpd->irq_safe)
>>>          return genpd_start_dev_no_timing(genpd, dev);
>>>
>>> -    mutex_lock(&genpd->lock);
>>> +    genpd_lock_domain(genpd);
>>>      ret = __pm_genpd_poweron(genpd);
>>> -    mutex_unlock(&genpd->lock);
>>> +    genpd_unlock_domain(genpd);
>>>
>>>      if (ret)
>>>          return ret;
>>> @@ -729,14 +801,14 @@ static int pm_genpd_prepare(struct device *dev)
>>>      if (resume_needed(dev, genpd))
>>>          pm_runtime_resume(dev);
>>>
>>> -    mutex_lock(&genpd->lock);
>>> +    genpd_lock_domain(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_domain(genpd);
>>>
>>>      if (genpd->suspend_power_off) {
>>>          pm_runtime_put_noidle(dev);
>>> @@ -754,12 +826,12 @@ static int pm_genpd_prepare(struct device *dev)
>>>
>>>      ret = pm_generic_prepare(dev);
>>>      if (ret) {
>>> -        mutex_lock(&genpd->lock);
>>> +        genpd_lock_domain(genpd);
>>>
>>>          if (--genpd->prepared_count == 0)
>>>              genpd->suspend_power_off = false;
>>>
>>> -        mutex_unlock(&genpd->lock);
>>> +        genpd_unlock_domain(genpd);
>>>          pm_runtime_enable(dev);
>>>      }
>>>
>>> @@ -1116,13 +1188,13 @@ static void pm_genpd_complete(struct device
>>> *dev)
>>>      if (IS_ERR(genpd))
>>>          return;
>>>
>>> -    mutex_lock(&genpd->lock);
>>> +    genpd_lock_domain(genpd);
>>>
>>>      run_complete = !genpd->suspend_power_off;
>>>      if (--genpd->prepared_count == 0)
>>>          genpd->suspend_power_off = false;
>>>
>>> -    mutex_unlock(&genpd->lock);
>>> +    genpd_unlock_domain(genpd);
>>>
>>>      if (run_complete) {
>>>          pm_generic_complete(dev);
>>> @@ -1266,11 +1338,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 */
>>
>> Why? Can you add this information here? Previously there was a reason in
>> case of irq_safe devices which you removed leaving only policy.
>>
> Sorry, your question is not clear to me.
> I believe this is a new requirement that enforces the contained devices
> of an irq-safe domain to be irq-safe as well.

What I wanted to say is that it would be nice if comment explained why
domain have to be IRQ safe too. Without this "WHY" answer the comment is
quite redundant - the "if" statement is obvious. But the "WHY" is not
such obvious.

Previous comments in few places mentioned the answer:
/*
 * 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.
 */

Best regards,
Krzysztof