Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Rafael J. Wysocki]

Please always CC linux-pm on CC patches.

On Thursday, March 05, 2015 04:34:06 PM Tony Lindgren wrote:
> Some devices have separate wake-up interrupts in addition to the
> normal device interrupts. The wake-up interrupts can be connected
> to a separate interrupt controller that is always powered. This
> allows the devices and the whole system to enter deeper idle states
> while still being able to wake-up to events.
> 
> As some devices are already using wake-up interrupts, let's add
> some helper functions. This is to avoid having the drivers getting
> things wrong in a different ways. Some of these drivers also have
> a interrupt re-entrancy problem as the normal device interrupt
> handler is being called from the wake-up interrupt as pointed out
> by Thomas Gleixner <tglx@linutronix.de>.

We need to talk a bit about the design here IMO.

I need to have a look at this tomorrow or over the weekend and not in the
middle of the night in particular.

Some comments below, but I may be overlooking things ATM.

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/Kconfig  |   1 +
>  drivers/base/power/Makefile  |   1 +
>  drivers/base/power/wakeirq.c | 201 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_wakeirq.h   |  69 +++++++++++++++
>  kernel/power/Kconfig         |   4 +
>  5 files changed, 276 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 2b8e477..f3e9b88 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -83,6 +83,7 @@ config ARCH_OMAP2PLUS
>  	select OMAP_DM_TIMER
>  	select OMAP_GPMC
>  	select PINCTRL
> +	select PM_WAKEIRQ
>  	select SOC_BUS
>  	select TI_PRIV_EDMA
>  	select OMAP_IRQCHIP
> 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/wakeirq.c b/drivers/base/power/wakeirq.c
> new file mode 100644
> index 0000000..566d69d
> --- /dev/null
> +++ b/drivers/base/power/wakeirq.c
> @@ -0,0 +1,201 @@
> +/*
> + * 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>
> +
> +/**
> + * handle_dedicated_wakeirq - Handler for device wake-up interrupts
> + * @wakeirq: Separate wake-up interrupt for a device different
> + * @_wirq: Wake-up interrupt data
> + *
> + * Some devices have a separate wake-up interrupt in addition to the
> + * regular device interrupt. The wake-up interrupts signal that the
> + * device should be woken up from a deeper idle state. This handler
> + * uses device specific pm_runtime functions to wake-up 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,
> + * this is not a fast path.
> + *
> + * 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_dedicated_wakeirq(int wakeirq, void *_wirq)
> +{
> +	struct wakeirq_source *wirq = _wirq;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> +	if (pm_runtime_suspended(wirq->dev)) {

What if the device is resumed on a different CPU right here?

> +		pm_runtime_mark_last_busy(wirq->dev);
> +		pm_runtime_resume(wirq->dev);

Calling this with disabled interrupts is a bad idea in general.
Is the device guaranteed to have power.irq_safe set?

I guess what you want to call here is pm_request_resume() and
I wouldn't say that calling pm_runtime_mark_last_busy() on a
suspended device was valid.

> +		ret = IRQ_HANDLED;
> +	}
> +
> +	if (wirq->handler)
> +		ret = wirq->handler(wakeirq, wirq->data);
> +
> +	return ret;
> +}
> +
> +static void dev_pm_wakeirq_init(struct device *dev,
> +				struct wakeirq_source *wirq)
> +{
> +	wirq->dev = dev;
> +	wirq->wakeirq = -EINVAL;
> +	wirq->handler = NULL;
> +	wirq->data = NULL;
> +	wirq->initialized = true;
> +}
> +
> +/**
> + * dev_pm_wakeirq_request - Request a wake-up interrupt
> + * @dev: Device dev entry
> + * @wakeirq: Device wake-up interrupt
> + * @handler: Optional device specific handler
> + * @irqflags: Optional irqflags, IRQF_ONESHOT if not specified
> + * @data: Optional device specific data
> + * @wirq: Wake-up irq data
> + *
> + * Sets up a threaded interrupt handler for a device that
> + * by default just wakes up the device on a wake-up interrupt.
> + * The interrupt starts disabled, and needs to be managed for
> + * the device by the bus code or the device driver using
> + * dev_pm_wakeirq_enable() and dev_pm_wakeirq_disable()
> + * functions.
> + */
> +int dev_pm_wakeirq_request(struct device *dev,
> +			   int wakeirq,
> +			   irq_handler_t handler,
> +			   unsigned long irqflags,
> +			   void *data,
> +			   struct wakeirq_source *wirq)
> +{
> +	int err;
> +
> +	if (!dev || !wirq)
> +		return -EINVAL;
> +
> +	if (!wirq->initialized)
> +		dev_pm_wakeirq_init(dev, wirq);
> +
> +	if (!irqflags)
> +		irqflags = IRQF_ONESHOT;
> +
> +	irq_set_status_flags(wakeirq, IRQ_NOAUTOEN);
> +
> +	/*
> +	 * Consumer device may need to power up and restore state
> +	 * so let's just use threaded irq.
> +	 */
> +	err = devm_request_threaded_irq(wirq->dev,
> +					wakeirq,
> +					handler,
> +					handle_dedicated_wakeirq,
> +					irqflags,
> +					dev_name(wirq->dev),
> +					wirq);
> +	if (err)
> +		return err;
> +
> +	wirq->wakeirq = wakeirq;
> +	wirq->handler = handler;
> +	wirq->data = data;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_wakeirq_request);
> +
> +static int is_valid_wakeirq(struct wakeirq_source *wirq)
> +{
> +	return wirq && wirq->initialized && (wirq->wakeirq >= 0);
> +}
> +
> +#define is_invalid_wakeirq(w)	!is_valid_wakeirq(w)
> +
> +/**
> + * dev_pm_wakeirq_free - Free a wake-up interrupt
> + * @wirq: Device wake-up interrupt
> + */
> +void dev_pm_wakeirq_free(struct wakeirq_source *wirq)
> +{
> +	if (is_invalid_wakeirq(wirq))
> +		return;
> +
> +	devm_free_irq(wirq->dev, wirq->wakeirq, wirq);
> +	wirq->wakeirq = -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_wakeirq_free);
> +
> +/**
> + * dev_pm_wakeirq_enable - Enable device wake-up interrupt
> + * @wirq: Device wake-up interrupt
> + *
> + * 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_wakeirq_enable(struct wakeirq_source *wirq)
> +{
> +	if (is_invalid_wakeirq(wirq))
> +		return;
> +
> +	enable_irq(wirq->wakeirq);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_wakeirq_enable);
> +
> +/**
> + * dev_pm_wakeirq_disable - Disable device wake-up interrupt
> + * @wirq: Device wake-up interrupt
> + *
> + * 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_wakeirq_disable(struct wakeirq_source *wirq)
> +{
> +	if (is_invalid_wakeirq(wirq))
> +		return;
> +
> +	disable_irq_nosync(wirq->wakeirq);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_wakeirq_disable);
> +
> +/**
> + * dev_pm_wakeirq_arm_for_suspend - Configure device wake-up
> + * @wirq: Device wake-up interrupt
> + *
> + * Called from the bus code or the device driver for
> + * device suspend(). Just sets up the wake-up event
> + * conditionally based on the device_may_wake(). The
> + * rest is handled automatically by the generic suspend()
> + * code and runtime_suspend().
> + */
> +void dev_pm_wakeirq_arm_for_suspend(struct wakeirq_source *wirq)
> +{
> +	if (is_invalid_wakeirq(wirq))
> +		return;
> +
> +	irq_set_irq_wake(wirq->wakeirq,
> +			 device_may_wakeup(wirq->dev));

You want to do

	if (device_may_wakeup(wirq->dev))
		enable_irq_wake(wirq->wakeirq);

here or strange things may happen if two devices share a wakeup IRQ.

Also instead of doing it this way, I'd prefer to hook system wakeup
interrupts into the wakeup source objects pointed to by the power.wakeup
fields in struct device.

Then we could just walk the list of wakeup sources and do enable_irq_wake()
automatically for the wakeup interrupts hooked up to them at the
suspend_device_irqs() time without the need to do anything from drivers
at suspend time.

> +}
> +EXPORT_SYMBOL_GPL(dev_pm_wakeirq_arm_for_suspend);
> +
> diff --git a/include/linux/pm_wakeirq.h b/include/linux/pm_wakeirq.h
> new file mode 100644
> index 0000000..3ecbc1a
> --- /dev/null
> +++ b/include/linux/pm_wakeirq.h
> @@ -0,0 +1,69 @@
> +/*
> + * pm_wakeirq.h - 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.
> + */
> +
> +#ifndef _LINUX_PM_WAKEIRQ_H
> +#define _LINUX_PM_WAKEIRQ_H
> +
> +struct wakeirq_source {
> +	struct device *dev;
> +	int wakeirq;
> +	bool initialized;
> +	bool enabled;
> +	irq_handler_t handler;
> +	void *data;
> +};

