| Message ID | 1348727761-16485-1-git-send-email-fwu@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 27, 2012 at 02:36:01PM +0800, Fan Wu wrote: > From: fwu <fwu@marvell.com> > > 1. On ARM platform, "nohlt" can be used to prevent core from idle > process, returning immediately. > 2. There are two interface, exported for other modules, named > disable_hlt and enable_hlt and used to enable/disable the > cpuidle mechanism by increasing/decreasing "hlt_counter". > So, the more "hlt_counter" is, the more user want to disable > cpuidle. Disable_hlt and enable_hlt are paired operation, > when you first call disable_hlt and then enable_hlt, the > semantics are right, but if you call enable_hlt and > then disable_hlt, it is wrong. > 3. Change "hlt_counter > 0" can fix the problem. > The judgement whethere the cpuidle is disabled need to check > whether the "hlt_counter > 0" rather than "hlt_counter != 0". NAK. The bug is that you're calling enable_hlt() without first calling disable_hlt(). That is something you _must_ _not_ do. If the count starts off zero, and a driver calls disable_hlt(), another driver _must_ _not_ override that by then calling enable_hlt().
Thanks a lot for reviewing. So, you mean it is driver's responsibility to make sure the "disable and enable" function are paired before using it, which I think is NOT OK for current code. 1. If we want different users have chance to sync about the counter, I think we may try the following ways 1). add one interface (like exported function) for other modules or driver to get the current counter value . 2). add constraint in "enable and disable" function to avoid the possible situation that the counter is less/more than "0". 2. If we want the "nohlt" is OK for every driver and module without sync or similar operation, We may just remove "enable and disable" interface directly, which will cause the "nohlt" change will only be the init interface and cannot be changed any more after kernel bootup. I have patches to try above possible ways. However, I still think my current way(My filed patch) is easier and better way? What's your suggestion ? Thanks a lot ! Best Regards. Fan Wu(??) Ext: 9853 -----Original Message----- From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] Sent: 2012?9?27? 16:53 To: Fan Wu Cc: Nicolas Pitre; Will Deacon; H Hartley Sweeten; Tony Lindgren; linux-arm-kernel@lists.infradead.org; Lu Mao; Ning Jiang Subject: Re: [PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter On Thu, Sep 27, 2012 at 02:36:01PM +0800, Fan Wu wrote: > From: fwu <fwu@marvell.com> > > 1. On ARM platform, "nohlt" can be used to prevent core from idle > process, returning immediately. > 2. There are two interface, exported for other modules, named > disable_hlt and enable_hlt and used to enable/disable the > cpuidle mechanism by increasing/decreasing "hlt_counter". > So, the more "hlt_counter" is, the more user want to disable > cpuidle. Disable_hlt and enable_hlt are paired operation, > when you first call disable_hlt and then enable_hlt, the > semantics are right, but if you call enable_hlt and > then disable_hlt, it is wrong. > 3. Change "hlt_counter > 0" can fix the problem. > The judgement whethere the cpuidle is disabled need to check > whether the "hlt_counter > 0" rather than "hlt_counter != 0". NAK. The bug is that you're calling enable_hlt() without first calling disable_hlt(). That is something you _must_ _not_ do. If the count starts off zero, and a driver calls disable_hlt(), another driver _must_ _not_ override that by then calling enable_hlt().
On Thu, Sep 27, 2012 at 02:19:49AM -0700, Fan Wu wrote: > Thanks a lot for reviewing. > > So, you mean it is driver's responsibility to make sure the "disable > and enable" function are paired before using it, which I think is > NOT OK for current code. It is, and always has been. > 1. If we want different users have chance to sync about the counter, > I think we may try the following ways > 1). add one interface (like exported function) for other > modules or driver to get the current counter value . That is broken. > 2). add constraint in "enable and disable" function to > avoid the possible situation that the counter is > less/more than "0". Err what? > 2. If we want the "nohlt" is OK for every driver and module without > sync or similar operation, > We may just remove "enable and disable" interface directly, > which will cause the "nohlt" change will only be the init interface > and cannot be changed any more after kernel bootup. WTF. No. Look, it is _VERY_ simple. Drivers must _not_ call enable_hlt() without first having called disable_hlt() - and the number of enable_hlt()s must _NEVER_ be more than the number of times you've called disable_hlt(). That's exactly the same with other subsystems - eg, you must not call enable_irq() without having first called disable_irq(), and you must not call enable_irq() more times than you've called disable_irq(). It is senseless to export the nohlt count - doing so will _create_ bugs because if a driver were to ever use this value (which is a cumulative value) then it can call enable_hlt() more times than it's called disable_hlt() because of the interaction with another driver. When drivers comply with the interface, the "nohlt" command line argument works - it calls disable_hlt() once without a following enable_hlt(), which means that hlt (or rather, putting the CPU into a low power mode) is disabled at boot time and never enabled. So, if you have a driver which is calling enable_hlt() without first having called disable_hlt(), fix that driver. There is nothing wrong in the generic ARM code, and the generic ARM code can't help you work around the broken driver.
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index 693b744..4dc2fee 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -204,7 +204,7 @@ void cpu_idle(void) #ifdef CONFIG_PL310_ERRATA_769419 wmb(); #endif - if (hlt_counter) { + if (hlt_counter > 0) { local_irq_enable(); cpu_relax(); } else if (!need_resched()) {