Re: [RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Rafael,

On Wed, 25 Feb 2015 22:59:36 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:
> > Hello,
> > 
> > I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE
> > debate aside to concentrate on another problem pointed out by Rafael and
> > Mark: the fact that we cannot mix IRQF_NO_SUSPEND and wakeup sources on
> > a shared IRQ line.
> > 
> > This is because the wakeup code is prevailing the IRQF_NO_SUSPEND case
> > and will trigger a system wakeup as soon as the IRQ line is tagged as a
> > wakeup source.
> > 
> > This series propose an approach to deal with such cases by doing the
> > following:
> > 1/ Prevent any system wakeup when at least one of the IRQ user has set
> >    the IRQF_NO_SUSPEND flag
> > 2/ Adapt IRQ handlers so that they can safely be called in suspended
> >    state
> > 3/ Let drivers decide when the system should be woken up
> > 
> > Let me know what you think of this approach.
> 
> So I have the appended patch that should deal with all that too (it doesn't
> rework drivers that need to share NO_SUSPEND IRQs and do wakeup, but that
> can be done on top of it in a straightforward way).
> 
> The idea is quite simple.  By default, the core replaces the interrupt handlers
> of everyone sharing NO_SUSPEND lines and not using IRQF_NO_SUSPEND with a null
> handler always returning IRQ_NONE at the suspend_device_irqs() time (the
> rationale being that if you don't use IRQF_NO_SUSPEND, then your device has
> no reason to generate interrupts after that point).  The original handlers are
> then restored by resume_device_irqs().
> 
> However, if the IRQ is configured for wakeup, there may be a reason to generate
> interrupts from a device not using IRQF_NO_SUSPEND.  For that, the patch adds
> IRQF_COND_SUSPEND that, if set, will prevent the default behavior described
> above from being applied to irqactions using it if the IRQs in question are
> configured for wakeup.  Of course, the users of IRQF_COND_SUSPEND are supposed
> to implement wakeup detection in their interrupt handlers and then call
> pm_system_wakeup() if necessary.

That patch sounds good to me.
Could you send it on its own to the appropriate MLs ?

Thomas, Peter, Mark, could you share you opinion ?
I know I'm a bit insistent on this fix, but I'd really like to get away
from this warning backtrace (and the associated problems behind it) as
soon as possible.

> 
> So your patch [3/3] could be redone on top of this AFAICS.

Absolutely.

Thanks.

Boris

> 
> Rafael
> 
> 
> ---
>  include/linux/interrupt.h |   10 +++++++++
>  kernel/irq/pm.c           |   49 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/include/linux/interrupt.h
> ===================================================================
> --- linux-pm.orig/include/linux/interrupt.h
> +++ linux-pm/include/linux/interrupt.h
> @@ -57,6 +57,11 @@
>   * IRQF_NO_THREAD - Interrupt cannot be threaded
>   * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
>   *                resume time.
> + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user and it is
> + *                configured for system wakeup, execute this interrup handler
> + *                after suspending interrupts as it may be necessary to detect
> + *                wakeup. Users need to implement system wakeup detection in
> + *                their interrupt handlers.
>   */
>  #define IRQF_DISABLED		0x00000020
>  #define IRQF_SHARED		0x00000080
> @@ -70,6 +75,7 @@
>  #define IRQF_FORCE_RESUME	0x00008000
>  #define IRQF_NO_THREAD		0x00010000
>  #define IRQF_EARLY_RESUME	0x00020000
> +#define IRQF_COND_SUSPEND	0x00040000
>  
>  #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
>  
> @@ -101,6 +107,7 @@ typedef irqreturn_t (*irq_handler_t)(int
>   * @thread_flags:	flags related to @thread
>   * @thread_mask:	bitmask for keeping track of @thread activity
>   * @dir:	pointer to the proc/irq/NN/name entry
> + * @saved_handler:	address of the original interrupt handler function
>   */
>  struct irqaction {
>  	irq_handler_t		handler;
> @@ -115,6 +122,9 @@ struct irqaction {
>  	unsigned long		thread_mask;
>  	const char		*name;
>  	struct proc_dir_entry	*dir;
> +#ifdef CONFIG_PM_SLEEP
> +	irq_handler_t		saved_handler;
> +#endif
>  } ____cacheline_internodealigned_in_smp;
>  
>  extern irqreturn_t no_action(int cpl, void *dev_id);
> Index: linux-pm/kernel/irq/pm.c
> ===================================================================
> --- linux-pm.orig/kernel/irq/pm.c
> +++ linux-pm/kernel/irq/pm.c
> @@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_de
>  
>  	if (action->flags & IRQF_NO_SUSPEND)
>  		desc->no_suspend_depth++;
> -
> -	WARN_ON_ONCE(desc->no_suspend_depth &&
> -		     desc->no_suspend_depth != desc->nr_actions);
>  }
>  
>  /*
> @@ -63,11 +60,53 @@ void irq_pm_remove_action(struct irq_des
>  		desc->no_suspend_depth--;
>  }
>  
> +static irqreturn_t irq_pm_null_handler(int irq, void *context)
> +{
> +	return IRQ_NONE;
> +}
> +
> +static void prepare_no_suspend_irq(struct irq_desc *desc)
> +{
> +	struct irqaction *action;
> +
> +	for (action = desc->action; action; action = action->next) {
> +		if (action->flags & IRQF_NO_SUSPEND)
> +			continue;
> +
> +		if ((action->flags & IRQF_COND_SUSPEND) &&
> +		    irqd_is_wakeup_set(&desc->irq_data))
> +			continue;
> +
> +		action->saved_handler = action->handler;
> +		action->handler = irq_pm_null_handler;
> +	}
> +}
> +
> +static void restore_no_suspend_irq(struct irq_desc *desc)
> +{
> +	struct irqaction *action;
> +
> +	for (action = desc->action; action; action = action->next) {
> +		if (action->flags & IRQF_NO_SUSPEND)
> +			continue;
> +
> +		if (action->saved_handler) {
> +			action->handler = action->saved_handler;
> +			action->saved_handler = NULL;
> +		}
> +	}
> +}
> +
>  static bool suspend_device_irq(struct irq_desc *desc, int irq)
>  {
> -	if (!desc->action || desc->no_suspend_depth)
> +	if (!desc->action)
>  		return false;
>  
> +	if (desc->no_suspend_depth) {
> +		prepare_no_suspend_irq(desc);
> +		return false;
> +	}
> +
>  	if (irqd_is_wakeup_set(&desc->irq_data)) {
>  		irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
>  		/*
> @@ -135,6 +174,8 @@ static void resume_irq(struct irq_desc *
>  	if (desc->istate & IRQS_SUSPENDED)
>  		goto resume;
>  
> +	restore_no_suspend_irq(desc);
> +
>  	/* Force resume the interrupt? */
>  	if (!desc->force_resume_depth)
>  		return;
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com