Well, so now we have struct wakeup_source already and here we get struct wakeirq_source
and they mean different things ...

> +
> +#ifdef CONFIG_PM_WAKEIRQ
> +
> +extern int dev_pm_wakeirq_request(struct device *dev,
> +				  int wakeirq,
> +				  irq_handler_t handler,
> +				  unsigned long irqflags,
> +				  void *data,
> +				  struct wakeirq_source *wirq);
> +extern void dev_pm_wakeirq_free(struct wakeirq_source *wirq);
> +extern void dev_pm_wakeirq_enable(struct wakeirq_source *wirq);
> +extern void dev_pm_wakeirq_disable(struct wakeirq_source *wirq);
> +extern void dev_pm_wakeirq_arm_for_suspend(struct wakeirq_source *wirq);
> +
> +#else	/* !CONFIG_PM_WAKEIRQ */
> +
> +static inline int dev_pm_wakeirq_request(struct device *dev,
> +					 int wakeirq,
> +					 irq_handler_t handler,
> +					 unsigned long irqflags,
> +					 void *data,
> +					 struct wakeirq_source *wirq)
> +{
> +	return 0;
> +}
> +
> +static inline void dev_pm_wakeirq_free(struct wakeirq_source *wirq)
> +{
> +}
> +
> +static inline void dev_pm_wakeirq_enable(struct wakeirq_source *wirq)
> +{
> +}
> +
> +static inline void dev_pm_wakeirq_disable(struct wakeirq_source *wirq)
> +{
> +}
> +
> +static inline void
> +dev_pm_wakeirq_arm_for_suspend(struct wakeirq_source *wirq)
> +{
> +}
> +
> +#endif	/* CONFIG_PM_WAKEIRQ */
> +#endif	/* _LINUX_PM_WAKEIRQ_H */
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 7e01f78..c249845 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
> +
>  config PM_GENERIC_DOMAINS
>  	bool
>  	depends on PM
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Rafael J. Wysocki]

