On Monday 23 February 2009, Ingo Molnar wrote: > > * Rafael J. Wysocki <rjw@xxxxxxx> 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 _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
