| Message ID | 1391531796.2538.21.camel@joe-AO722 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 04 February 2014 08:36:36 Joe Perches wrote: > On Tue, 2014-02-04 at 08:03 +0100, Holger Schurig wrote: > > Joe, look in linux/arch/arm/include/asm/delay.h. The macro udelay > > cannot handle large values because of lost-of-precision. > > > > IMHO udelay on ARM is broken, because it also cannot work with fast > > ARM processors (where bogomips >= 3355, which is in sight now). It's > > just not broken enought that someone did something against it ... so > > the current kludge is good enought. > > Maybe something like this would be better? > I actually like the fact that we get link errors for insane 'udelay' times. In most cases it's a driver bug because we shouldn't keep the CPU busy for an eternity in the kernel (and call msleep() instead). For the rare cases where mdelay makes sense, we also want to add a comment to the code explaining why msleep cannot be used. Arnd
On Tue, 2014-02-04 at 19:40 +0100, Arnd Bergmann wrote: > On Tuesday 04 February 2014 08:36:36 Joe Perches wrote: > > On Tue, 2014-02-04 at 08:03 +0100, Holger Schurig wrote: > > > Joe, look in linux/arch/arm/include/asm/delay.h. The macro udelay > > > cannot handle large values because of lost-of-precision. > > > > > > IMHO udelay on ARM is broken, because it also cannot work with fast > > > ARM processors (where bogomips >= 3355, which is in sight now). It's > > > just not broken enought that someone did something against it ... so > > > the current kludge is good enought. > > > > Maybe something like this would be better? > > > > I actually like the fact that we get link errors for insane 'udelay' > times. For static values yes, for computed values no. This emits a warning/dump_stack on the non-static values. It could also emit some similar #warning on static values > insane too.
On Tue, Feb 04, 2014 at 08:36:36AM -0800, Joe Perches wrote: > On Tue, 2014-02-04 at 08:03 +0100, Holger Schurig wrote: > > Joe, look in linux/arch/arm/include/asm/delay.h. The macro udelay > > cannot handle large values because of lost-of-precision. > > > > IMHO udelay on ARM is broken, because it also cannot work with fast > > ARM processors (where bogomips >= 3355, which is in sight now). It's > > just not broken enought that someone did something against it ... so > > the current kludge is good enought. > > Maybe something like this would be better? No, the point of __bad_udelay() is that people doing stupidly large udelay()s result in build errors, rather than having to run the kernel and trip over a non-existent debugging message beacuse they haven't built the kernel with DEBUG defined. NAK.
On Wed, 2014-02-05 at 11:50 +0000, Russell King - ARM Linux wrote: > On Tue, Feb 04, 2014 at 08:36:36AM -0800, Joe Perches wrote: > > On Tue, 2014-02-04 at 08:03 +0100, Holger Schurig wrote: > > > Joe, look in linux/arch/arm/include/asm/delay.h. The macro udelay > > > cannot handle large values because of lost-of-precision. > > > > > > IMHO udelay on ARM is broken, because it also cannot work with fast > > > ARM processors (where bogomips >= 3355, which is in sight now). It's > > > just not broken enought that someone did something against it ... so > > > the current kludge is good enought. > > > > Maybe something like this would be better? > > No, the point of __bad_udelay() is that people doing stupidly large > udelay()s result in build errors, Apparently, people just convert stupidly large udelay()s to mdelay and not be bothered. > rather than having to run the kernel > and trip over a non-existent debugging message beacuse they haven't > built the kernel with DEBUG defined. > > NAK. <shrug> Perhaps there should be some runtime udelay > maximum supported check.
On Wed, Feb 05, 2014 at 04:32:46AM -0800, Joe Perches wrote: > On Wed, 2014-02-05 at 11:50 +0000, Russell King - ARM Linux wrote: > > On Tue, Feb 04, 2014 at 08:36:36AM -0800, Joe Perches wrote: > > > On Tue, 2014-02-04 at 08:03 +0100, Holger Schurig wrote: > > > > Joe, look in linux/arch/arm/include/asm/delay.h. The macro udelay > > > > cannot handle large values because of lost-of-precision. > > > > > > > > IMHO udelay on ARM is broken, because it also cannot work with fast > > > > ARM processors (where bogomips >= 3355, which is in sight now). It's > > > > just not broken enought that someone did something against it ... so > > > > the current kludge is good enought. > > > > > > Maybe something like this would be better? > > > > No, the point of __bad_udelay() is that people doing stupidly large > > udelay()s result in build errors, > > Apparently, people just convert stupidly large udelay()s > to mdelay and not be bothered. And that's the correct answer. Having udelay(10000) rather than mdelay(10) is a sign that they weren't paying that much attention when writing the code. > Perhaps there should be some runtime udelay > maximum supported check. Having both a runtime check _and_ a compile time check would actually be a good thing, but any runtime check needs to be suitably rate- limited. The compile time check is very important because it catches a lot of cases which wouldn't otherwise be found (eg, in drivers which hardly anyone uses on ARM.) Maybe the compile time check should be something which is implemented in a cross-architecture way in linux/delay.h with the maximum set to the lowest that any architecture can do?
On Wed, 2014-02-05 at 12:41 +0000, Russell King - ARM Linux wrote: > On Wed, Feb 05, 2014 at 04:32:46AM -0800, Joe Perches wrote: > > Apparently, people just convert stupidly large udelay()s > > to mdelay and not be bothered. > > And that's the correct answer. Having udelay(10000) rather than mdelay(10) > is a sign that they weren't paying that much attention when writing the > code. Not really. Look at the code that brought this up in the first place. On Tue, 2014-02-04 at 08:37 +0530, Sujith Manoharan wrote: > From: Sujith Manoharan <c_manoha@qca.qualcomm.com> > > Use mdelay instead of udelay to fix this error: > > ERROR: "__bad_udelay" [drivers/net/wireless/ath/ath9k/ath9k_hw.ko] undefined! [] diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c [] > @@ -1316,7 +1316,7 @@ static bool ath9k_hw_set_reset(struct ath_hw *ah, int type) > if (AR_SREV_9300_20_OR_LATER(ah)) > udelay(50); > else if (AR_SREV_9100(ah)) > - udelay(10000); > + mdelay(10); > else > udelay(100); > > > > if (AR_SREV_9300_20_OR_LATER(ah)) > > udelay(50); > > else if (AR_SREV_9100(ah)) > > - udelay(10000); > > + mdelay(10); > > else > > udelay(100); One chip needs a larger delay than the others. It's not so much not paying attention as not knowing ARM is broken for large udelay().
On Wed, Feb 05, 2014 at 05:04:54AM -0800, Joe Perches wrote: > On Wed, 2014-02-05 at 12:41 +0000, Russell King - ARM Linux wrote: > > On Wed, Feb 05, 2014 at 04:32:46AM -0800, Joe Perches wrote: > > > Apparently, people just convert stupidly large udelay()s > > > to mdelay and not be bothered. > > > > And that's the correct answer. Having udelay(10000) rather than mdelay(10) > > is a sign that they weren't paying that much attention when writing the > > code. > > Not really. > > Look at the code that brought this up in the first place. > > On Tue, 2014-02-04 at 08:37 +0530, Sujith Manoharan wrote: > > From: Sujith Manoharan <c_manoha@qca.qualcomm.com> > > > > Use mdelay instead of udelay to fix this error: > > > > ERROR: "__bad_udelay" [drivers/net/wireless/ath/ath9k/ath9k_hw.ko] undefined! > [] > diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c > [] > > @@ -1316,7 +1316,7 @@ static bool ath9k_hw_set_reset(struct ath_hw *ah, int type) > > if (AR_SREV_9300_20_OR_LATER(ah)) > > udelay(50); > > else if (AR_SREV_9100(ah)) > > - udelay(10000); > > + mdelay(10); > > else > > udelay(100); > > > > > > > if (AR_SREV_9300_20_OR_LATER(ah)) > > > udelay(50); > > > else if (AR_SREV_9100(ah)) > > > - udelay(10000); > > > + mdelay(10); > > > else > > > udelay(100); > > One chip needs a larger delay than the others. > > It's not so much not paying attention as not > knowing ARM is broken for large udelay(). And now read my suggestion about how to avoid the "not knowing" problem. :)
On Wed, 2014-02-05 at 13:39 +0000, Russell King - ARM Linux wrote: > On Wed, Feb 05, 2014 at 05:04:54AM -0800, Joe Perches wrote: > > On Wed, 2014-02-05 at 12:41 +0000, Russell King - ARM Linux wrote: > > > On Wed, Feb 05, 2014 at 04:32:46AM -0800, Joe Perches wrote: > > > > Apparently, people just convert stupidly large udelay()s > > > > to mdelay and not be bothered. > > > > > > And that's the correct answer. Having udelay(10000) rather than mdelay(10) > > > is a sign that they weren't paying that much attention when writing the > > > code. > > > > Not really. [] > > It's not so much not paying attention as not > > knowing ARM is broken for large udelay(). > > And now read my suggestion about how to avoid the "not knowing" problem. :) I'd read it already. I didn't and don't disagree. I still think adding a #warning on large static udelay()s would be sensible. Maybe adding another option like #define UDELAY_TOO_BIG_I_KNOW_ALREADY_DONT_BOTHER_ME guard to avoid seeing the #warning when there's no other option.
On Wed, Feb 05, 2014 at 06:03:13AM -0800, Joe Perches wrote: > On Wed, 2014-02-05 at 13:39 +0000, Russell King - ARM Linux wrote: > > On Wed, Feb 05, 2014 at 05:04:54AM -0800, Joe Perches wrote: > > > On Wed, 2014-02-05 at 12:41 +0000, Russell King - ARM Linux wrote: > > > > On Wed, Feb 05, 2014 at 04:32:46AM -0800, Joe Perches wrote: > > > > > Apparently, people just convert stupidly large udelay()s > > > > > to mdelay and not be bothered. > > > > > > > > And that's the correct answer. Having udelay(10000) rather than mdelay(10) > > > > is a sign that they weren't paying that much attention when writing the > > > > code. > > > > > > Not really. > [] > > > It's not so much not paying attention as not > > > knowing ARM is broken for large udelay(). > > > > And now read my suggestion about how to avoid the "not knowing" problem. :) > > I'd read it already. I didn't and don't disagree. Please explain /why/ you don't agree. > I still think adding a #warning on large static udelay()s > would be sensible. Maybe adding another option like > #define UDELAY_TOO_BIG_I_KNOW_ALREADY_DONT_BOTHER_ME > guard to avoid seeing the #warning when there's no other > option. How does that help? It's /not/ a warning situation - if you ask for udelay(10000) on ARM, you will /not/ get a 10ms delay. You'll instead get something much shorter because the arithmetic will overflow. Now, you could argue that maybe ARM's udelay() implementation should know about this and implement a loop around the underlying udelay() implementation to achieve it, and I might agree with you - but I don't for one very simple reason: we /already/ have such an implementation. It's called mdelay(): #ifndef mdelay #define mdelay(n) (\ (__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \ ({unsigned long __ms=(n); while (__ms--) udelay(1000);})) #endif So, the right answer is /not/ do duplicate this, but to /use/ the appropriate facilities provided by the kernel.
On Wed, 2014-02-05 at 14:21 +0000, Russell King - ARM Linux wrote: > On Wed, Feb 05, 2014 at 06:03:13AM -0800, Joe Perches wrote: > > On Wed, 2014-02-05 at 13:39 +0000, Russell King - ARM Linux wrote: > > > On Wed, Feb 05, 2014 at 05:04:54AM -0800, Joe Perches wrote: > > > > On Wed, 2014-02-05 at 12:41 +0000, Russell King - ARM Linux wrote: > > > > > On Wed, Feb 05, 2014 at 04:32:46AM -0800, Joe Perches wrote: > > > > > > Apparently, people just convert stupidly large udelay()s > > > > > > to mdelay and not be bothered. > > > > > > > > > > And that's the correct answer. Having udelay(10000) rather than mdelay(10) > > > > > is a sign that they weren't paying that much attention when writing the > > > > > code. > > > > > > > > Not really. > > [] > > > > It's not so much not paying attention as not > > > > knowing ARM is broken for large udelay(). > > > > > > And now read my suggestion about how to avoid the "not knowing" problem. :) > > > > I'd read it already. I didn't and don't disagree. > > Please explain /why/ you don't agree. Please reread what I wrote. We agree a lot more than you seem to think. > > I still think adding a #warning on large static udelay()s > > would be sensible. Maybe adding another option like > > #define UDELAY_TOO_BIG_I_KNOW_ALREADY_DONT_BOTHER_ME > > guard to avoid seeing the #warning when there's no other > > option. > > How does that help? It's /not/ a warning situation - if you ask for > udelay(10000) on ARM, you will /not/ get a 10ms delay. You'll instead > get something much shorter because the arithmetic will overflow. > Now, you could argue that maybe ARM's udelay() implementation should > know about this and implement a loop around the underlying udelay() > implementation to achieve it, I suggested something like that was possible. > and I might agree with you - but I > don't for one very simple reason: we /already/ have such an > implementation. It's called mdelay(): I think mdelay should be for the case where the range is too big for a udelay. I think arm's 11 bit range is unfortunately small. I think we agree be nice to get a generic compiler #warning when a __builtin_constant_p value is too large and a ratelimited dmesg/warning for the runtime case.
diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h index dff714d..ac33c56 100644 --- a/arch/arm/include/asm/delay.h +++ b/arch/arm/include/asm/delay.h @@ -15,6 +15,8 @@ #ifndef __ASSEMBLY__ +#include <linux/printk.h> + struct delay_timer { unsigned long (*read_current_timer)(void); unsigned long freq; @@ -51,11 +53,34 @@ extern void __bad_udelay(void); #define __udelay(n) arm_delay_ops.udelay(n) #define __const_udelay(n) arm_delay_ops.const_udelay(n) +#ifdef DEBUG +#define __udelay_debug_max_delay(n) \ +do { \ + if (n > MAX_UDELAY_MS * 1000) { \ + pr_debug("udelay(%d) too large - Convert to mdelay\n", n); \ + dump_stack(); \ + } \ +} while (0) +#else +#define __udelay_debug_max_delay(n) \ + do {} while (0) +#endif + #define udelay(n) \ - (__builtin_constant_p(n) ? \ - ((n) > (MAX_UDELAY_MS * 1000) ? __bad_udelay() : \ - __const_udelay((n) * UDELAY_MULT)) : \ - __udelay(n)) +({ \ + if (__builtin_constant_p(n)) { \ + typeof n _n = n; \ + while (_n > MAX_UDELAY_MS * 1000) { \ + __const_udelay(MAX_UDELAY_MS * 1000 * UDELAY_MULT); \ + _n -= MAX_UDELAY_MS * 1000; \ + } \ + if (_n) \ + __const_udelay(_n * 1000 * UDELAY_MULT); \ + } else { \ + __udelay_debug_max_delay(n); \ + __udelay(n); \ + } \ +}) /* Loop-based definitions for assembly code. */ extern void __loop_delay(unsigned long loops);