On Fri, Mar 6, 2015 at 3:02 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Please always CC linux-pm on CC patches.

Doh.  That was supposed to say "Please always CC linux-pm on PM patches".

I really should not reply to email when I'm too tired ...

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Tony Lindgren]

Hi,

* Rafael J. Wysocki <rjw@rjwysocki.net> [150305 17:38]:
> Please always CC linux-pm on CC patches.

Sure will do for the next rev, sorry forgot to add that.
 
> On Thursday, March 05, 2015 04:34:06 PM Tony Lindgren wrote:
> > +/**
> > + * handle_dedicated_wakeirq - Handler for device wake-up interrupts
> > + * @wakeirq: Separate wake-up interrupt for a device different
> > + * @_wirq: Wake-up interrupt data
> > + *
> > + * Some devices have a separate wake-up interrupt in addition to the
> > + * regular device interrupt. The wake-up interrupts signal that the
> > + * device should be woken up from a deeper idle state. This handler
> > + * uses device specific pm_runtime functions to wake-up 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,
> > + * this is not a fast path.
> > + *
> > + * 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_dedicated_wakeirq(int wakeirq, void *_wirq)
> > +{
> > +	struct wakeirq_source *wirq = _wirq;
> > +	irqreturn_t ret = IRQ_NONE;
> > +
> > +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> > +	if (pm_runtime_suspended(wirq->dev)) {
> 
> What if the device is resumed on a different CPU right here?

Good point, sounds like we need to do this in some pm_runtime
function directly for the locking.
 
> > +		pm_runtime_mark_last_busy(wirq->dev);
> > +		pm_runtime_resume(wirq->dev);
> 
> Calling this with disabled interrupts is a bad idea in general.
> Is the device guaranteed to have power.irq_safe set?

Well right now it's using threaded irq, and I'd like to get rid of
the pm_runtime calls in the regular driver interrupts completely.
We need to ensure the device runtime_resume is completed before
returning IRQ_HANDLED here.
 
> I guess what you want to call here is pm_request_resume() and
> I wouldn't say that calling pm_runtime_mark_last_busy() on a
> suspended device was valid.

I'll verify again, but I believe the issue was that without doing
mark_last_busy here the device falls back asleep right away.
That probably should be fixed in pm_runtime in general if that's
the case.

Considering the above, should we add a new function something like
pm_resume_complete() that does not need irq_safe set but does
not return until the device has completed resume?

I think that would be pretty much probably just pm_request_resume
+ pm_runtime_barrier.

> > +/**
> > + * dev_pm_wakeirq_arm_for_suspend - Configure device wake-up
> > + * @wirq: Device wake-up interrupt
> > + *
> > + * Called from the bus code or the device driver for
> > + * device suspend(). Just sets up the wake-up event
> > + * conditionally based on the device_may_wake(). The
> > + * rest is handled automatically by the generic suspend()
> > + * code and runtime_suspend().
> > + */
> > +void dev_pm_wakeirq_arm_for_suspend(struct wakeirq_source *wirq)
> > +{
> > +	if (is_invalid_wakeirq(wirq))
> > +		return;
> > +
> > +	irq_set_irq_wake(wirq->wakeirq,
> > +			 device_may_wakeup(wirq->dev));
> 
> You want to do
> 
> 	if (device_may_wakeup(wirq->dev))
> 		enable_irq_wake(wirq->wakeirq);
> 
> here or strange things may happen if two devices share a wakeup IRQ.

OK sure.
 
> Also instead of doing it this way, I'd prefer to hook system wakeup
> interrupts into the wakeup source objects pointed to by the power.wakeup
> fields in struct device.
> 
> Then we could just walk the list of wakeup sources and do enable_irq_wake()
> automatically for the wakeup interrupts hooked up to them at the
> suspend_device_irqs() time without the need to do anything from drivers
> at suspend time.

OK that's a good idea. Then we can drop dev_pm_wakeirq_arm_for_suspend()
and make that part automatic.

Then for runtime_pm, we could make the toggling of the wakeirq handling
automatic too. Or do you see a problem with that?

> > +struct wakeirq_source {
> > +	struct device *dev;
> > +	int wakeirq;
> > +	bool initialized;
> > +	bool enabled;
> > +	irq_handler_t handler;
> > +	void *data;
> > +};
> 
> Well, so now we have struct wakeup_source already and here we get struct wakeirq_source
> and they mean different things ...

Well I was trying to keep it out of the way for most drivers not needing
to use wakeirqs. I'll take a look at making it a pointer in the struct
wakeup_source.

Regards,

Tony

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Alan Stern]

On Fri, 6 Mar 2015, Tony Lindgren wrote:

> > > +	struct wakeirq_source *wirq = _wirq;
> > > +	irqreturn_t ret = IRQ_NONE;
> > > +
> > > +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> > > +	if (pm_runtime_suspended(wirq->dev)) {
> > 
> > What if the device is resumed on a different CPU right here?
> 
> Good point, sounds like we need to do this in some pm_runtime
> function directly for the locking.
>  
> > > +		pm_runtime_mark_last_busy(wirq->dev);
> > > +		pm_runtime_resume(wirq->dev);
> > 
> > Calling this with disabled interrupts is a bad idea in general.
> > Is the device guaranteed to have power.irq_safe set?
> 
> Well right now it's using threaded irq, and I'd like to get rid of
> the pm_runtime calls in the regular driver interrupts completely.
> We need to ensure the device runtime_resume is completed before
> returning IRQ_HANDLED here.

In general, runtime_resume methods are allowed to sleep.  They can't be
used in an interrupt handler top half unless the driver has
specifically promised they are IRQ-safe.  That's what Rafael was
getting at.

Of course, if this routine is a threaded-irq bottom half then there's 
no problem.

> > I guess what you want to call here is pm_request_resume() and
> > I wouldn't say that calling pm_runtime_mark_last_busy() on a
> > suspended device was valid.
> 
> I'll verify again, but I believe the issue was that without doing
> mark_last_busy here the device falls back asleep right away.
> That probably should be fixed in pm_runtime in general if that's
> the case.

It's up to the subsystem to handle this.  For example, the USB 
subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.

> Considering the above, should we add a new function something like
> pm_resume_complete() that does not need irq_safe set but does
> not return until the device has completed resume?

That doesn't make sense.  You're asking for a routine that is allowed
to sleep but can safely be called in interrupt context.

Alan Stern

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Tony Lindgren]

* Alan Stern <stern@rowland.harvard.edu> [150306 11:05]:
> On Fri, 6 Mar 2015, Tony Lindgren wrote:
> 
> > > > +	struct wakeirq_source *wirq = _wirq;
> > > > +	irqreturn_t ret = IRQ_NONE;
> > > > +
> > > > +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> > > > +	if (pm_runtime_suspended(wirq->dev)) {
> > > 
> > > What if the device is resumed on a different CPU right here?
> > 
> > Good point, sounds like we need to do this in some pm_runtime
> > function directly for the locking.
> >  
> > > > +		pm_runtime_mark_last_busy(wirq->dev);
> > > > +		pm_runtime_resume(wirq->dev);
> > > 
> > > Calling this with disabled interrupts is a bad idea in general.
> > > Is the device guaranteed to have power.irq_safe set?
> > 
> > Well right now it's using threaded irq, and I'd like to get rid of
> > the pm_runtime calls in the regular driver interrupts completely.
> > We need to ensure the device runtime_resume is completed before
> > returning IRQ_HANDLED here.
> 
> In general, runtime_resume methods are allowed to sleep.  They can't be
> used in an interrupt handler top half unless the driver has
> specifically promised they are IRQ-safe.  That's what Rafael was
> getting at.

Yes I understand, otherwise things certainly would not work :)

> Of course, if this routine is a threaded-irq bottom half then there's 
> no problem.

Right this is threaded-irq bottom half because the devices may
need to restore state and start regulators.
 
> > > I guess what you want to call here is pm_request_resume() and
> > > I wouldn't say that calling pm_runtime_mark_last_busy() on a
> > > suspended device was valid.
> > 
> > I'll verify again, but I believe the issue was that without doing
> > mark_last_busy here the device falls back asleep right away.
> > That probably should be fixed in pm_runtime in general if that's
> > the case.
> 
> It's up to the subsystem to handle this.  For example, the USB 
> subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.

Hmm.. OK thanks this probably explains why pm_request_resume() did
not work.

For omaps, I could call this from the interconnect related code,
but then how dow we deal with the subsystems that don't call it?

> > Considering the above, should we add a new function something like
> > pm_resume_complete() that does not need irq_safe set but does
> > not return until the device has completed resume?
> 
> That doesn't make sense.  You're asking for a routine that is allowed
> to sleep but can safely be called in interrupt context.

Oh it naturally would not work in irq context, it's for the bottom
half again. But I'll take a look if we can just call
pm_request_resume() and disable_irq() on the wakeirq in without
waiting for the device driver runtime_suspend to disable the wakeirq.
That would minimize the interface to just dev_pm_request_wakeirq()
and dev_pm_free_wakeirq().

Regards,

Tony

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Rafael J. Wysocki]

