[2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Adrian Bunk]

On Tue, Feb 26, 2008 at 01:21:00PM +0100, Martin Michlmayr wrote:
> With 2.6.25-rc3 and a config file with
> 
> CONFIG_CRYPTO_DEV_HIFN_795X=m
> CONFIG_CRYPTO_DEV_HIFN_795X_RNG=y
> 
> I get the following build error on at least ARM and MIPS:
> 
>   Building modules, stage 2.
>   MODPOST 759 modules
> ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!

Fix below.

> Martin Michlmayr

cu
Adrian


<--  snip  -->


Using ndelay() with a 64bit variable as parameter can result in build 
errors like the following on some 32bit systems when it results in a 
64bit division:

<--  snip  -->

 ...
  MODPOST 759 modules
ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!

<--  snip  -->

Reported by Martin Michlmayr.

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

40b45041ddc587c20b872a86a6a36952c28b02c7 diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 3110bf7..b1541c6 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -807,7 +807,7 @@ static int hifn_rng_data_present(struct hwrng *rng, int wait)
 		return 1;
 	if (!wait)
 		return 0;
-	ndelay(nsec);
+	ndelay((u32)nsec);
 	return 1;
 }
 

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Martin Michlmayr]

* Adrian Bunk <bunk@kernel.org> [2008-02-26 17:34]:
> >   MODPOST 759 modules
> > ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!
> 
> Fix below.

Tested-by: Martin Michlmayr <tbm@cyrius.com>

-- 
Martin Michlmayr
http://www.cyrius.com/

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Evgeniy Polyakov]

Hi Adrian.

On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk (bunk@kernel.org) wrote:
> Using ndelay() with a 64bit variable as parameter can result in build 
> errors like the following on some 32bit systems when it results in a 
> 64bit division:
> 
> <--  snip  -->
> 
>  ...
>   MODPOST 759 modules
> ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!
> 
> <--  snip  -->
> 
> Reported by Martin Michlmayr.
> 
> Signed-off-by: Adrian Bunk <bunk@kernel.org>

Yep, ndelay() uses division, thanks a lot Adrian for spotting this.

Herbert, please apply.

ACK.

-- 
	Evgeniy Polyakov

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Andrew Morton]

On Tue, 26 Feb 2008 21:52:40 +0300 Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:

> Hi Adrian.
> 
> On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk (bunk@kernel.org) wrote:
> > Using ndelay() with a 64bit variable as parameter can result in build 
> > errors like the following on some 32bit systems when it results in a 
> > 64bit division:
> > 
> > <--  snip  -->
> > 
> >  ...
> >   MODPOST 759 modules
> > ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!
> > 
> > <--  snip  -->
> > 
> > Reported by Martin Michlmayr.
> > 
> > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> 
> Yep, ndelay() uses division, thanks a lot Adrian for spotting this.

hm.  Where?

> Herbert, please apply.
> 
> ACK.

udelay() might be exposed to the same problem.  It would be better to fix
ndelay() and udelay() rather than callers.  It is reasonable to pass a u64
into ndelay() and to expect the build to not explode.

(Geeze macros suck)

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Adrian Bunk]

On Tue, Feb 26, 2008 at 04:04:39PM -0800, Andrew Morton wrote:
> On Tue, 26 Feb 2008 21:52:40 +0300 Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> 
> > Hi Adrian.
> > 
> > On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk (bunk@kernel.org) wrote:
> > > Using ndelay() with a 64bit variable as parameter can result in build 
> > > errors like the following on some 32bit systems when it results in a 
> > > 64bit division:
> > > 
> > > <--  snip  -->
> > > 
> > >  ...
> > >   MODPOST 759 modules
> > > ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!
> > > 
> > > <--  snip  -->
> > > 
> > > Reported by Martin Michlmayr.
> > > 
> > > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > 
> > Yep, ndelay() uses division, thanks a lot Adrian for spotting this.
> 
> hm.  Where?
>...

