[PATCH] joydev: fix HZ->millisecond transformation

[PATCH] joydev: fix HZ->millisecond transformation

[Bjorn Helgaas]

* fix a problem with HZ->millisecond transformation on
  non-x86 archs (from 2.5 change by vojtech@suse.cz)

Applies to 2.4.20.

diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
--- a/drivers/input/joydev.c	Mon Dec 16 12:16:32 2002
+++ b/drivers/input/joydev.c	Mon Dec 16 12:16:32 2002
@@ -50,6 +50,8 @@
 #define JOYDEV_MINORS		32
 #define JOYDEV_BUFFER_SIZE	64
 
+#define MSECS(t)	(1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
+
 struct joydev {
 	int exist;
 	int open;
@@ -134,7 +136,7 @@
 			return;
 	}  
 
-	event.time = jiffies * (1000 / HZ);
+	event.time = MSECS(jiffies);
 
 	while (list) {
 
@@ -279,7 +281,7 @@
 
 		struct js_event event;
 
-		event.time = jiffies * (1000/HZ);
+		event.time = MSECS(jiffies);
 
 		if (list->startup < joydev->nkey) {
 			event.type = JS_EVENT_BUTTON | JS_EVENT_INIT;

Re: [PATCH] joydev: fix HZ->millisecond transformation

[george anzinger]

Bjorn Helgaas wrote:
> 
> * fix a problem with HZ->millisecond transformation on
>   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> 
> Applies to 2.4.20.
> 
> diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> @@ -50,6 +50,8 @@
>  #define JOYDEV_MINORS          32
>  #define JOYDEV_BUFFER_SIZE     64
> 
> +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
Uh...                                                
^^^^^^^^^^^^^^^^
by definition this is zero, is it not?

> +
>  struct joydev {
>         int exist;
>         int open;
> @@ -134,7 +136,7 @@
>                         return;
>         }
> 
> -       event.time = jiffies * (1000 / HZ);
> +       event.time = MSECS(jiffies);
> 
>         while (list) {
> 
> @@ -279,7 +281,7 @@
> 
>                 struct js_event event;
> 
> -               event.time = jiffies * (1000/HZ);
> +               event.time = MSECS(jiffies);
> 
>                 if (list->startup < joydev->nkey) {
>                         event.type = JS_EVENT_BUTTON | JS_EVENT_INIT;
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: [PATCH] joydev: fix HZ->millisecond transformation

[Vojtech Pavlik]

On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
> Bjorn Helgaas wrote:
> > 
> > * fix a problem with HZ->millisecond transformation on
> >   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> > 
> > Applies to 2.4.20.
> > 
> > diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> > --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > @@ -50,6 +50,8 @@
> >  #define JOYDEV_MINORS          32
> >  #define JOYDEV_BUFFER_SIZE     64
> > 
> > +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
> Uh...                                                
> ^^^^^^^^^^^^^^^^
> by definition this is zero, is it not?

No, both parts of the equaition can be nonzero.

Though it might be easier to say (1000 * t) / HZ, now that I think about
it.

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] joydev: fix HZ->millisecond transformation

[Joe Korty]

[ repost -- I am going to have to get outgoing mail fixed on my lkml-reading
 machine ]

> On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
>> Bjorn Helgaas wrote:
>>> 
>>> * fix a problem with HZ->millisecond transformation on
>>>   non-x86 archs (from 2.5 change by vojtech@suse.cz)
>>> [...]
>>> +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
>>> [...]
> Though it might be easier to say (1000 * t) / HZ, now that I think about
> it.

The more complex version avoids integer overflow for more values of 't'
in the (1000 * t) subexpression.

Joe

Re: [PATCH] joydev: fix HZ->millisecond transformation

[george anzinger]

Vojtech Pavlik wrote:
> 
> On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
> > Bjorn Helgaas wrote:
> > >
> > > * fix a problem with HZ->millisecond transformation on
> > >   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> > >
> > > Applies to 2.4.20.
> > >
> > > diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> > > --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > @@ -50,6 +50,8 @@
> > >  #define JOYDEV_MINORS          32
> > >  #define JOYDEV_BUFFER_SIZE     64
> > >
> > > +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
> > Uh...
> > ^^^^^^^^^^^^^^^^
> > by definition this is zero, is it not?
> 
> No, both parts of the equaition can be nonzero.

I don't think so.  s%HZ has to be less than HZ.  Then
dividing that by HZ should result in zero.  Where is my
thinking flawed?
> 
> Though it might be easier to say (1000 * t) / HZ, now that I think about
> it.

That overflows...  As does the other if HZ is less than
1000....
> 
> --
> Vojtech Pavlik
> SuSE Labs
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: [PATCH] joydev: fix HZ->millisecond transformation

[Vojtech Pavlik]

On Fri, Dec 20, 2002 at 09:24:58AM -0800, george anzinger wrote:
> Vojtech Pavlik wrote:
> > 
> > On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
> > > Bjorn Helgaas wrote:
> > > >
> > > > * fix a problem with HZ->millisecond transformation on
> > > >   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> > > >
> > > > Applies to 2.4.20.
> > > >
> > > > diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> > > > --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > @@ -50,6 +50,8 @@
> > > >  #define JOYDEV_MINORS          32
> > > >  #define JOYDEV_BUFFER_SIZE     64
> > > >
> > > > +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
> > > Uh...
> > > ^^^^^^^^^^^^^^^^
> > > by definition this is zero, is it not?
> > 
> > No, both parts of the equaition can be nonzero.
> 
> I don't think so.  s%HZ has to be less than HZ.  Then
> dividing that by HZ should result in zero.  Where is my
> thinking flawed?

You first multiply it by 1000.

> > Though it might be easier to say (1000 * t) / HZ, now that I think about
> > it.
> 
> That overflows...  As does the other if HZ is less than 1000....

You're right, t can be all 32 bits.

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] joydev: fix HZ->millisecond transformation

[george anzinger]

Vojtech Pavlik wrote:
> 
> On Fri, Dec 20, 2002 at 09:24:58AM -0800, george anzinger wrote:
> > Vojtech Pavlik wrote:
> > >
> > > On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
> > > > Bjorn Helgaas wrote:
> > > > >
> > > > > * fix a problem with HZ->millisecond transformation on
> > > > >   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> > > > >
> > > > > Applies to 2.4.20.
> > > > >
> > > > > diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> > > > > --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > > +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > > @@ -50,6 +50,8 @@
> > > > >  #define JOYDEV_MINORS          32
> > > > >  #define JOYDEV_BUFFER_SIZE     64
> > > > >
> > > > > +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
> > > > Uh...
> > > > ^^^^^^^^^^^^^^^^
> > > > by definition this is zero, is it not?
> > >
> > > No, both parts of the equaition can be nonzero.
> >
> > I don't think so.  s%HZ has to be less than HZ.  Then
> > dividing that by HZ should result in zero.  Where is my
> > thinking flawed?
> 
> You first multiply it by 1000.

But then it should read: (1000 * (t)) % HZ) / HZ

But this is still zero.  There is no way that ((X % HZ) /
HZ) is other than zero, me thinks.

> 
> > > Though it might be easier to say (1000 * t) / HZ, now that I think about
> > > it.
> >
> > That overflows...  As does the other if HZ is less than 1000....
> 
> You're right, t can be all 32 bits.

What, exactly, is this code trying to do?  It looks like the
number is being passed to user space...
> 
> --
> Vojtech Pavlik
> SuSE Labs
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: [PATCH] joydev: fix HZ->millisecond transformation

[Vojtech Pavlik]

On Fri, Dec 20, 2002 at 10:16:27AM -0800, george anzinger wrote:
> Vojtech Pavlik wrote:
> > 
> > On Fri, Dec 20, 2002 at 09:24:58AM -0800, george anzinger wrote:
> > > Vojtech Pavlik wrote:
> > > >
> > > > On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
> > > > > Bjorn Helgaas wrote:
> > > > > >
> > > > > > * fix a problem with HZ->millisecond transformation on
> > > > > >   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> > > > > >
> > > > > > Applies to 2.4.20.
> > > > > >
> > > > > > diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> > > > > > --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > > > +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > > > @@ -50,6 +50,8 @@
> > > > > >  #define JOYDEV_MINORS          32
> > > > > >  #define JOYDEV_BUFFER_SIZE     64
> > > > > >
> > > > > > +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
> > > > > Uh...
> > > > > ^^^^^^^^^^^^^^^^
> > > > > by definition this is zero, is it not?
> > > >
> > > > No, both parts of the equaition can be nonzero.
> > >
> > > I don't think so.  s%HZ has to be less than HZ.  Then
> > > dividing that by HZ should result in zero.  Where is my
> > > thinking flawed?
> > 
> > You first multiply it by 1000.
> 
> But then it should read: (1000 * (t)) % HZ) / HZ