On Friday, March 06, 2015 03:05:40 PM Tony Lindgren wrote:
> * Alan Stern <stern@rowland.harvard.edu> [150306 11:05]:
> > On Fri, 6 Mar 2015, Tony Lindgren wrote:
> > 
> > > > > +	struct wakeirq_source *wirq = _wirq;
> > > > > +	irqreturn_t ret = IRQ_NONE;
> > > > > +
> > > > > +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> > > > > +	if (pm_runtime_suspended(wirq->dev)) {
> > > > 
> > > > What if the device is resumed on a different CPU right here?
> > > 
> > > Good point, sounds like we need to do this in some pm_runtime
> > > function directly for the locking.
> > >  
> > > > > +		pm_runtime_mark_last_busy(wirq->dev);
> > > > > +		pm_runtime_resume(wirq->dev);
> > > > 
> > > > Calling this with disabled interrupts is a bad idea in general.
> > > > Is the device guaranteed to have power.irq_safe set?
> > > 
> > > Well right now it's using threaded irq, and I'd like to get rid of
> > > the pm_runtime calls in the regular driver interrupts completely.
> > > We need to ensure the device runtime_resume is completed before
> > > returning IRQ_HANDLED here.
> > 
> > In general, runtime_resume methods are allowed to sleep.  They can't be
> > used in an interrupt handler top half unless the driver has
> > specifically promised they are IRQ-safe.  That's what Rafael was
> > getting at.
> 
> Yes I understand, otherwise things certainly would not work :)
> 
> > Of course, if this routine is a threaded-irq bottom half then there's 
> > no problem.
> 
> Right this is threaded-irq bottom half because the devices may
> need to restore state and start regulators.
>  
> > > > I guess what you want to call here is pm_request_resume() and
> > > > I wouldn't say that calling pm_runtime_mark_last_busy() on a
> > > > suspended device was valid.
> > > 
> > > I'll verify again, but I believe the issue was that without doing
> > > mark_last_busy here the device falls back asleep right away.
> > > That probably should be fixed in pm_runtime in general if that's
> > > the case.
> > 
> > It's up to the subsystem to handle this.  For example, the USB 
> > subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.
> 
> Hmm.. OK thanks this probably explains why pm_request_resume() did
> not work.
> 
> For omaps, I could call this from the interconnect related code,
> but then how dow we deal with the subsystems that don't call it?

Good question. :-)

> > > Considering the above, should we add a new function something like
> > > pm_resume_complete() that does not need irq_safe set but does
> > > not return until the device has completed resume?
> > 
> > That doesn't make sense.  You're asking for a routine that is allowed
> > to sleep but can safely be called in interrupt context.
> 
> Oh it naturally would not work in irq context, it's for the bottom
> half again. But I'll take a look if we can just call
> pm_request_resume() and disable_irq() on the wakeirq in without
> waiting for the device driver runtime_suspend to disable the wakeirq.
> That would minimize the interface to just dev_pm_request_wakeirq()
> and dev_pm_free_wakeirq().

But this is part of a bigger picture.  Namely, if a separete wakeup interrupt
is required for a device, the device's power.can_wakeup flag cannot be set
until that interrupt has been successfully requested.  Also for devices that
can signal wakeup via their own IO interrupts, it would make sense to allow
those interrupts to be registered somehow as "wakeup interrupts".

So I wonder if we can define a new struct along the lines of your
struct wakeirq_source, but call it struct wake_irq and make it look
something like this:

struct wake_irq {
       struct device *dev;
       int irq;
       irq_handler_t handler;
};

Then, add a struct wake_irq pointer to struct dev_pm_info *and* to
struct wakeup_source.  Next, make dev_pm_request_wake_irq() allocate the
structure and request the interrupt and only set the pointer to it from
struct dev_pm_info *along* *with* power.can_wakeup if all that was
successful.

For devices that use their own IO IRQ for wakeup, we can add something
like dev_pm_set_wake_irq() that will work analogously, but without requesting
the interrupt.  It will just set the dev and irq members of struct wake_irq
and point struct dev_pm_info to it and set its power.can_wakeup flag.

Then, device_wakeup_enable() will be able to see that the device has a
wakeup IRQ and it may then point its own struct wake_irq pointer to that.
The core may then use that pointer to trigger enable_irq_wake() for the
IRQ in question and it will cover the devices that don't need separate
wakeup interrupts too.

Does that make sense to you?

Rafael

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Tony Lindgren]

* Rafael J. Wysocki <rjw@rjwysocki.net> [150306 16:19]:
> On Friday, March 06, 2015 03:05:40 PM Tony Lindgren wrote:
> > 
> > Oh it naturally would not work in irq context, it's for the bottom
> > half again. But I'll take a look if we can just call
> > pm_request_resume() and disable_irq() on the wakeirq in without
> > waiting for the device driver runtime_suspend to disable the wakeirq.
> > That would minimize the interface to just dev_pm_request_wakeirq()
> > and dev_pm_free_wakeirq().
> 
> But this is part of a bigger picture.  Namely, if a separete wakeup interrupt
> is required for a device, the device's power.can_wakeup flag cannot be set
> until that interrupt has been successfully requested.  Also for devices that
> can signal wakeup via their own IO interrupts, it would make sense to allow
> those interrupts to be registered somehow as "wakeup interrupts".

It sure would be nice to provide at least some automated handling
for those too, even if it was just to deal with if device_may_wake()
irq_set_irq_wake().

At least in the cases I've seen, the IO interrupt is capable of waking
up too, but not from any deeper idle states. The wakeirq is always
capable of waking up the system, so if wakeirq was configured we
could just ignore the wake configureation for the IO interrupt.

