On Sunday 22 February 2009, Linus Torvalds wrote:
>
> On Sun, 22 Feb 2009, Rafael J. Wysocki wrote:
> >
> > Use these functions to rework the handling of interrupts during
> > suspend (hibernation) and resume. Namely, interrupts will only be
> > disabled on the CPU right before suspending sysdevs, while device
> > interrupts will be disabled (at the IO-APIC level), with the help of
> > the new helper function, before calling "late" suspend callbacks
> > provided by device drivers and analogously during resume.
>
> I think this patch is actually a bit too complicated.
>
> > +struct disabled_irq {
> > + struct list_head list;
> > + int irq;
> > +};
> > +
> > +static LIST_HEAD(resume_irqs_list);
> > +
> > +/**
> > + * enable_device_irqs - enable interrupts disabled by disable_device_irqs()
> > + *
> > + * Enable all interrupt lines previously disabled by disable_device_irqs()
> > + * that are on resume_irqs_list.
> > + */
> > +void enable_device_irqs(void)
> > +{
> > + struct disabled_irq *resume_irq, *tmp;
> > +
> > + list_for_each_entry_safe(resume_irq, tmp, &resume_irqs_list, list) {
> > + enable_irq(resume_irq->irq);
> > + list_del(&resume_irq->list);
> > + kfree(resume_irq);
> > + }
> > +}
>
> Don't do this whole separate list. Instead, just add a per-irq-descriptor
> flag to the desc->status field that says "suspended". IOW, just do
> something like
OK
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index f899b50..7bc2a31 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -65,6 +65,7 @@ typedef void (*irq_flow_handler_t)(unsigned int irq,
> #define IRQ_SPURIOUS_DISABLED 0x00800000 /* IRQ was disabled by the spurious trap */
> #define IRQ_MOVE_PCNTXT 0x01000000 /* IRQ migration from process context */
> #define IRQ_AFFINITY_SET 0x02000000 /* IRQ affinity was set from userspace*/
> +#define IRQ_SUSPENDED 0x04000000 /* IRQ has gone through suspend sequence */
>
> #ifdef CONFIG_IRQ_PER_CPU
> # define CHECK_IRQ_PER_CPU(var) ((var) & IRQ_PER_CPU)
>
> and then just make the suspend sequence do
>
> for_each_irq_desc(irq, desc) {
> .. check desc if we should disable it ..
> disable_irq(irq);
> desc->status |= IRQ_SUSPENDED;
> }
>
> and the resume sequence do
>
> for_each_irq_desc(irq, desc) {
> if (!(desc->status & IRQ_SUSPENDED))
> continue;
> desc->status &= ~IRQ_SUSPENDED;
> enabled_irq(irq);
> }
>
> And that simplifcation then gets rid of
>
> > +/**
> > + * disable_device_irqs - disable all enabled interrupt lines
> > + *
> > + * During system-wide suspend or hibernation device interrupts need to be
> > + * disabled at the chip level and this function is provided for this
> > + * purpose. It disables all interrupt lines that are enabled at the
> > + * moment and saves their numbers for enable_device_irqs().
> > + */
> > +int disable_device_irqs(void)
> > +{
> > + struct irq_desc *desc;
> > + int irq;
> > +
> > + for_each_irq_desc(irq, desc) {
> > + unsigned long flags;
> > + struct disabled_irq *resume_irq;
> > + struct irqaction *action;
> > + bool is_timer_irq;
> > +
> > + resume_irq = kzalloc(sizeof(*resume_irq), GFP_NOIO);
> > + if (!resume_irq) {
> > + enable_device_irqs();
> > + return -ENOMEM;
> > + }
>
> this just goes away.
>
> > + is_timer_irq = false;
> > + action = desc->action;
> > + while (action) {
> > + if (action->flags | IRQF_TIMER) {
> > + is_timer_irq = true;
> > + break;
> > + }
> > + action = action->next;
> > + }
>
> This is also pointless and wrong (and buggy). You should use '&' to
> test that flag, not '|',
Ouch, sorry.
> but more importantly, if you share interrupts with a timer irq, there's
> nothing sane the irq layer can do ANYWAY, so just ignore the whole problem.
> Just look at the first one, don't try to be clever, because your clever code
> doesn't buy anything at all.
>
> So get rid of the loop, and just do
>
> if (desc->action && !(desc->action->flags & IRQF_TIMER)) {
> desc->depth++;
> desc->status |= IRQ_DISABLED | IRQ_SUSPENDED;
> desc->chip->disable(irq);
> }
> spin_unlock_irqrestore(&desc->lock, flags);
>
> and you're done.
OK
> Also, I'd actually suggest that the whole "synchronize_irq()" be handled
> in a separate loop after the main one, so make that one just be
>
> for_each_irq_desc(irq, desc) {
> if (desc->status & IRQ_SUSPENDED)
> serialize_irq(irq);
> }
>
> at the end. No need for desc->lock, since the IRQ_SUSPENDED bit is stable.
OK
> Finally:
>
> > +extern int disable_device_irqs(void);
> > +extern void enable_device_irqs(void);
>
> I think the naming is not great. It's not about disable/enable, it's very
> much about suspend/resume. In your version, it had that global
> "disabled_irq" list, and in mine it has that IRQ_SUSPENDED bit - and in
> both cases you can't nest things, and you can't consider them in any way
> "generic" enable/disable things, they are very specialized "shut up
> everything but the timer irq".
OK, would
extern void suspend_device_irqs(void);
extern void resume_device_irqs(void);
be better?
> I also don't think there is any reasonable error case, so just make the
> "suspend" thing return 'void', and don't complicate the caller. We don't
> error out on the simple "disable_irq()" either. It's a imperative
> statement, not a "please can you try to do that" thing.
The error is there just because the memory allocation can fail. With the
IRQ_SUSPENDED flag as per your suggestion it won't be necessary any more.
Thanks a lot for your comments, I'll send an updated patch shortly.
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
