2002-12-16 19:19:44

by Bjorn Helgaas

[permalink] [raw]
Subject: [PATCH] joydev: fix HZ->millisecond transformation

* fix a problem with HZ->millisecond transformation on
non-x86 archs (from 2.5 change by [email protected])

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;


2002-12-20 10:34:14

by George Anzinger

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

Bjorn Helgaas wrote:
>
> * fix a problem with HZ->millisecond transformation on
> non-x86 archs (from 2.5 change by [email protected])
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
George Anzinger [email protected]
High-res-timers:
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

2002-12-20 13:19:54

by Vojtech Pavlik

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

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

2002-12-20 14:45:21

by jak

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

[ 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 [email protected])
>>> [...]
>>> +#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

2002-12-20 17:17:33

by George Anzinger

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

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 [email protected])
> > >
> > > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
George Anzinger [email protected]
High-res-timers:
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

2002-12-20 17:28:22

by Vojtech Pavlik

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

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

2002-12-20 18:09:02

by George Anzinger

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

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 [email protected])
> > > > >
> > > > > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
George Anzinger [email protected]
High-res-timers:
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

2002-12-21 09:11:13

by Vojtech Pavlik

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

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

2002-12-21 16:38:39

by George Anzinger

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

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 [email protected])
> > > > > > >
> > > > > > > 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 [email protected]
High-res-timers:
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml