Date:	Wed, 13 May 2015 21:06:34 -0500
From:	Felipe Balbi <balbi@...com>
To:	Tony Lindgren <tony@...mide.com>
CC:	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Andreas Fenkart <afenkart@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Felipe Balbi <balbi@...com>,
	Huiquan Zhong <huiquan.zhong@...el.com>,
	Kevin Hilman <khilman@...nel.org>, NeilBrown <neilb@...e.de>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Nishanth Menon <nm@...com>,
	Peter Hurley <peter@...leysoftware.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-serial@...r.kernel.org>, <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling

Hi,

On Wed, May 13, 2015 at 04:36:33PM -0700, Tony Lindgren wrote:
> 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:
> 
> 	...
> 	if (device_may_wakeup(dev)
> 		enable_irq_wake(irq);
> 	...
> 	if (device_may_wakeup(dev)
> 		enable_irq_wake(irq);
> 	...
> 
> 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_request_wake_irq(dev, wakeirq, NULL, 0, NULL);
> 	...
> 	dev_pm_enable_wake_irq(dev);
> 	...
> 	dev_pm_disable_wake_irq(dev);
> 	...
> 	dev_pm_free_wake_irq(dev);
> 	device_init_wakeup(dev, false);
> 	...
> 
> For now, let's only enable it for select PM_WAKEIRQ.
> 
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
>  arch/arm/mach-omap2/Kconfig  |   1 +
>  drivers/base/power/Makefile  |   1 +
>  drivers/base/power/main.c    |   2 +
>  drivers/base/power/power.h   |  38 ++++++
>  drivers/base/power/wakeirq.c | 316 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/base/power/wakeup.c  |  96 +++++++++++++
>  include/linux/pm.h           |   2 +
>  include/linux/pm_wakeirq.h   |  72 ++++++++++
>  include/linux/pm_wakeup.h    |   7 +
>  kernel/power/Kconfig         |   4 +
>  10 files changed, 539 insertions(+)
>  create mode 100644 drivers/base/power/wakeirq.c
>  create mode 100644 include/linux/pm_wakeirq.h
> 
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index 6468f15..ac7b570 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -85,6 +85,7 @@ config ARCH_OMAP2PLUS
>  	select OMAP_DM_TIMER
>  	select OMAP_GPMC
>  	select PINCTRL
> +	select PM_WAKEIRQ if PM_SLEEP
>  	select SOC_BUS
>  	select TI_PRIV_EDMA
>  	select OMAP_IRQCHIP

seems like enabling this for OMAP should be a patch of its own.

> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index 1cb8544..527546e 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -4,5 +4,6 @@ obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
>  obj-$(CONFIG_PM_OPP)	+= opp.o
>  obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
>  obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
> +obj-$(CONFIG_PM_WAKEIRQ)	+= wakeirq.o
>  
>  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 3d874ec..24515e7 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -587,6 +587,7 @@ void dpm_resume_noirq(pm_message_t state)
>  	async_synchronize_full();
>  	dpm_show_time(starttime, state, "noirq");
>  	resume_device_irqs();
> +	device_wakeup_disarm_wakeirqs();
>  	cpuidle_resume();
>  	trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, false);
>  }
> @@ -1104,6 +1105,7 @@ int dpm_suspend_noirq(pm_message_t state)
>  
>  	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
>  	cpuidle_pause();
> +	device_wakeup_arm_wakeirqs();
>  	suspend_device_irqs();
>  	mutex_lock(&dpm_list_mtx);
>  	pm_transition = state;
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index b6b8a27..6183c5d 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -20,6 +20,44 @@ static inline void pm_runtime_early_init(struct device *dev)
>  extern void pm_runtime_init(struct device *dev);
>  extern void pm_runtime_remove(struct device *dev);
>  
> +#ifdef CONFIG_PM_WAKEIRQ
> +
> +extern int device_wakeup_attach_irq(struct device *dev,
> +				    struct wake_irq *wakeirq);
> +extern void device_wakeup_detach_irq(struct device *dev);
> +extern void device_wakeup_arm_wakeirqs(void);
> +extern void device_wakeup_disarm_wakeirqs(void);
> +
> +extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
> +extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
> +
> +#else
> +
> +static inline int
> +device_wakeup_attach_irq(struct device *dev,
> +			 struct wake_irq *wakeirq)
> +{
> +	return 0;
> +}
> +
> +static inline void device_wakeup_detach_irq(struct device *dev)
> +{
> +}
> +
> +static inline void device_wakeup_arm_wakeirqs(void)
> +{
> +}
> +
> +static inline void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> +{
> +}
> +
> +static inline void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> +{
> +}
> +
> +#endif /* CONFIG_PM_WAKEIRQ */
> +
>  /*
>   * sysfs.c
>   */
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> new file mode 100644
> index 0000000..579157b
> --- /dev/null
> +++ b/drivers/base/power/wakeirq.c
> @@ -0,0 +1,316 @@
> +/*
> + * wakeirq.c - Device wakeirq helper functions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
> +
> +#include "power.h"
> +
> +/**
> + * dev_pm_attach_wake_irq - Attach device interrupt as a wake IRQ
> + * @dev: Device entry
> + * @irq: Device wake-up capable interrupt
> + * @wirq: Wake irq specific data
> + *
> + * Internal function to attach either a device IO interrupt or a
> + * dedicated wake-up interrupt as a wake IRQ.
> + */
> +static int dev_pm_attach_wake_irq(struct device *dev, int irq,
> +				  struct wake_irq *wirq)
> +{
> +	unsigned long flags;
> +	int err;
> +
> +	if (!dev || !wirq)
> +		return -EINVAL;
> +
> +	if (!dev->power.wakeup) {
> +		dev_err(dev, "forgot to call call device_init_wakeup?\n");
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	if (WARN_ON(dev->power.wakeirq)) {
> +		dev_err(dev, "wake irq already initialized\n");
> +		err = -EEXIST;
> +		goto err_unlock;
> +	}
> +
> +	dev->power.wakeirq = wirq;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +
> +	err = device_wakeup_attach_irq(dev, wirq);
> +	if (err)
> +		goto err_free_mem;
> +
> +	return 0;
> +
> +err_unlock:
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +err_free_mem:

minor nit, there's no memory being freed here :-)

> +	return err;
> +}
> +
> +/**
> + * 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
> + * on the device specific sysfs wakeup entry. Typically called
> + * during driver probe after calling device_init_wakeup().
> + */
> +int dev_pm_set_wake_irq(struct device *dev, int irq)
> +{
> +	struct wake_irq *wirq;
> +	int err;
> +
> +	wirq = devm_kzalloc(dev, sizeof(*wirq), GFP_KERNEL);
> +	if (!wirq)
> +		return -ENOMEM;
> +
> +	wirq->dev = dev;
> +	wirq->irq = irq;
> +
> +	err = dev_pm_attach_wake_irq(dev, irq, wirq);
> +	if (err)
> +		devm_kfree(dev, wirq);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_set_wake_irq);
> +
> +/**
> + * dev_pm_clear_wake_irq - Detach a device IO interrupt wake IRQ
> + * @dev: Device entry
> + *
> + * Detach a device IO interrupt wake IRQ and free resources.
> + */
> +void dev_pm_clear_wake_irq(struct device *dev)
> +{
> +	struct wake_irq *wirq = dev->power.wakeirq;
> +	unsigned long flags;
> +
> +	if (!wirq)

WARN() to catch bad users ? Maybe with unlikely() around to give
compiler a hint that this is likely not going to happen ?

> +		return;
> +
> +	device_wakeup_detach_irq(dev);
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	dev->power.wakeirq = NULL;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +
> +	wirq->irq = -EINVAL;
> +	devm_kfree(dev, wirq);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_clear_wake_irq);
> +
> +/**
> + * handle_threaded_wakeirq - 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_wakeirq(int wakeirq, void *_wirq)
> +{
> +	struct wake_irq *wirq = _wirq;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	if (!pm_runtime_suspended(wirq->dev))

should you WARN() here ? Why would this IRQ fire if we're not
pm_runtime_suspended() ?

> +		goto out;
> +
> +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> +	pm_runtime_resume(wirq->dev);
> +	ret = IRQ_HANDLED;
> +
> +	if (wirq->handler)
> +		ret = wirq->handler(wakeirq, wirq->data);
> +out:
> +	return ret;
> +}
> +
> +/**
> + * dev_pm_request_wake_irq - Request a dedicated wake-up interrupt
> + * @dev: Device entry
> + * @irq: Device wake-up interrupt
> + * @handler: Optional device specific handler
> + * @irqflags: Optional irqflags, IRQF_ONESHOT if not specified
> + * @data: Optional device specific data
> + *
> + * Unless your hardware has separate wake-up interrupts in addition
> + * to the device IO interrupts, you don't need this.
> + *
> + * Sets up a threaded interrupt handler for a device that has
> + * a dedicated wake-up interrupt in addition to the device IO
> + * interrupt.
> + *
> + * The interrupt starts disabled, and needs to be managed for
> + * the device by the bus code or the device driver using
> + * dev_pm_enable_wake_irq() and dev_pm_disable_wake_irq()
> + * functions.
> + */
> +int dev_pm_request_wake_irq(struct device *dev,
> +			    int irq,
> +			    irq_handler_t handler,
> +			    unsigned long irqflags,
> +			    void *data)
> +{
> +	struct wake_irq *wirq;
> +	int err;
> +
> +	wirq = devm_kzalloc(dev, sizeof(*wirq), GFP_KERNEL);
> +	if (!wirq)
> +		return -ENOMEM;
> +
> +	wirq->dev = dev;
> +	wirq->irq = irq;
> +	wirq->handler = handler;

you need to make sure that IRQF_ONESHOT is set in cases where handler is
NULL. Either set it by default:

	if (!handler)
		irqflags |= IRQF_ONESHOT;

or WARN() about it:

	WARN_ON((!handler && !(irqflags & IRQF_ONESHOT));

Actually, after reading more of the code, you have some weird circular
call chain going on here. If handler is a valid function pointer, you
use as primary handler, so IRQ core will call it from hardirq context.
But you also save that pointer as wirq->handler, and call that from
within handle_threaded_wakeirq(). Essentially, handler() is called
twice, once with IRQs disabled, one with IRQs (potentially) enabled.

What did you have in mind for handler() anyway, it seems completely
unnecessary.

> +	wirq->data = data;
> +	if (!irqflags) {
> +		irqflags = IRQF_ONESHOT;
> +		wirq->manage_irq = true;
> +	}
> +	irq_set_status_flags(irq, IRQ_NOAUTOEN);
> +
> +	/*
> +	 * Consumer device may need to power up and restore state
> +	 * so we use a threaded irq.
> +	 */
> +	err = devm_request_threaded_irq(dev, irq, handler,
> +					handle_threaded_wakeirq,
> +					irqflags, dev_name(dev),
> +					wirq);
> +	if (err)
> +		goto err_free;
> +
> +	err = dev_pm_attach_wake_irq(dev, irq, wirq);
> +	if (err)
> +		goto err_free_irq;
> +
> +	return err;
> +
> +err_free_irq:
> +	devm_free_irq(dev, irq, wirq);
> +err_free:
> +	devm_kfree(dev, wirq);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_request_wake_irq);
> +
> +/**
> + * dev_pm_free_wake_irq - Free a wake-up interrupt
> + * @wirq: Device wake-up interrupt
> + */
> +void dev_pm_free_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);
> +	wirq->manage_irq = false;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +	devm_free_irq(wirq->dev, wirq->irq, wirq);

this seems unnecessary, the IRQ will be freed anyway when the device
kobj is destroyed, dev_pm_clear_wake_irq() seems important, however.

> +	dev_pm_clear_wake_irq(dev);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_free_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
> + * the device is running.
> + *
> + * Note that for runtime_suspend()) the wake-up interrupts
> + * should be unconditionally enabled unlike for suspend()
> + * that is conditional.
> + */
> +void dev_pm_enable_wake_irq(struct device *dev)
> +{
> +	struct wake_irq *wirq = dev->power.wakeirq;
> +
> +	if (wirq && wirq->manage_irq)
> +		enable_irq(wirq->irq);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);

you probably want to enable dev_pm_enable_wake_irq() automatically for
from rpm_suspend(). According to runtime_pm documentation, wakeup should
always be enabled for runtime suspended devices. I didn't really look
through the whole thing yet to know if you did call it or not.

> +/**
> + * 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
> + * the device is running.
> + */
> +void dev_pm_disable_wake_irq(struct device *dev)
> +{
> +	struct wake_irq *wirq = dev->power.wakeirq;
> +
> +	if (wirq && wirq->manage_irq)
> +		disable_irq_nosync(wirq->irq);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);

likewise, call this automatically from rpm_resume().

This brings up a question, actually. What to do with devices which were
already runtime suspended when user initiated suspend-to-ram ? Do we
leave wakeups enabled, or do we revisit device_may_wakeup() and
conditionally runtime_resume the device, disable wakeup, and let its
->suspend() callback be called ?

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists