[PATCH RFC 2/3] PM / Domains: Support atomic PM domains
Ulf Hansson
ulf.hansson at linaro.org
Thu Jun 11 02:41:46 PDT 2015
On 5 June 2015 at 00:29, Lina Iyer <lina.iyer at linaro.org> wrote:
> 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.
>
> 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.
>
> Cc: Ulf Hansson <ulf.hansson at linaro.org>
> Cc: Rafael J. Wysocki <rjw at rjwysocki.net>
> Cc: Kevin Hilman <khilman at linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer at linaro.org>
> ---
> drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++----------
> include/linux/pm_domain.h | 11 ++-
> 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,
Nitpick:
I would prefer a bit shorter names of all these new locking functions.
In general, I think you can remove "domain" from the names. In this
case it would mean genpd_lock_noirq().
> + 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;
This is wrong, it should be genpd->lock_flags that you assign.
BTW, can't you assign the genpd->lock_flags immediately when call
spin_lock_irqsave*(), instead of keeping a local copy?
> +
> + 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))
In general I don't like macros (as Krzysztof).
In this case I also don't see the benfit, could you consider to
convert to in-line functions instead?
> +
> 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)))
This deserves a comment in the code.
> 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);
> - 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. */
I wouldn't mind to keep some more pieces of the earlier comment, since
it explains a bit more of the *why*. Could you try to rephrase this
comment in that regard?
> + if (dev->power.irq_safe && !genpd->irq_safe)
This looks correct...
> 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;
... but this doesn't. You have already returned -EBUSY above for this case.
> +
> + 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
Let's take the opportunity to rephrase this comment a bit.
Perhaps something along the lines: "As we don't power off a non IRQ
safe PM domain which holds an IRQ safe device, we don't need to
restore the power to it."
> + * 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 */
Let's be consistent and stick with IRQ in upper case letters for the
comments. I think there are other places in the patch which needs
update as well.
> + if (genpd->irq_safe && !dev->power.irq_safe) {
> + dev_warn(dev,
dev_err(), or perhaps even WARN() ?
> + "Devices in an irq-safe domain have to be irq safe.\n");
> + 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_domain(genpd);
>
> if (genpd->prepared_count > 0) {
> ret = -EAGAIN;
> @@ -1287,7 +1366,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_domain(genpd);
>
> if (ret)
> genpd_free_dev_data(dev, gpd_data);
> @@ -1331,7 +1410,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_domain(genpd);
>
> if (genpd->prepared_count > 0) {
> ret = -EAGAIN;
> @@ -1346,14 +1425,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
> list_del_init(&pdd->list_node);
>
> - mutex_unlock(&genpd->lock);
> + genpd_unlock_domain(genpd);
>
> genpd_free_dev_data(dev, gpd_data);
>
> return 0;
>
> out:
> - mutex_unlock(&genpd->lock);
> + genpd_unlock_domain(genpd);
> dev_pm_qos_add_notifier(dev, &gpd_data->nb);
>
> return ret;
> @@ -1374,12 +1453,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.
> + */
Have you considered if/how to address this limitation?
Just to be clear, for now I think this limitation is perfectly okay,
but going forward we might see a need to mix non IRQ safe domains with
IRQ safe domains, via sub-domains.
> + if (genpd->irq_safe && !subdomain->irq_safe) {
> + pr_warn("Incompatible sub-domain %s of irq-safe domain %s\n",
> + subdomain->name, genpd->name);
Similar comment as earlier. Perhaps pr_err() or WARN() instead.
> + 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_domain(genpd);
> + genpd_lock_domain_nested(subdomain);
>
> if (genpd->status == GPD_STATE_POWER_OFF
> && subdomain->status != GPD_STATE_POWER_OFF) {
> @@ -1402,8 +1492,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_domain(subdomain);
> + genpd_unlock_domain(genpd);
>
> return ret;
> }
> @@ -1451,13 +1541,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_domain(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_domain_nested(subdomain);
>
> list_del(&link->master_node);
> list_del(&link->slave_node);
> @@ -1465,13 +1555,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_domain(subdomain);
>
> ret = 0;
> break;
> }
>
> - mutex_unlock(&genpd->lock);
> + genpd_unlock_domain(genpd);
>
> return ret;
> }
> @@ -1495,11 +1585,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) {
> + pr_err("Domain %s is irq safe, cannot attach to cpuidle\n",
> + genpd->name);
Here you have pr_err(), which is okay but not consistent. Also, I
wonder if we should use a WARN() instead?
> + return -EINVAL;
> + }
> +
> cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> if (!cpuidle_data)
> return -ENOMEM;
>
> - mutex_lock(&genpd->lock);
> + genpd_lock_domain(genpd);
>
> if (genpd->cpuidle_data) {
> ret = -EEXIST;
> @@ -1526,7 +1622,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_domain(genpd);
> return ret;
>
> err:
> @@ -1563,7 +1659,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_domain(genpd);
>
> cpuidle_data = genpd->cpuidle_data;
> if (!cpuidle_data) {
> @@ -1581,7 +1677,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
> kfree(cpuidle_data);
>
> out:
> - mutex_unlock(&genpd->lock);
> + genpd_unlock_domain(genpd);
> return ret;
> }
>
> @@ -1642,6 +1738,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
> return cb ? cb(dev) : 0;
> }
>
> +static void genpd_lock_domain_init(struct generic_pm_domain *genpd)
> +{
> + if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> + spin_lock_init(&genpd->slock);
> + genpd->irq_safe = true;
Copying the result from "genpd->flags & GENPD_FLAG_IRQ_SAFE" into the
irq_safe bool, seems like a "micro" optimization. How about only using
the genpd->flags instead?
> + } else {
> + mutex_init(&genpd->mlock);
> + genpd->irq_safe = false;
> + }
> +}
> +
> /**
> * pm_genpd_init - Initialize a generic I/O PM domain object.
> * @genpd: PM domain object to initialize.
> @@ -1657,7 +1764,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_domain_init(genpd);
> genpd->gov = gov;
> INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
> genpd->in_progress = 0;
> @@ -2042,7 +2149,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_domain_interruptible(genpd);
> if (ret)
> return -ERESTARTSYS;
>
> @@ -2062,7 +2169,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;
>
> @@ -2073,7 +2181,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>
> seq_puts(s, "\n");
> exit:
> - mutex_unlock(&genpd->lock);
> + genpd_unlock_domain(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;
> + 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
More information about the linux-arm-kernel
mailing list