Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs
From: Rafael J. Wysocki
Date: Wed Nov 23 2016 - 17:37:54 EST
On Fri, Nov 18, 2016 at 9:18 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> Hi,
>
> * Rafael J. Wysocki <rafael@xxxxxxxxxx> [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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> ---
> 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