The expression is evaluated from left to right. No need for parentheses
here.

> But this is still zero.  There is no way that ((X % HZ) /
> HZ) is other than zero, me thinks.

Example:

HZ = 100
t = 70

1000 * (t % HZ) / HZ = 1000 * 70 / 100 = 700 != 0

Anyway (x % HZ) / HZ must be zero, but that's something different.

> > > > Though it might be easier to say (1000 * t) / HZ, now that I think about
> > > > it.
> > >
> > > That overflows...  As does the other if HZ is less than 1000....
> > 
> > You're right, t can be all 32 bits.
> 
> What, exactly, is this code trying to do?  It looks like the
> number is being passed to user space...

It's trying to convert jiffies to microseconds from computer start (or
any other arbitrary point in time), thus making the value independent of
HZ.

> -- 
> George Anzinger   george@mvista.com
> High-res-timers: 
> http://sourceforge.net/projects/high-res-timers/
> Preemption patch:
> http://www.kernel.org/pub/linux/kernel/people/rml

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] joydev: fix HZ->millisecond transformation

[george anzinger]

Vojtech Pavlik wrote:
> 
> On Fri, Dec 20, 2002 at 10:16:27AM -0800, george anzinger wrote:
> > Vojtech Pavlik wrote:
> > >
> > > On Fri, Dec 20, 2002 at 09:24:58AM -0800, george anzinger wrote:
> > > > Vojtech Pavlik wrote:
> > > > >
> > > > > On Fri, Dec 20, 2002 at 02:41:50AM -0800, george anzinger wrote:
> > > > > > Bjorn Helgaas wrote:
> > > > > > >
> > > > > > > * fix a problem with HZ->millisecond transformation on
> > > > > > >   non-x86 archs (from 2.5 change by vojtech@suse.cz)
> > > > > > >
> > > > > > > Applies to 2.4.20.
> > > > > > >
> > > > > > > diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
> > > > > > > --- a/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > > > > +++ b/drivers/input/joydev.c    Mon Dec 16 12:16:32 2002
> > > > > > > @@ -50,6 +50,8 @@
> > > > > > >  #define JOYDEV_MINORS          32
> > > > > > >  #define JOYDEV_BUFFER_SIZE     64
> > > > > > >
> > > > > > > +#define MSECS(t)       (1000 * ((t) / HZ) + 1000 * ((t) % HZ) / HZ)
> > > > > > Uh...
> > > > > > ^^^^^^^^^^^^^^^^
> > > > > > by definition this is zero, is it not?
> > > > >
> > > > > No, both parts of the equaition can be nonzero.
> > > >
> > > > I don't think so.  s%HZ has to be less than HZ.  Then
> > > > dividing that by HZ should result in zero.  Where is my
> > > > thinking flawed?
> > >
> > > You first multiply it by 1000.
> >
> > But then it should read: (1000 * (t)) % HZ) / HZ
> 
> The expression is evaluated from left to right. No need for parentheses
> here.
> 
> > But this is still zero.  There is no way that ((X % HZ) /
> > HZ) is other than zero, me thinks.
> 
> Example:
> 
> HZ = 100
> t = 70
> 
> 1000 * (t % HZ) / HZ = 1000 * 70 / 100 = 700 != 0
> 
> Anyway (x % HZ) / HZ must be zero, but that's something different.

OK, I miss read the "("s.  :(
> 
> > > > > Though it might be easier to say (1000 * t) / HZ, now that I think about
> > > > > it.
> > > >
> > > > That overflows...  As does the other if HZ is less than 1000....
> > >
> > > You're right, t can be all 32 bits.
> >
> > What, exactly, is this code trying to do?  It looks like the
> > number is being passed to user space...
> 
> It's trying to convert jiffies to microseconds from computer start (or
> any other arbitrary point in time), thus making the value independent of
> HZ.
> 
This is possible, but in general it can not be kept in one
32-bit word.  I don't know what it is used for or if the
format can be changed, but the struct timeval comes to mind.
-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml