| Message ID | 1477347668-41901-9-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Hi Lina, On Tue, Oct 25, 2016 at 12:21 AM, Lina Iyer <lina.iyer@linaro.org> 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 non-IRQ > safe domains may not have IRQ-safe subdomains, but IRQ safe domains may > have IRQ safe and non-IRQ safe subdomains and devices. > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/domain.c | 111 ++++++++++++++++++++++++++++++++++++++++---- > include/linux/pm_domain.h | 10 +++- > 2 files changed, 110 insertions(+), 11 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 1ad42f2..07ed835 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = { > +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev, > + struct generic_pm_domain *genpd) > +{ > + bool ret; > + > + ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd); > + > + /* Warn once for each IRQ safe dev in no sleep domain */ This comment is not correct, as dev_warn_once() will print the warning once, not once per device. Hence users will not be informed if there are multiple IRQ safe devices. > + if (ret) > + dev_warn_once(dev, "PM domain %s will not be powered off\n", > + genpd->name); BTW, I'm seeing messages like: sh_cmt ffca0000.timer: PM domain always-on will not be powered off and sh_cmt e6138000.timer: PM domain c5 will not be powered off on various Renesas SoCs, and wanted to know if there was more than one IRQ safe device preventing a PM domain power down (answer: there isn't). Note that the above are OK, as both "always-on" and "c5" are always-on PM domains. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lina, Yes, this is a reply to an old patch. But it seems there are no upstream users of GENPD_FLAG_IRQ_SAFE yet... On Tue, Oct 25, 2016 at 12:21 AM Lina Iyer <lina.iyer@linaro.org> 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 non-IRQ > safe domains may not have IRQ-safe subdomains, but IRQ safe domains may > have IRQ safe and non-IRQ safe subdomains and devices. > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/domain.c | 111 ++++++++++++++++++++++++++++++++++++++++---- > include/linux/pm_domain.h | 10 +++- > 2 files changed, 110 insertions(+), 11 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 1ad42f2..07ed835 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = { > .unlock = genpd_unlock_mtx, > }; > > +static void genpd_lock_spin(struct generic_pm_domain *genpd) > + __acquires(&genpd->slock) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&genpd->slock, flags); > + genpd->lock_flags = flags; > +} > + > +static void genpd_lock_nested_spin(struct generic_pm_domain *genpd, > + int depth) > + __acquires(&genpd->slock) > +{ > + unsigned long flags; > + > + spin_lock_irqsave_nested(&genpd->slock, flags, depth); > + genpd->lock_flags = flags; I don't think this is correct: during the second or any subsequent call, the old genpd->lock_flags will be overwritten by the new one. As interrupts are already disabled when doing a second call, the new flags will reflect this, and calling genpd_unlock_spin() below will never re-enable interrupts again. Note that changing the above to if (!depth) genpd->lock_flags = flags; won't help, as that means the first call to genpd_unlock_spin() will re-enable interrupts, which is too early. Using a single genpd->lock_flags can work only in case interrupts were already disabled before the first call. But perhaps that's guaranteed? Apart from that, given depth is used as a lockdep subclass, and the following comment in include/linux/lockdep.h: /* * For trivial one-depth nesting of a lock-class, the following * global define can be used. (Subsystems with multiple levels * of nesting should define their own lock-nesting subclasses.) */ #define SINGLE_DEPTH_NESTING 1 I'm wondering if depth > 1 actually works as expected. Am I missing something? Thanks! > +} > + > +static int genpd_lock_interruptible_spin(struct generic_pm_domain *genpd) > + __acquires(&genpd->slock) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&genpd->slock, flags); > + genpd->lock_flags = flags; > + return 0; > +} > + > +static void genpd_unlock_spin(struct generic_pm_domain *genpd) > + __releases(&genpd->slock) > +{ > + spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags); > +} Gr{oetje,eeting}s, Geert
On Tue, 26 Feb 2019 at 16:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Lina, > > Yes, this is a reply to an old patch. > But it seems there are no upstream users of GENPD_FLAG_IRQ_SAFE yet... Correct. But I are working on adding the first ones, as you probably know. > > On Tue, Oct 25, 2016 at 12:21 AM Lina Iyer <lina.iyer@linaro.org> 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 non-IRQ > > safe domains may not have IRQ-safe subdomains, but IRQ safe domains may > > have IRQ safe and non-IRQ safe subdomains and devices. > > > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Kevin Hilman <khilman@kernel.org> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > Acked-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > drivers/base/power/domain.c | 111 ++++++++++++++++++++++++++++++++++++++++---- > > include/linux/pm_domain.h | 10 +++- > > 2 files changed, 110 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 1ad42f2..07ed835 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = { > > .unlock = genpd_unlock_mtx, > > }; > > > > +static void genpd_lock_spin(struct generic_pm_domain *genpd) > > + __acquires(&genpd->slock) > > +{ > > + unsigned long flags; > > + > > + spin_lock_irqsave(&genpd->slock, flags); > > + genpd->lock_flags = flags; > > +} > > + > > +static void genpd_lock_nested_spin(struct generic_pm_domain *genpd, > > + int depth) > > + __acquires(&genpd->slock) > > +{ > > + unsigned long flags; > > + > > + spin_lock_irqsave_nested(&genpd->slock, flags, depth); > > + genpd->lock_flags = flags; > > I don't think this is correct: during the second or any subsequent call, > the old genpd->lock_flags will be overwritten by the new one. > As interrupts are already disabled when doing a second call, the new > flags will reflect this, and calling genpd_unlock_spin() below will > never re-enable interrupts again. That seems like a correct observation, at least as far as I can tell. Thanks for looking into this! > > Note that changing the above to > > if (!depth) > genpd->lock_flags = flags; > > won't help, as that means the first call to genpd_unlock_spin() will > re-enable interrupts, which is too early. > > Using a single genpd->lock_flags can work only in case interrupts were > already disabled before the first call. But perhaps that's guaranteed? So far, the only user case I have been playing with, is for CPUs. In that case, this code in executed in the cpuidle path and thus IRQs are disabled, so yes then it works. > > Apart from that, given depth is used as a lockdep subclass, and the > following comment in include/linux/lockdep.h: > > /* > * For trivial one-depth nesting of a lock-class, the following > * global define can be used. (Subsystems with multiple levels > * of nesting should define their own lock-nesting subclasses.) > */ > #define SINGLE_DEPTH_NESTING 1 > > I'm wondering if depth > 1 actually works as expected. > > Am I missing something? I don't think so, we need to fix this problem, in one way or the other. Clearly this code hasn't been executed enough, probably only by me and Lina and for CPUs. Ideally, we should try to make it work for the generic case, as I think there are more potential users of it. [...] Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 1ad42f2..07ed835 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = { .unlock = genpd_unlock_mtx, }; +static void genpd_lock_spin(struct generic_pm_domain *genpd) + __acquires(&genpd->slock) +{ + unsigned long flags; + + spin_lock_irqsave(&genpd->slock, flags); + genpd->lock_flags = flags; +} + +static void genpd_lock_nested_spin(struct generic_pm_domain *genpd, + int depth) + __acquires(&genpd->slock) +{ + unsigned long flags; + + spin_lock_irqsave_nested(&genpd->slock, flags, depth); + genpd->lock_flags = flags; +} + +static int genpd_lock_interruptible_spin(struct generic_pm_domain *genpd) + __acquires(&genpd->slock) +{ + unsigned long flags; + + spin_lock_irqsave(&genpd->slock, flags); + genpd->lock_flags = flags; + return 0; +} + +static void genpd_unlock_spin(struct generic_pm_domain *genpd) + __releases(&genpd->slock) +{ + spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags); +} + +static const struct genpd_lock_ops genpd_spin_ops = { + .lock = genpd_lock_spin, + .lock_nested = genpd_lock_nested_spin, + .lock_interruptible = genpd_lock_interruptible_spin, + .unlock = genpd_unlock_spin, +}; + #define genpd_lock(p) p->lock_ops->lock(p) #define genpd_lock_nested(p, d) p->lock_ops->lock_nested(p, d) #define genpd_lock_interruptible(p) p->lock_ops->lock_interruptible(p) #define genpd_unlock(p) p->lock_ops->unlock(p) +#define genpd_is_irq_safe(genpd) (genpd->flags & GENPD_FLAG_IRQ_SAFE) + +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev, + struct generic_pm_domain *genpd) +{ + bool ret; + + ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd); + + /* Warn once for each IRQ safe dev in no sleep domain */ + if (ret) + dev_warn_once(dev, "PM domain %s will not be powered off\n", + genpd->name); + + return ret; +} + /* * Get the generic PM domain for a particular struct device. * This validates the struct device pointer, the PM domain pointer, @@ -343,7 +402,12 @@ static int genpd_poweroff(struct generic_pm_domain *genpd, bool is_async) if (stat > PM_QOS_FLAGS_NONE) return -EBUSY; - if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe) + /* + * Do not allow PM domain to be powered off, when an IRQ safe + * device is part of a non-IRQ safe domain. + */ + if (!pm_runtime_suspended(pdd->dev) || + irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd)) not_suspended++; } @@ -506,10 +570,10 @@ static int 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 + * IRQs disabled, so suspend only if the PM domain also is irq_safe. */ - if (dev->power.irq_safe) + if (irq_safe_dev_in_no_sleep_domain(dev, genpd)) return 0; genpd_lock(genpd); @@ -543,8 +607,11 @@ static int 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 don't power off a non IRQ safe domain, which holds + * an IRQ safe device, we don't need to restore power to it. + */ + if (irq_safe_dev_in_no_sleep_domain(dev, genpd)) { timed = false; goto out; } @@ -586,7 +653,8 @@ static int genpd_runtime_resume(struct device *dev) err_stop: genpd_stop_dev(genpd, dev); err_poweroff: - if (!dev->power.irq_safe) { + if (!pm_runtime_is_irq_safe(dev) || + (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) { genpd_lock(genpd); genpd_poweroff(genpd, 0); genpd_unlock(genpd); @@ -1223,6 +1291,17 @@ static int 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_is_irq_safe(genpd) && genpd_is_irq_safe(subdomain)) { + WARN("Parent %s of subdomain %s must be IRQ safe\n", + genpd->name, subdomain->name); + return -EINVAL; + } + link = kzalloc(sizeof(*link), GFP_KERNEL); if (!link) return -ENOMEM; @@ -1337,6 +1416,17 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd) return 0; } +static void genpd_lock_init(struct generic_pm_domain *genpd) +{ + if (genpd->flags & GENPD_FLAG_IRQ_SAFE) { + spin_lock_init(&genpd->slock); + genpd->lock_ops = &genpd_spin_ops; + } else { + mutex_init(&genpd->mlock); + genpd->lock_ops = &genpd_mtx_ops; + } +} + /** * pm_genpd_init - Initialize a generic I/O PM domain object. * @genpd: PM domain object to initialize. @@ -1356,8 +1446,7 @@ int 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->mlock); - genpd->lock_ops = &genpd_mtx_ops; + genpd_lock_init(genpd); genpd->gov = gov; INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn); atomic_set(&genpd->sd_count, 0); @@ -2131,7 +2220,9 @@ 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_is_irq_safe(genpd) ? + GFP_ATOMIC : GFP_KERNEL); if (kobj_path == NULL) continue; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 811b968..81ece61 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -15,9 +15,11 @@ #include <linux/err.h> #include <linux/of.h> #include <linux/notifier.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 */ @@ -76,7 +78,13 @@ struct generic_pm_domain { unsigned int state_idx; /* state that genpd will go to when off */ void *free; /* Free the state that was allocated for default */ const struct genpd_lock_ops *lock_ops; - struct mutex mlock; + union { + struct mutex mlock; + struct { + spinlock_t slock; + unsigned long lock_flags; + }; + }; };