From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [update 3] Re: [RFC][PATCH] PM: Avoid losing wakeup events during suspend Date: Fri, 25 Jun 2010 22:33:32 +0200 Message-ID: <201006252233.32784.rjw__13083.4941439845$1277498189$gmane$org@sisk.pl> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org To: Alan Stern Cc: mark gross <640e9920@gmail.com>, Neil Brown , Dmitry Torokhov , Linux Kernel Mailing List , Arve@smtp1.linux-foundation.org, Florian Mickler , Linux-pm mailing list List-Id: linux-pm@vger.kernel.org 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 > > +#include > > +#include > > +#include > > +#include > > + > > +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