[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Joe Perches]

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?

---
 arch/arm/include/asm/delay.h | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

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);

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Arnd Bergmann]

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

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Joe Perches]

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.

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Joe Perches]

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.

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Russell King - ARM Linux]

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?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Joe Perches]

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().

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Russell King - ARM Linux]

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. :)

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Joe Perches]

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.

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[ath9k-devel] [PATCH 1/3] ath9k: Fix build error on ARM

[Joe Perches]

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.