And it seems some devices have a single wakeirq dealing with a group
of IO interrupts (GPIOs), see commit 97d86e07b716 ("Input: gpio_keys -
allow separating gpio and irq in device tree"). Not sure if that
interrupt is wake-up capable, but that would certainly make sense
considering it's for gpio-keys.

So it seems as long as we have one wakeirq entry per device, we
should be covered, even if a single wakeirq needs to wake up multiple
devices.

> So I wonder if we can define a new struct along the lines of your
> struct wakeirq_source, but call it struct wake_irq and make it look
> something like this:
> 
> struct wake_irq {
>        struct device *dev;
>        int irq;
>        irq_handler_t handler;
> };
> 
> Then, add a struct wake_irq pointer to struct dev_pm_info *and* to
> struct wakeup_source.  Next, make dev_pm_request_wake_irq() allocate the
> structure and request the interrupt and only set the pointer to it from
> struct dev_pm_info *along* *with* power.can_wakeup if all that was
> successful.
> 
> For devices that use their own IO IRQ for wakeup, we can add something
> like dev_pm_set_wake_irq() that will work analogously, but without requesting
> the interrupt.  It will just set the dev and irq members of struct wake_irq
> and point struct dev_pm_info to it and set its power.can_wakeup flag.

OK
 
> Then, device_wakeup_enable() will be able to see that the device has a
> wakeup IRQ and it may then point its own struct wake_irq pointer to that.
> The core may then use that pointer to trigger enable_irq_wake() for the
> IRQ in question and it will cover the devices that don't need separate
> wakeup interrupts too.

Are you thinking we could do more than automate irq_set_irq_wake()
for the devices with just wake-up capable IO IRQ?
 
> Does that make sense to you?

Sure, at least for the irq_set_irq_wake() case.

Regards,

Tony

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Alan Stern]

On Sat, 7 Mar 2015, Rafael J. Wysocki wrote:

> But this is part of a bigger picture.  Namely, if a separete wakeup interrupt
> is required for a device, the device's power.can_wakeup flag cannot be set
> until that interrupt has been successfully requested.  Also for devices that
> can signal wakeup via their own IO interrupts, it would make sense to allow
> those interrupts to be registered somehow as "wakeup interrupts".
> 
> So I wonder if we can define a new struct along the lines of your
> struct wakeirq_source, but call it struct wake_irq and make it look
> something like this:
> 
> struct wake_irq {
>        struct device *dev;
>        int irq;
>        irq_handler_t handler;
> };
> 
> Then, add a struct wake_irq pointer to struct dev_pm_info *and* to
> struct wakeup_source.  Next, make dev_pm_request_wake_irq() allocate the
> structure and request the interrupt and only set the pointer to it from
> struct dev_pm_info *along* *with* power.can_wakeup if all that was
> successful.
> 
> For devices that use their own IO IRQ for wakeup, we can add something
> like dev_pm_set_wake_irq() that will work analogously, but without requesting
> the interrupt.  It will just set the dev and irq members of struct wake_irq
> and point struct dev_pm_info to it and set its power.can_wakeup flag.
> 
> Then, device_wakeup_enable() will be able to see that the device has a
> wakeup IRQ and it may then point its own struct wake_irq pointer to that.
> The core may then use that pointer to trigger enable_irq_wake() for the
> IRQ in question and it will cover the devices that don't need separate
> wakeup interrupts too.
> 
> Does that make sense to you?

Can we back up a little?  What is the basic problem the two of you are 
trying to solve?

Alan Stern

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Rafael J. Wysocki]

On Sunday, March 08, 2015 11:43:34 AM Alan Stern wrote:
> On Sat, 7 Mar 2015, Rafael J. Wysocki wrote:
> 
> > But this is part of a bigger picture.  Namely, if a separete wakeup interrupt
> > is required for a device, the device's power.can_wakeup flag cannot be set
> > until that interrupt has been successfully requested.  Also for devices that
> > can signal wakeup via their own IO interrupts, it would make sense to allow
> > those interrupts to be registered somehow as "wakeup interrupts".
> > 
> > So I wonder if we can define a new struct along the lines of your
> > struct wakeirq_source, but call it struct wake_irq and make it look
> > something like this:
> > 
> > struct wake_irq {
> >        struct device *dev;
> >        int irq;
> >        irq_handler_t handler;
> > };
> > 
> > Then, add a struct wake_irq pointer to struct dev_pm_info *and* to
> > struct wakeup_source.  Next, make dev_pm_request_wake_irq() allocate the
> > structure and request the interrupt and only set the pointer to it from
> > struct dev_pm_info *along* *with* power.can_wakeup if all that was
> > successful.
> > 
> > For devices that use their own IO IRQ for wakeup, we can add something
> > like dev_pm_set_wake_irq() that will work analogously, but without requesting
> > the interrupt.  It will just set the dev and irq members of struct wake_irq
> > and point struct dev_pm_info to it and set its power.can_wakeup flag.
> > 
> > Then, device_wakeup_enable() will be able to see that the device has a
> > wakeup IRQ and it may then point its own struct wake_irq pointer to that.
> > The core may then use that pointer to trigger enable_irq_wake() for the
> > IRQ in question and it will cover the devices that don't need separate
> > wakeup interrupts too.
> > 
> > Does that make sense to you?
> 
> Can we back up a little?  What is the basic problem the two of you are 
> trying to solve?

