Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: [RFC][PATCH 2/2] PM: Rework handling of interrupts during suspend-resume | Date | Mon, 23 Feb 2009 15:48:11 +0100 |
| |
On Monday 23 February 2009, Ingo Molnar wrote: > > * Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > > > Index: linux-2.6/arch/x86/kernel/apm_32.c > > > > =================================================================== > > > > --- linux-2.6.orig/arch/x86/kernel/apm_32.c > > > > +++ linux-2.6/arch/x86/kernel/apm_32.c > > > > > > > + > > > > + suspend_device_irqs(); > > > > device_power_down(PMSG_SUSPEND); > > > > + > > > > + local_irq_disable(); > > > > > > hm, this is a very repetitive pattern, all around the various > > > suspend/resume variants. Might make sense to make: > > > > > > device_power_down(PMSG_SUSPEND); > > > > > > do the irq line disabling plus the local irq disabling > > > automatically. That also means it cannot be forgotten. The > > > symmetric action should happen for PMSG_RESUME. > > > > > > Is there ever a case where we want a different pattern? > > > > Even if there's no such case, I prefer to call > > local_irq_disable() explicitly in here, so that it's clearly > > known where it happens to anyone reading this code. > > That property can be implied in the function name: > > device_power_down_irq_disable(PMSG_SUSPEND); > > Open-coding it, if it looks the same in all the cases just > increases the chances that someone somewhere copies them > incorrectly.
Well, I see your point, but in that case I'd rather couple the disabling of local interrupts on the CPU with sysdev_suspend and the disabling (or whatever Eric would like to call that) of device interrupts with device_power_down().
Thanks, Rafael
| |