From: k.kozlowski@samsung•com (Krzysztof Kozlowski)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH RFC 2/3] PM / Domains: Support atomic PM domains
Date: Sun, 07 Jun 2015 18:21:04 +0900 [thread overview]
Message-ID: <55740D00.1030203@samsung.com> (raw)
In-Reply-To: <1433456946-53296-3-git-send-email-lina.iyer@linaro.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 <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.
> 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
next prev parent reply other threads:[~2015-06-07 9:21 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-04 22:29 [PATCH RFC 0/3] PM / Domains: Generic PM domains for cpus Lina Iyer
2015-06-04 22:29 ` [PATCH RFC 1/3] PM / Domains: Allocate memory outside domain locks Lina Iyer
2015-06-07 8:35 ` Krzysztof Kozlowski
2015-06-09 22:45 ` Lina Iyer
2015-06-10 17:33 ` Kevin Hilman
2015-06-04 22:29 ` [PATCH RFC 2/3] PM / Domains: Support atomic PM domains Lina Iyer
2015-06-07 9:21 ` Krzysztof Kozlowski [this message]
2015-06-10 16:13 ` Lina Iyer
2015-06-11 0:13 ` Krzysztof Kozlowski
2015-06-11 14:33 ` Lina Iyer
2015-06-10 18:04 ` Kevin Hilman
2015-06-10 20:35 ` Lina Iyer
2015-06-11 9:41 ` Ulf Hansson
2015-06-11 19:47 ` Lina Iyer
2015-06-11 21:13 ` Ulf Hansson
2015-06-04 22:29 ` [PATCH RFC 3/3] PM / Domains: Introduce generic PM domain for cpu domain Lina Iyer
2015-06-07 9:42 ` Krzysztof Kozlowski
2015-06-10 16:57 ` Lina Iyer
2015-06-11 0:27 ` Krzysztof Kozlowski
2015-06-11 14:42 ` Lina Iyer
2015-06-10 17:01 ` Kevin Hilman
2015-06-11 0:35 ` Krzysztof Kozlowski
2015-06-10 21:37 ` Kevin Hilman
2015-06-11 14:56 ` Lina Iyer
2015-06-15 18:43 ` Kevin Hilman
2015-06-15 19:14 ` Lina Iyer
2015-06-16 15:50 ` Kevin Hilman
2015-06-10 17:24 ` [PATCH RFC 0/3] PM / Domains: Generic PM domains for cpus Kevin Hilman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55740D00.1030203@samsung.com \
--to=k.kozlowski@samsung$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox