Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755935AbbESNj1 (ORCPT ); Tue, 19 May 2015 09:39:27 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:59527 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751928AbbESNjZ (ORCPT ); Tue, 19 May 2015 09:39:25 -0400 From: "Rafael J. Wysocki" To: Tony Lindgren Cc: Felipe Balbi , "Rafael J. Wysocki" , Alan Stern , Andreas Fenkart , Greg Kroah-Hartman , Huiquan Zhong , Kevin Hilman , NeilBrown , Mika Westerberg , Nishanth Menon , Peter Hurley , Sebastian Andrzej Siewior , Ulf Hansson , Thomas Gleixner , linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, linux-omap@vger.kernel.org Subject: Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling Date: Tue, 19 May 2015 16:04:43 +0200 Message-ID: <3554198.gIGuGWK14O@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.0.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150518234400.GN10274@atomide.com> References: <1431560196-5722-1-git-send-email-tony@atomide.com> <20150518220459.GJ10274@atomide.com> <20150518234400.GN10274@atomide.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit 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: 5445 Lines: 190 On Monday, May 18, 2015 04:44:01 PM Tony Lindgren wrote: > * Tony Lindgren [150518 15:06]: > > +/** > > + * dev_pm_set_wake_irq - Attach device IO interrupt as wake IRQ > > + * @dev: Device entry > > + * @irq: Device IO interrupt > > + * > > + * Attach a device IO interrupt as a wake IRQ. The wake IRQ gets > > + * automatically configured for wake-up from suspend based > > +void dev_pm_clear_wake_irq(struct device *dev) > > +{ > > + struct wake_irq *wirq = dev->power.wakeirq; > > + unsigned long flags; > > + > > + if (!wirq) > > + return; > > + > > + device_wakeup_detach_irq(dev); > > + > > + spin_lock_irqsave(&dev->power.lock, flags); > > + if (wirq->manage_irq) { > > + free_irq(wirq->irq, wirq); > > + wirq->manage_irq = false; > > + } > > + dev->power.wakeirq = NULL; > > + spin_unlock_irqrestore(&dev->power.lock, flags); > > + > > + wirq->irq = -EINVAL; > > + kfree(wirq); > > +} > > I just noticed most of the dev_pm_clear_wake_irq is no longer needed. > We're now freeing it anyways. so it can be just: > > void dev_pm_clear_wake_irq(struct device *dev) > { > struct wake_irq *wirq = dev->power.wakeirq; > unsigned long flags; > > if (!wirq) > return; > > spin_lock_irqsave(&dev->power.lock, flags); > dev->power.wakeirq = NULL; > spin_unlock_irqrestore(&dev->power.lock, flags); > > device_wakeup_detach_irq(dev); > if (wirq->manage_irq) > free_irq(wirq->irq, wirq); > kfree(wirq); > } > > > Regards, > > Tony > > 8< --------------------- > From: Tony Lindgren > Date: Mon, 18 May 2015 15:40:29 -0700 > Subject: [PATCH] PM / Wakeirq: Add automated device wake IRQ handling > > Turns out we can automate the handling for the device_may_wakeup() > quite a bit by using the kernel wakeup source list. > > And as some hardware has separate dedicated wake-up interrupt > in addition to the IO interrupt, we can automate the handling by > adding a generic threaded interrupt handler that just calls the > device PM runtime to wake up the device. > > This allows dropping code from device drivers as we currently > are doing it in multiple ways, and often wrong. > > For most drivers, we should be able to drop the following > boilerplate code from runtime_suspend and runtime_resume > functions: > > ... > device_init_wakeup(dev, true); > ... > if (device_may_wakeup(dev) > enable_irq_wake(irq); > ... > if (device_may_wakeup(dev) > enable_irq_wake(irq); Closing parens are missin in the above two if () statements. Also, should the second one be disable_irq_wake(irq)? > ... > device_init_wakeup(dev, false); > ... > > We can replace it with just the following init and exit > time code: > > ... > device_init_wakeup(dev, true); > dev_pm_set_wake_irq(dev, irq); > ... > dev_pm_clear_wake_irq(dev); > device_init_wakeup(dev, false); > ... > > And for hardware with dedicated wake-up interrupts: > > ... > device_init_wakeup(dev, true); > dev_pm_set_dedicated_wake_irq(dev, irq); > ... > dev_pm_clear_wake_irq(dev); > device_init_wakeup(dev, false); > ... > > For now, let's only enable it for select PM_WAKEIRQ. Why? What would be wrong with doing that unconditionally? > Signed-off-by: Tony Lindgren Looks good overall, a couple of nits below. [cut] > +/** > + * handle_threaded_wake_irq - Handler for dedicated wake-up interrupts > + * @irq: Device dedicated wake-up interrupt > + * @_wirq: Wake IRQ data > + * > + * Some devices have a separate wake-up interrupt in addition to the > + * device IO interrupt. The wake-up interrupts signal that the device > + * should be woken up from a idle state. This handler uses device > + * specific pm_runtime functions to wake the device and then it's > + * up to the device to do whatever it needs to. Note as the device > + * may need to restore context and start up regulators, we use a > + * threaded IRQ. > + * > + * Also note that we are not resending the lost device interrupts. > + * We assume that the wake-up interrupt just needs to wake-up the > + * device, and the device pm_runtime_resume() can deal with the > + * situation. > + */ > +static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq) > +{ > + struct wake_irq *wirq = _wirq; > + > + /* We don't want RPM_ASYNC or RPM_NOWAIT here */ > + return pm_runtime_resume(wirq->dev) ? IRQ_NONE : IRQ_HANDLED; There are various reasons why pm_runtime_resume() may return error codes and some of them don't mean that the interrupt was not legitimate. Moreover, it returns 1 if the device is already active, in which case the above check won't do any good to us. Why not to return IRQ_HANDLED unconditionally from here? [cut] > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig > index 7e01f78..d3735bd 100644 > --- a/kernel/power/Kconfig > +++ b/kernel/power/Kconfig > @@ -267,6 +267,10 @@ config PM_CLK > def_bool y > depends on PM && HAVE_CLK > > +config PM_WAKEIRQ > + bool > + depends on PM_SLEEP > + If you really really want this (I'm still not sure why exactly, though), it should depend on PM_SLEEP || PM_RUNTIME, because the latter uses it too. > config PM_GENERIC_DOMAINS > bool > depends on PM Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/