2008-02-26 15:35:25

by Adrian Bunk

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

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 <[email protected]>

---

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



2008-02-26 17:20:24

by Martin Michlmayr

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

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

Tested-by: Martin Michlmayr <[email protected]>

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

2008-02-26 18:53:44

by Evgeniy Polyakov

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

Hi Adrian.

On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk ([email protected]) 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 <[email protected]>

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

Herbert, please apply.

ACK.

--
Evgeniy Polyakov

2008-02-27 00:10:38

by Andrew Morton

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

On Tue, 26 Feb 2008 21:52:40 +0300 Evgeniy Polyakov <[email protected]> wrote:

> Hi Adrian.
>
> On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk ([email protected]) 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 <[email protected]>
>
> 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)

2008-02-27 06:23:15

by Adrian Bunk

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

On Tue, Feb 26, 2008 at 04:04:39PM -0800, Andrew Morton wrote:
> On Tue, 26 Feb 2008 21:52:40 +0300 Evgeniy Polyakov <[email protected]> wrote:
>
> > Hi Adrian.
> >
> > On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk ([email protected]) 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 <[email protected]>
> >
> > 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

2008-02-27 06:47:19

by Andrew Morton

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

On Wed, 27 Feb 2008 08:22:07 +0200 Adrian Bunk <[email protected]> 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 <[email protected]> wrote:
> >
> > > Hi Adrian.
> > >
> > > On Tue, Feb 26, 2008 at 05:34:21PM +0200, Adrian Bunk ([email protected]) 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 <[email protected]>
> > >
> > > 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);
_

2008-02-27 16:00:07

by Evgeniy Polyakov

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

On Tue, Feb 26, 2008 at 04:04:39PM -0800, Andrew Morton ([email protected]) 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

2008-02-28 08:03:57

by Adrian Bunk

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

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