Essentially, code duplication between drivers that all need to do the same
thing which can be moved to the core quite easily.

Rafael

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Alan Stern]

On Fri, 6 Mar 2015, Tony Lindgren wrote:

> > > I'll verify again, but I believe the issue was that without doing
> > > mark_last_busy here the device falls back asleep right away.

As it should.  If you don't increment the usage counter (for example, 
by calling pm_runtime_get instead of pm_request_resume) and you don't 
update last_busy then you are telling the PM core that the device 
currently isn't busy and it hasn't been in use since the last time it 
was suspended.  Under those circumstances, the PM core is _supposed_ to 
suspend the device right away.

> > > That probably should be fixed in pm_runtime in general if that's
> > > the case.
> > 
> > It's up to the subsystem to handle this.  For example, the USB 
> > subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.
> 
> Hmm.. OK thanks this probably explains why pm_request_resume() did
> not work.
> 
> For omaps, I could call this from the interconnect related code,
> but then how dow we deal with the subsystems that don't call it?

Start by determining _why_ they don't call it.  Maybe they have a good 
reason.  If they don't then fix them.

> > > Considering the above, should we add a new function something like
> > > pm_resume_complete() that does not need irq_safe set but does
> > > not return until the device has completed resume?
> > 
> > That doesn't make sense.  You're asking for a routine that is allowed
> > to sleep but can safely be called in interrupt context.
> 
> Oh it naturally would not work in irq context, it's for the bottom
> half again.

In other words, you're suggesting we add a function that runs in 
process context and doesn't return until the device is fully resumed?  
That's exactly what pm_runtime_resume does right now.

>  But I'll take a look if we can just call
> pm_request_resume() and disable_irq() on the wakeirq in without
> waiting for the device driver runtime_suspend to disable the wakeirq.
> That would minimize the interface to just dev_pm_request_wakeirq()
> and dev_pm_free_wakeirq().

Will that be acceptable in systems with shared IRQ lines?

Alan Stern

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Tony Lindgren]

* Alan Stern <stern@rowland.harvard.edu> [150308 08:41]:
> On Fri, 6 Mar 2015, Tony Lindgren wrote:
> 
> > > > I'll verify again, but I believe the issue was that without doing
> > > > mark_last_busy here the device falls back asleep right away.
> 
> As it should.  If you don't increment the usage counter (for example, 
> by calling pm_runtime_get instead of pm_request_resume) and you don't 
> update last_busy then you are telling the PM core that the device 
> currently isn't busy and it hasn't been in use since the last time it 
> was suspended.  Under those circumstances, the PM core is _supposed_ to 
> suspend the device right away.

OK so it's a feature then.
 
> > > > That probably should be fixed in pm_runtime in general if that's
> > > > the case.
> > > 
> > > It's up to the subsystem to handle this.  For example, the USB 
> > > subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.
> > 
> > Hmm.. OK thanks this probably explains why pm_request_resume() did
> > not work.
> > 
> > For omaps, I could call this from the interconnect related code,
> > but then how dow we deal with the subsystems that don't call it?
> 
> Start by determining _why_ they don't call it.  Maybe they have a good 
> reason.  If they don't then fix them.

Yes I'll check, it's just probably because the drivers have been
calling it instead.
 
> > > > Considering the above, should we add a new function something like
> > > > pm_resume_complete() that does not need irq_safe set but does
> > > > not return until the device has completed resume?
> > > 
> > > That doesn't make sense.  You're asking for a routine that is allowed
> > > to sleep but can safely be called in interrupt context.
> > 
> > Oh it naturally would not work in irq context, it's for the bottom
> > half again.
> 
> In other words, you're suggesting we add a function that runs in 
> process context and doesn't return until the device is fully resumed?  
> That's exactly what pm_runtime_resume does right now.

But doesn't it only wait for completion if the driver is marked with
pm_runtime_irq_safe()?
 
> >  But I'll take a look if we can just call
> > pm_request_resume() and disable_irq() on the wakeirq in without
> > waiting for the device driver runtime_suspend to disable the wakeirq.
> > That would minimize the interface to just dev_pm_request_wakeirq()
> > and dev_pm_free_wakeirq().
> 
> Will that be acceptable in systems with shared IRQ lines?

Not without us keeping track of when the wakeirq is enabled or
disabled :)

Regards,

Tony

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Alan Stern]

On Mon, 9 Mar 2015, Tony Lindgren wrote:

> > > > > Considering the above, should we add a new function something like
> > > > > pm_resume_complete() that does not need irq_safe set but does
> > > > > not return until the device has completed resume?
> > > > 
> > > > That doesn't make sense.  You're asking for a routine that is allowed
> > > > to sleep but can safely be called in interrupt context.
> > > 
> > > Oh it naturally would not work in irq context, it's for the bottom
> > > half again.
> > 
> > In other words, you're suggesting we add a function that runs in 
> > process context and doesn't return until the device is fully resumed?  
> > That's exactly what pm_runtime_resume does right now.
> 
> But doesn't it only wait for completion if the driver is marked with
> pm_runtime_irq_safe()?

Put it this way: pm_runtime_resume invokes a ->runtime_resume
callback routine (the subsystem's or the driver's or whichever), and it
assumes that if the routine returns 0 then the device has been resumed.  
It doesn't really _wait_ for anything; it just calls the callback
routine.

