From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [PATCH RFC 2/3] PM / Domains: Support atomic PM domains Date: Sun, 07 Jun 2015 18:21:04 +0900 Message-ID: <55740D00.1030203@samsung.com> References: <1433456946-53296-1-git-send-email-lina.iyer@linaro.org> <1433456946-53296-3-git-send-email-lina.iyer@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Return-path: Received: from mail-pd0-f169.google.com ([209.85.192.169]:33362 "EHLO mail-pd0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752015AbbFGJVL (ORCPT ); Sun, 7 Jun 2015 05:21:11 -0400 Received: by pdjn11 with SMTP id n11so42593259pdj.0 for ; Sun, 07 Jun 2015 02:21:10 -0700 (PDT) In-Reply-To: <1433456946-53296-3-git-send-email-lina.iyer@linaro.org> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Lina Iyer , rjw@rjwysocki.net, ulf.hansson@linaro.org, khilman@linaro.org Cc: mathieu.poirier@linaro.org, linux-pm@vger.kernel.org, galak@codeaurora.org, msivasub@codeaurora.org, agross@codeaurora.org, linux-arm-kernel@lists.infradead.org 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). > > 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? > > Cc: Ulf Hansson > Cc: Rafael J. Wysocki > Cc: Kevin Hilman > Signed-off-by: Lina Iyer > --- > drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++---------- > include/linux/pm_domain.h | 11 ++- Documentation should also be reflected. > 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. 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?). > + > 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? > - 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. Best regards, Krzysztof From mboxrd@z Thu Jan 1 00:00:00 1970 From: k.kozlowski@samsung.com (Krzysztof Kozlowski) Date: Sun, 07 Jun 2015 18:21:04 +0900 Subject: [PATCH RFC 2/3] PM / Domains: Support atomic PM domains In-Reply-To: <1433456946-53296-3-git-send-email-lina.iyer@linaro.org> References: <1433456946-53296-1-git-send-email-lina.iyer@linaro.org> <1433456946-53296-3-git-send-email-lina.iyer@linaro.org> Message-ID: <55740D00.1030203@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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). > > 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? > > Cc: Ulf Hansson > Cc: Rafael J. Wysocki > Cc: Kevin Hilman > Signed-off-by: Lina Iyer > --- > drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++---------- > include/linux/pm_domain.h | 11 ++- Documentation should also be reflected. > 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. 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?). > + > 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? > - 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. Best regards, Krzysztof