On Thursday, July 24, 2014 11:26:20 PM Peter Zijlstra wrote:
> Subject: irq: Rework IRQF_NO_SUSPENDED
> From: Peter Zijlstra <pet...@infradead.org>
> Date: Thu Jul 24 22:34:50 CEST 2014
>
> Typically when devices are suspended they're quiesced such that they
> will not generate any further interrupts.
>
> However some devices should still generate interrupts, even when
> suspended, typically used to wake the machine back up.
>
> Such logic should ideally be contained inside each driver, if it can
> generate interrupts when suspended, it knows about this and the
> interrupt handler can deal with it.
>
> Except of course for shared interrupts, when such a wakeup device is
> sharing an interrupt line with a device that does not expect
> interrupts while suspended things can go funny.
>
> This is where IRQF_NO_SUSPEND comes in, the idea is that drivers that
> have the capability to wake the machine set IRQF_NO_SUSPEND and their
> handler will still be called, even when the IRQ subsystem is formally
> suspended. Handlers without IRQF_NO_SUSPEND will not be called.
>
> This replaced the prior implementation of IRQF_NO_SUSPEND which had
> a number of fatal issues in that it didn't actually work for the
> shared case, exactly the case it should be helping.
>
> There is still enable_irq_wake()/IRQD_WAKEUP_STATE that tries to serve
> a similar purpose but is equially wrecked for shared interrupts,
> ideally this would be removed.
Let me comment about this particular thing.
I had a discussion with Dmitry about that and his argument was that
enable_irq_wake() should imply IRQF_NO_SUSPEND, because drivers that
set up interrupts for system wakeup should expect those interrupts to
trigger at any time, including system suspend. Hence the patch that
added the IRQD_WAKEUP_STATE check to __disable_irq().
However, in the face of the problem that is being addressed here I'm
not really sure that this argument is valid, because if the driver
calling enable_irq_wake() is sharing the IRQ with another one, the
other driver may not actually know that the IRQ will be a wakeup one
and still may not expect interrupts to come to it during system
suspend/resume.
Yes, drivers using enable_irq_wake() will likely want IRQF_NO_SUSPEND to
be set for their irqactions, but that should not imply "no suspend" for
all irqactions sharing the same desc. So I guess it may be better to go
forth and use a global "interrupts suspended" state variable instead of the
IRQS_SUSPENDED flag for each desc and throw away the IRQD_WAKEUP_STATE
check from suspend_device_irqs() entirely.
Peter, it looks like you'd prefer that?
Rafael
> Cc: r...@rjwysocki.net
> Signed-off-by: Peter Zijlstra <pet...@infradead.org>
> ---
> kernel/irq/chip.c | 4 +---
> kernel/irq/handle.c | 24 +++++++++++++++++++++---
> kernel/irq/internals.h | 6 ++++--
> kernel/irq/manage.c | 31 ++++++-------------------------
> kernel/irq/pm.c | 17 +++++++++--------
> 5 files changed, 41 insertions(+), 41 deletions(-)
>
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -677,9 +677,7 @@ void handle_percpu_devid_irq(unsigned in
> if (chip->irq_ack)
> chip->irq_ack(&desc->irq_data);
>
> - trace_irq_handler_entry(irq, action);
> - res = action->handler(irq, dev_id);
> - trace_irq_handler_exit(irq, action, res);
> + res = do_irqaction(desc, action, irq, dev_id);
>
> if (chip->irq_eoi)
> chip->irq_eoi(&desc->irq_data);
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -131,6 +131,23 @@ void __irq_wake_thread(struct irq_desc *
> }
>
> irqreturn_t
> +do_irqaction(struct irq_desc *desc, struct irqaction *action,
> + unsigned int irq, void *dev_id)
> +{
> + irqreturn_t ret;
> +
> + if (unlikely((desc->istate & IRQS_SUSPENDED) &&
> + !(action->flags & IRQF_NO_SUSPEND)))
> + return IRQ_NONE;
> +
> + trace_irq_handler_entry(irq, action);
> + ret = action->handler(irq, dev_id);
> + trace_irq_handler_exit(irq, action, ret);
> +
> + return ret;
> +}
> +
> +irqreturn_t
> handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> {
> irqreturn_t retval = IRQ_NONE;
> @@ -139,9 +156,7 @@ handle_irq_event_percpu(struct irq_desc
> do {
> irqreturn_t res;
>
> - trace_irq_handler_entry(irq, action);
> - res = action->handler(irq, action->dev_id);
> - trace_irq_handler_exit(irq, action, res);
> + res = do_irqaction(desc, action, irq, action->dev_id);
>
> if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled
> interrupts\n",
> irq, action->handler))
> @@ -175,6 +190,9 @@ handle_irq_event_percpu(struct irq_desc
>
> add_interrupt_randomness(irq, flags);
>
> + if (unlikely(desc->istate & IRQS_SUSPENDED) && retval == IRQ_NONE)
> + retval = IRQ_HANDLED;
> +
> if (!noirqdebug)
> note_interrupt(irq, desc, retval);
> return retval;
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -63,8 +63,10 @@ enum {
>
> extern int __irq_set_trigger(struct irq_desc *desc, unsigned int irq,
> unsigned long flags);
> -extern void __disable_irq(struct irq_desc *desc, unsigned int irq, bool
> susp);
> -extern void __enable_irq(struct irq_desc *desc, unsigned int irq, bool
> resume);
> +
> +extern irqreturn_t
> +do_irqaction(struct irq_desc *desc, struct irqaction *action,
> + unsigned int irq, void *dev_id);
>
> extern int irq_startup(struct irq_desc *desc, bool resend);
> extern void irq_shutdown(struct irq_desc *desc);
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -382,15 +382,8 @@ setup_affinity(unsigned int irq, struct
> }
> #endif
>
> -void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
> +void __disable_irq(struct irq_desc *desc, unsigned int irq)
> {
> - if (suspend) {
> - if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND) ||
> - irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
> - return;
> - desc->istate |= IRQS_SUSPENDED;
> - }
> -
> if (!desc->depth++)
> irq_disable(desc);
> }
> @@ -402,7 +395,7 @@ static int __disable_irq_nosync(unsigned
>
> if (!desc)
> return -EINVAL;
> - __disable_irq(desc, irq, false);
> + __disable_irq(desc, irq);
> irq_put_desc_busunlock(desc, flags);
> return 0;
> }
> @@ -443,20 +436,8 @@ void disable_irq(unsigned int irq)
> }
> EXPORT_SYMBOL(disable_irq);
>
> -void __enable_irq(struct irq_desc *desc, unsigned int irq, bool resume)
> +void __enable_irq(struct irq_desc *desc, unsigned int irq)
> {
> - if (resume) {
> - if (!(desc->istate & IRQS_SUSPENDED)) {
> - if (!desc->action)
> - return;
> - if (!(desc->action->flags & IRQF_FORCE_RESUME))
> - return;
> - /* Pretend that it got disabled ! */
> - desc->depth++;
> - }
> - desc->istate &= ~IRQS_SUSPENDED;
> - }
> -
> switch (desc->depth) {
> case 0:
> err_out:
> @@ -498,7 +479,7 @@ void enable_irq(unsigned int irq)
> KERN_ERR "enable_irq before setup/request_irq: irq %u\n", irq))
> goto out;
>
> - __enable_irq(desc, irq, false);
> + __enable_irq(desc, irq);
> out:
> irq_put_desc_busunlock(desc, flags);
> }
> @@ -1079,7 +1060,7 @@ __setup_irq(unsigned int irq, struct irq
> */
>
> #define IRQF_MISMATCH \
> - (IRQF_TRIGGER_MASK | IRQF_ONESHOT | IRQF_NO_SUSPEND)
> + (IRQF_TRIGGER_MASK | IRQF_ONESHOT)
>
> if (!((old->flags & new->flags) & IRQF_SHARED) ||
> ((old->flags ^ new->flags) & IRQF_MISMATCH))
> @@ -1232,7 +1213,7 @@ __setup_irq(unsigned int irq, struct irq
> */
> if (shared && (desc->istate & IRQS_SPURIOUS_DISABLED)) {
> desc->istate &= ~IRQS_SPURIOUS_DISABLED;
> - __enable_irq(desc, irq, false);
> + __enable_irq(desc, irq);
> }
>
> raw_spin_unlock_irqrestore(&desc->lock, flags);
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -29,14 +29,20 @@ void suspend_device_irqs(void)
> for_each_irq_desc(irq, desc) {
> unsigned long flags;
>
> + /*
> + * Ideally this would be a global state, but we cannot
> + * for the trainwreck that is IRQD_WAKEUP_STATE.
> + */
> raw_spin_lock_irqsave(&desc->lock, flags);
> - __disable_irq(desc, irq, true);
> + if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
> + desc->istate |= IRQS_SUSPENDED;
> raw_spin_unlock_irqrestore(&desc->lock, flags);
> }
>
> - for_each_irq_desc(irq, desc)
> + for_each_irq_desc(irq, desc) {
> if (desc->istate & IRQS_SUSPENDED)
> synchronize_irq(irq);
> + }
> }
> EXPORT_SYMBOL_GPL(suspend_device_irqs);
>
> @@ -47,14 +53,9 @@ static void resume_irqs(bool want_early)
>
> for_each_irq_desc(irq, desc) {
> unsigned long flags;
> - bool is_early = desc->action &&
> - desc->action->flags & IRQF_EARLY_RESUME;
> -
> - if (!is_early && want_early)
> - continue;
>
> raw_spin_lock_irqsave(&desc->lock, flags);
> - __enable_irq(desc, irq, true);
> + desc->istate &= ~IRQS_SUSPENDED;
> raw_spin_unlock_irqrestore(&desc->lock, flags);
> }
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
