Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934824AbcKWWhv (ORCPT ); Wed, 23 Nov 2016 17:37:51 -0500 Received: from mail-wj0-f196.google.com ([209.85.210.196]:33380 "EHLO mail-wj0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933302AbcKWWht (ORCPT ); Wed, 23 Nov 2016 17:37:49 -0500 MIME-Version: 1.0 In-Reply-To: <20161118201823.GX4082@atomide.com> References: <20161110184910.GA135921@google.com> <20161111163147.GC7138@atomide.com> <20161111222911.GL7138@atomide.com> <20161111223234.GM7138@atomide.com> <20161112001917.GN7138@atomide.com> <20161118201823.GX4082@atomide.com> From: "Rafael J. Wysocki" Date: Wed, 23 Nov 2016 23:37:46 +0100 X-Google-Sender-Auth: vdlqjGyV-e94GjLgNLi5Sjr39qU Message-ID: Subject: Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs To: Tony Lindgren Cc: "Rafael J. Wysocki" , Brian Norris , Dmitry Torokhov , "Rafael J . Wysocki" , Pavel Machek , Len Brown , Greg Kroah-Hartman , lkml , Brian Norris , "linux-pm@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6131 Lines: 170 On Fri, Nov 18, 2016 at 9:18 PM, Tony Lindgren wrote: > Hi, > > * Rafael J. Wysocki [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 > 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 > --- > 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