It behaves this way whether or not the irq_safe flag is set.  The only
difference is that if irq_safe is set then the callback routine is
invoked with interrupts disabled (and in this case pm_runtime_resume
may be called in interrupt context -- normally it can be called only in
process context).

Alan Stern

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Tony Lindgren]

* Alan Stern <stern@rowland.harvard.edu> [150309 08:42]:
> On Mon, 9 Mar 2015, Tony Lindgren wrote:
> 
> > > > > > Considering the above, should we add a new function something like
> > > > > > pm_resume_complete() that does not need irq_safe set but does
> > > > > > not return until the device has completed resume?
> > > > > 
> > > > > That doesn't make sense.  You're asking for a routine that is allowed
> > > > > to sleep but can safely be called in interrupt context.
> > > > 
> > > > Oh it naturally would not work in irq context, it's for the bottom
> > > > half again.
> > > 
> > > In other words, you're suggesting we add a function that runs in 
> > > process context and doesn't return until the device is fully resumed?  
> > > That's exactly what pm_runtime_resume does right now.
> > 
> > But doesn't it only wait for completion if the driver is marked with
> > pm_runtime_irq_safe()?
> 
> Put it this way: pm_runtime_resume invokes a ->runtime_resume
> callback routine (the subsystem's or the driver's or whichever), and it
> assumes that if the routine returns 0 then the device has been resumed.  
> It doesn't really _wait_ for anything; it just calls the callback
> routine.
> 
> It behaves this way whether or not the irq_safe flag is set.  The only
> difference is that if irq_safe is set then the callback routine is
> invoked with interrupts disabled (and in this case pm_runtime_resume
> may be called in interrupt context -- normally it can be called only in
> process context).

Oh right you are. Looking at rpm_resume() again, it's the RPM_ASYNC that
was causing problems to me earlier, not the irq_safe. Sorry it seems I
was a bit confused. And yes, pm_runtime_resume() does not set RPM_ASYNC
like you pointed out earlier so no need to do anything there.

Regards,

Tony

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Rafael J. Wysocki]

On Friday, March 06, 2015 02:05:43 PM Alan Stern wrote:
> On Fri, 6 Mar 2015, Tony Lindgren wrote:
> 
> > > > +	struct wakeirq_source *wirq = _wirq;
> > > > +	irqreturn_t ret = IRQ_NONE;
> > > > +
> > > > +	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
> > > > +	if (pm_runtime_suspended(wirq->dev)) {
> > > 
> > > What if the device is resumed on a different CPU right here?
> > 
> > Good point, sounds like we need to do this in some pm_runtime
> > function directly for the locking.
> >  
> > > > +		pm_runtime_mark_last_busy(wirq->dev);
> > > > +		pm_runtime_resume(wirq->dev);
> > > 
> > > Calling this with disabled interrupts is a bad idea in general.
> > > Is the device guaranteed to have power.irq_safe set?
> > 
> > Well right now it's using threaded irq, and I'd like to get rid of
> > the pm_runtime calls in the regular driver interrupts completely.
> > We need to ensure the device runtime_resume is completed before
> > returning IRQ_HANDLED here.
> 
> In general, runtime_resume methods are allowed to sleep.  They can't be
> used in an interrupt handler top half unless the driver has
> specifically promised they are IRQ-safe.  That's what Rafael was
> getting at.
> 
> Of course, if this routine is a threaded-irq bottom half then there's 
> no problem.

Yup.  I overlooked the threaded part.

> > > I guess what you want to call here is pm_request_resume() and
> > > I wouldn't say that calling pm_runtime_mark_last_busy() on a
> > > suspended device was valid.
> > 
> > I'll verify again, but I believe the issue was that without doing
> > mark_last_busy here the device falls back asleep right away.
> > That probably should be fixed in pm_runtime in general if that's
> > the case.
> 
> It's up to the subsystem to handle this.  For example, the USB 
> subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.

I'm wondering, though, if there's any reason for us to avoid updating
power.last_busy in rpm_resume().

If I was a driver writer, I'd expect the core to do that for me quite frankly.

Rafael

Re: [PATCH 1/4] PM / Wakeirq: Add minimal device wakeirq helper functions

[Alan Stern]

On Sat, 7 Mar 2015, Rafael J. Wysocki wrote:

> > > Well right now it's using threaded irq, and I'd like to get rid of
> > > I'll verify again, but I believe the issue was that without doing
> > > mark_last_busy here the device falls back asleep right away.
> > > That probably should be fixed in pm_runtime in general if that's
> > > the case.
> > 
> > It's up to the subsystem to handle this.  For example, the USB 
> > subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.
> 
> I'm wondering, though, if there's any reason for us to avoid updating
> power.last_busy in rpm_resume().
> 
> If I was a driver writer, I'd expect the core to do that for me quite frankly.

In theory, you might want to wake up a device to perform some very 
limited operation (like reading an internal register) and then put it 
back into suspend very quickly, without waiting for the autosuspend 
delay to elapse.  Apart from that, I can't think of any reason not to 
update last_busy in rpm_resume.

Alan Stern