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@xxxxxxxxxx> 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@xxxxxxxxxx>
> Cc: Kevin Hilman <khilman@xxxxxxxxxx>
> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> Acked-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