include/linux/delay.h:35

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Andrew Morton]

On Wed, 27 Feb 2008 08:22:07 +0200 Adrian Bunk <bunk@kernel.org> wrote:

> On Tue, Feb 26, 2008 at 04:04:39PM -0800, Andrew Morton wrote:
> > On Tue, 26 Feb 2008 21:52:40 +0300 Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> > 
> > > Hi Adrian.
> > > 
> > > On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk (bunk@kernel.org) wrote:
> > > > Using ndelay() with a 64bit variable as parameter can result in build 
> > > > errors like the following on some 32bit systems when it results in a 
> > > > 64bit division:
> > > > 
> > > > <--  snip  -->
> > > > 
> > > >  ...
> > > >   MODPOST 759 modules
> > > > ERROR: "__divdi3" [drivers/crypto/hifn_795x.ko] undefined!
> > > > 
> > > > <--  snip  -->
> > > > 
> > > > Reported by Martin Michlmayr.
> > > > 
> > > > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > > 
> > > Yep, ndelay() uses division, thanks a lot Adrian for spotting this.
> > 
> > hm.  Where?
> >...
> 
> include/linux/delay.h:35
> 

well found.

Something like this:


--- a/include/linux/delay.h~a
+++ a/include/linux/delay.h
@@ -7,10 +7,12 @@
  * Delay routines, using a pre-computed "loops_per_jiffy" value.
  */
 
-extern unsigned long loops_per_jiffy;
+#include <linux/kernel.h>
 
 #include <asm/delay.h>
 
+extern unsigned long loops_per_jiffy;
+
 /*
  * Using udelay() for intervals greater than a few milliseconds can
  * risk overflow for high loops_per_jiffy (high bogomips) machines. The
@@ -32,7 +34,11 @@ extern unsigned long loops_per_jiffy;
 #endif
 
 #ifndef ndelay
-#define ndelay(x)	udelay(((x)+999)/1000)
+static inline void ndelay(unsigned long x)
+{
+	udelay(DIV_ROUND_UP(x, 1000));
+}
+#define ndelay(x) ndelay(x)
 #endif
 
 void calibrate_delay(void);
_

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Adrian Bunk]

On Tue, Feb 26, 2008 at 10:47:19PM -0800, Andrew Morton wrote:
>...
> Something like this:

Generally agreed, but

> --- a/include/linux/delay.h~a
> +++ a/include/linux/delay.h
> @@ -7,10 +7,12 @@
>   * Delay routines, using a pre-computed "loops_per_jiffy" value.
>   */
>  
> -extern unsigned long loops_per_jiffy;
> +#include <linux/kernel.h>
>  
>  #include <asm/delay.h>
>  
> +extern unsigned long loops_per_jiffy;
> +
>...

this part of your patch breaks m68k:

<--  snip  -->

...
  CC      init/main.o
In file included from 
/home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/delay.h:12,
                 from 
/home/bunk/linux/kernel-2.6/git/linux-2.6/init/main.c:19:
include2/asm/delay.h: In function '__const_udelay':
include2/asm/delay.h:33: error: 'loops_per_jiffy' undeclared (first use in this function)
include2/asm/delay.h:33: error: (Each undeclared identifier is reported only once
include2/asm/delay.h:33: error: for each function it appears in.)
make[2]: *** [init/main.o] Error 1

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6.25 patch] drivers/crypto/hifn_795x.c: fix 64bit division

[Evgeniy Polyakov]

On Tue, Feb 26, 2008 at 04:04:39PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> udelay() might be exposed to the same problem.  It would be better to fix
> ndelay() and udelay() rather than callers.  It is reasonable to pass a u64
> into ndelay() and to expect the build to not explode.

Well, if you think it is resonable to pass u64 into function, which is 
supposed to sleep no more than several cpu cycles. I do not want to
start any kind of flame about it, but this looks like an overkill.

> (Geeze macros suck)

Absolutely.

-- 
	Evgeniy Polyakov