PM: Avoid losing wakeup events during suspend
in [Kernel]
|
Prev: [PATCH] [RFC] kvm: time accelerator/decelerator for kvm guests
Next: wait_for_completion_interruptible does not wait !!
From: Rafael J. Wysocki on 25 Jun 2010 09:40 On Friday, June 25, 2010, Florian Mickler wrote: > On Thu, 24 Jun 2010 13:09:27 -0400 (EDT) > Alan Stern <stern(a)rowland.harvard.edu> wrote: > > > > > This requires you to define an explicit PCI_WAKEUP_COOLDOWN delay. I > > > > think that's okay; I had to do something similar with USB and SCSI. > > > > (And I still think it would be a good idea to prevent workqueue threads > > > > from freezing until their queues are empty.) > > I'm not that familiar with the freezer, but couldn't it be > deadlocky if the work depends on some already frozen part? No, in the case of freezable workqueues (which is the one we're discussing) they generally can't depend on anything freezable, because it's never known which freezable tasks will be frozen first. > What about a new work-type that calls > pm_relax() after executing it's workfunction and executing > pm_stay_awake() on enqueue? That might be useful., although it doesn't really help here, because there still is a window between queuing up a work item and executing it. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 25 Jun 2010 16:40 On Friday, June 25, 2010, Alan Stern wrote: > On Fri, 25 Jun 2010, Rafael J. Wysocki wrote: > > > So, there it goes. > > > > I decided not to play with memory allocations at this point, because I really > > don't expect pm_wakeup_event() to be heavily used initially. If there are more > > users and it's called more frequently, we can always switch to using a separate > > slab cache. > > > > Hopefully, I haven't overlooked anything vitally important this time. > > > > Please tell me what you think. > > Obviously comments still need to be added. Indeed. > Beyond that... > > > --- /dev/null > > +++ linux-2.6/drivers/base/power/wakeup.c > > @@ -0,0 +1,143 @@ > > + > > +#include <linux/device.h> > > +#include <linux/slab.h> > > +#include <linux/sched.h> > > +#include <linux/capability.h> > > +#include <linux/pm.h> > > + > > +bool events_check_enabled; > > + > > +static unsigned long event_count; > > +static unsigned long saved_event_count; > > +static unsigned long events_in_progress; > > +static spinlock_t events_lock; > > Use static DEFINE_SPINLOCK(events_lock) instead. Hmm. I thought that was deprecated. Never mind. > > +static DECLARE_WAIT_QUEUE_HEAD(events_wait_queue); > > + > > +void pm_wakeup_events_init(void) > > +{ > > + spin_lock_init(&events_lock); > > +} > > Then this routine won't be needed. > > > +unsigned long pm_dev_wakeup_count(struct device *dev) > > +{ > > + unsigned long flags; > > + unsigned long count; > > + > > + spin_lock_irqsave(&events_lock, flags); > > + count = dev->power.wakeup_count; > > + spin_unlock_irqrestore(&events_lock, flags); > > + return count; > > +} > > Are the spin_lock calls needed here? I doubt it. No, they aren't. In fact it may be a static inline. > > --- linux-2.6.orig/kernel/power/power.h > > +++ linux-2.6/kernel/power/power.h > > @@ -184,6 +184,15 @@ static inline void suspend_test_finish(c > > #ifdef CONFIG_PM_SLEEP > > /* kernel/power/main.c */ > > extern int pm_notifier_call_chain(unsigned long val); > > + > > +/* drivers/base/power/wakeup.c */ > > +extern bool events_check_enabled; > > + > > +extern void pm_wakeup_events_init(void); > > +extern bool pm_check_wakeup_events(void); > > +extern bool pm_check_wakeup_events_final(void); > > +extern bool pm_get_wakeup_count(unsigned long *count); > > +extern bool pm_save_wakeup_count(unsigned long count); > > #endif > > This is unfortunate. These declarations belong in a file that can > also be #included by drivers/base/power/wakeup.c. Otherwise future > changes might cause type mismatches the compiler won't be able to > catch. You're right. In that case I think include/linux/suspend.h is the right header to put them into. > > @@ -511,18 +513,24 @@ int hibernation_platform_enter(void) > > > > local_irq_disable(); > > sysdev_suspend(PMSG_HIBERNATE); > > + if (!pm_check_wakeup_events()) { > > + error = -EAGAIN; > > + goto Power_up; > > + } > > + > > hibernation_ops->enter(); > > /* We should never get here */ > > while (1); > > > > - /* > > - * We don't need to reenable the nonboot CPUs or resume consoles, since > > - * the system is going to be halted anyway. > > - */ > > + Power_up: > > + sysdev_resume(); > > + local_irq_enable(); > > + enable_nonboot_cpus(); > > + > > Platform_finish: > > hibernation_ops->finish(); > > > > - dpm_suspend_noirq(PMSG_RESTORE); > > + dpm_resume_noirq(PMSG_RESTORE); > > Is this a bug fix that crept in along with the other changes? Yeah. > > --- linux-2.6.orig/drivers/pci/pci.h > > +++ linux-2.6/drivers/pci/pci.h > > @@ -6,6 +6,8 @@ > > #define PCI_CFG_SPACE_SIZE 256 > > #define PCI_CFG_SPACE_EXP_SIZE 4096 > > > > +#define PCI_WAKEUP_COOLDOWN 100 > > This definition can go directly in pci.c, since it isn't used anywhere > else. OK Thanks for the comments, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 25 Jun 2010 16:40
On Friday, June 25, 2010, Alan Stern wrote: > On Fri, 25 Jun 2010, Rafael J. Wysocki wrote: > > > > That's not the point. If a wakeup handler queues a work item (for > > > example, by calling pm_request_resume) then it wouldn't need to guess a > > > timeout. The work item would be guaranteed to run before the system > > > could suspend again. > > > > You seem to be referring to the PM workqueue specifically. Perhaps it would be > > better to special-case it and stop it by adding a barrier work during suspend > > instead of just freezing? Then, it wouldn't need to be singlethread any more. > > The barrier work would have to be queued to each CPU's thread. That > would be okay. I guess we should stop the PM workqueue after the freezing of tasks, shouldn't we? > Hmm, it looks like wait_event_freezable() and > wait_event_freezable_timeout() could use similar changes: If the > condition is true then they shouldn't try to freeze the caller. Yes, but that should be a separate patch IMHO. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |