linux-kernel - Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 23 Nov 2016 23:37:46 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Brian Norris <briannorris@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Brian Norris <computersforpeace@...il.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs

On Fri, Nov 18, 2016 at 9:18 PM, Tony Lindgren <tony@...mide.com> wrote:
> Hi,
>
> * Rafael J. Wysocki <rafael@...nel.org> [161111 16:35]:
>> However, my understanding is that the current code actually works for
>> runtime PM just fine.
>
> Hmm well I just noticed that for drivers not using autosuspend it can be
> flakey, see the patch below. That probably explains some mysteries people
> are seeing with wakeirqs.
>
> Do you have any better ideas for setting wirq->active on the first
> rpm_suspend()?

You could change dedicated_irq into status and have three values for
it: INVALID, ALLOCATED and IN_USE.

dev_pm_set_dedicated_wake_irq() would make it ALLOCATED and
dev_pm_check_wake_irq() would change it into IN_USE.  In turn,
dev_pm_enable/disable_wake_irq() would only touch it if it is IN_USE
and dev_pm_clear_wake_irq() would free it if it is not INVALID.

>> What Brian seems to be wanting is to make system resume synchronize
>> the wakeup interrupt at one point, so maybe there could be a "sync"
>> version of dev_pm_disable_wake_irq() to be invoked then?
>
> We call rpm_resume() from handle_threaded_wake_irq(), that's no better :)
>
> Regards,
>
> Tony
>
> 8< -------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@...mide.com>
> Date: Fri, 18 Nov 2016 10:15:34 -0800
> Subject: [PATCH] PM / wakeirq: Fix wakeirq init
>
> I noticed some wakeirq flakeyness with consumer drivers not using
> autosuspend. For drivers not using autosuspend, the wakeirq may never
> get unmasked in rpm_suspend() because of irq desc->depth.
>
> We are configuring dedicated wakeirqs to start with IRQ_NOAUTOEN as we
> naturally don't want them running until rpm_suspend() is called.
>
> However, when a consumer driver calls pm_runtime_get functions, we now
> wrongly start with disable_irq_nosync() call on the dedicated wakeirq
> that is disabled to start with.
>
> This causes desc->depth to toggle between 1 and 2 instead of the usual
> 0 and 1. This can prevent enable_irq() from unmasking the wakeirq as
> that only happens at desc->depth 1.
>
> This does not necessarily show up with drivers using autosuspend as
> there is time for disable_irq_nosync() before rpm_suspend() gets called
> after the autosuspend timeout.
>
> Fix the issue by adding wirq->active flag that lazily gets set on
> the first rpm_suspend().
>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
>  drivers/base/power/power.h   | 19 +++++++++++++++++++
>  drivers/base/power/runtime.c |  1 +
>  drivers/base/power/wakeirq.c | 10 ++++------
>  3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -24,9 +24,24 @@ extern void pm_runtime_remove(struct device *dev);
>  struct wake_irq {
>         struct device *dev;
>         int irq;
> +       bool active:1;
>         bool dedicated_irq:1;
>  };
>
> +/* Caller must hold &dev->power.lock to change wirq->active */
> +static inline void dev_pm_check_wake_irq(struct device *dev)
> +{
> +       struct wake_irq *wirq = dev->power.wakeirq;
> +
> +       if (!wirq)
> +               return;
> +
> +       if (unlikely(!wirq->active)) {
> +               wirq->active = true;
> +               wmb();  /* ensure dev_pm_enable_wake_irq() sees active */

smp_wmb()?

Also, do we have a corresponding barrier on the reader side?

> +       }
> +}
> +
>  extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
>  extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
>
> @@ -96,6 +111,10 @@ static inline void wakeup_sysfs_remove(struct device *dev) {}
>  static inline int pm_qos_sysfs_add(struct device *dev) { return 0; }
>  static inline void pm_qos_sysfs_remove(struct device *dev) {}
>
> +static inline void dev_pm_check_wake_irq(struct device *dev)
> +{
> +}
> +
>  static inline void dev_pm_arm_wake_irq(struct wake_irq *wirq)
>  {
>  }
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -592,6 +592,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>
> +       dev_pm_check_wake_irq(dev);

I wonder if it would make sense to fold dev_pm_check_wake_irq() into
dev_pm_enable_wake_irq()?

>         dev_pm_enable_wake_irq(dev);
>         retval = rpm_callback(callback, dev);
>         if (retval)
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> --- a/drivers/base/power/wakeirq.c
> +++ b/drivers/base/power/wakeirq.c
> @@ -212,8 +212,7 @@ EXPORT_SYMBOL_GPL(dev_pm_set_dedicated_wake_irq);
>   * dev_pm_enable_wake_irq - Enable device wake-up interrupt
>   * @dev: Device
>   *
> - * Called from the bus code or the device driver for
> - * runtime_suspend() to enable the wake-up interrupt while
> + * Called from rpm_suspend() to enable the wake-up interrupt while
>   * the device is running.
>   *
>   * Note that for runtime_suspend()) the wake-up interrupts
> @@ -224,7 +223,7 @@ void dev_pm_enable_wake_irq(struct device *dev)
>  {
>         struct wake_irq *wirq = dev->power.wakeirq;
>
> -       if (wirq && wirq->dedicated_irq)
> +       if (wirq && wirq->dedicated_irq && wirq->active)
>                 enable_irq(wirq->irq);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
> @@ -233,15 +232,14 @@ EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
>   * dev_pm_disable_wake_irq - Disable device wake-up interrupt
>   * @dev: Device
>   *
> - * Called from the bus code or the device driver for
> - * runtime_resume() to disable the wake-up interrupt while
> + * Called from rpm_resume() to disable the wake-up interrupt while
>   * the device is running.
>   */
>  void dev_pm_disable_wake_irq(struct device *dev)
>  {
>         struct wake_irq *wirq = dev->power.wakeirq;
>
> -       if (wirq && wirq->dedicated_irq)
> +       if (wirq && wirq->dedicated_irq && wirq->active)
>                 disable_irq_nosync(wirq->irq);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
> --

Thanks,
Rafael