* [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs [not found] <CGME20161128114356epcms5p836ef0c36a1cbb8b68638ed046afbb777@epcms5p8> @ 2016-11-28 11:43 ` Aniroop Mathur 2016-11-28 11:53 ` vojtech 2016-11-28 13:49 ` Aniroop Mathur 0 siblings, 2 replies; 6+ messages in thread From: Aniroop Mathur @ 2016-11-28 11:43 UTC (permalink / raw) To: dmitry.torokhov, vojtech, linux-input, linux-kernel Cc: SAMUEL SEQUEIRA, Rahul Mahale, aniroop.mathur [-- Attachment #1: Type: text/plain, Size: 1909 bytes --] msleep(1~20) may not do what the caller intends, and will often sleep longer. (~20 ms actual sleep for any value given in the 1~20ms range) This is not the desired behaviour for many cases like device resume time, device suspend time, device enable time, data reading time, etc. Thus, change msleep to usleep_range for precise wakeups. Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> --- joystick/adi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/joystick/adi.c b/joystick/adi.c index d09cefa..6799bd9 100644 --- a/joystick/adi.c +++ b/joystick/adi.c @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL"); #define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */ #define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */ -#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */ -#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */ +#define ADI_INIT_DELAY 10000 /* Delay after init packet [10ms] */ +#define ADI_DATA_DELAY 4000 /* Delay after data packet [4000us] */ #define ADI_MAX_LENGTH 256 #define ADI_MIN_LENGTH 8 @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport) for (i = 0; seq[i]; i++) { gameport_trigger(gameport); if (seq[i] > 0) - msleep(seq[i]); + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100); if (seq[i] < 0) { mdelay(-seq[i]); udelay(-seq[i]*14); /* It looks like mdelay() is off by approx 1.4% */ @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv) gameport_set_poll_handler(gameport, adi_poll); gameport_set_poll_interval(gameport, 20); - msleep(ADI_INIT_DELAY); + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100); if (adi_read(port)) { - msleep(ADI_DATA_DELAY); + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100); adi_read(port); } -- 2.6.4.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs 2016-11-28 11:43 ` [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs Aniroop Mathur @ 2016-11-28 11:53 ` vojtech 2016-11-28 13:49 ` Aniroop Mathur 1 sibling, 0 replies; 6+ messages in thread From: vojtech @ 2016-11-28 11:53 UTC (permalink / raw) To: Aniroop Mathur Cc: dmitry.torokhov, linux-input, linux-kernel, SAMUEL SEQUEIRA, Rahul Mahale, aniroop.mathur Hi. ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer sleep doesn't matter. In the initilization sequence - first chunk of your patch - a way too long delay could in theory make the device fail to initialize. What's critical is that the mdelay() calls are precise. One day I'll open my box of old joystick and re-test these drivers to see if they survived the years of kernel infrastructure updates ... Vojtech On Mon, Nov 28, 2016 at 11:43:56AM +0000, Aniroop Mathur wrote: > msleep(1~20) may not do what the caller intends, and will often sleep longer. > (~20 ms actual sleep for any value given in the 1~20ms range) > This is not the desired behaviour for many cases like device resume time, > device suspend time, device enable time, data reading time, etc. > Thus, change msleep to usleep_range for precise wakeups. > > Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> > --- > joystick/adi.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/joystick/adi.c b/joystick/adi.c > index d09cefa..6799bd9 100644 > --- a/joystick/adi.c > +++ b/joystick/adi.c > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL"); > > #define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */ > #define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */ > -#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */ > -#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */ > +#define ADI_INIT_DELAY 10000 /* Delay after init packet [10ms] */ > +#define ADI_DATA_DELAY 4000 /* Delay after data packet [4000us] */ > > #define ADI_MAX_LENGTH 256 > #define ADI_MIN_LENGTH 8 > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport) > for (i = 0; seq[i]; i++) { > gameport_trigger(gameport); > if (seq[i] > 0) > - msleep(seq[i]); > + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100); > if (seq[i] < 0) { > mdelay(-seq[i]); > udelay(-seq[i]*14); /* It looks like mdelay() is off by approx 1.4% */ > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv) > gameport_set_poll_handler(gameport, adi_poll); > gameport_set_poll_interval(gameport, 20); > > - msleep(ADI_INIT_DELAY); > + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100); > if (adi_read(port)) { > - msleep(ADI_DATA_DELAY); > + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100); > adi_read(port); > } > > -- > 2.6.4.windows.1 -- Vojtech Pavlik ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs 2016-11-28 11:43 ` [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs Aniroop Mathur 2016-11-28 11:53 ` vojtech @ 2016-11-28 13:49 ` Aniroop Mathur 2016-11-29 6:59 ` vojtech 1 sibling, 1 reply; 6+ messages in thread From: Aniroop Mathur @ 2016-11-28 13:49 UTC (permalink / raw) To: vojtech, dmitry.torokhov, linux-input, linux-kernel Cc: SAMUEL SEQUEIRA, Rahul Mahale, aniroop.mathur [-- Attachment #1: Type: text/plain, Size: 4955 bytes --] Hello Mr. Vojtech Pavlik, On 28 Nov 2016 17:23, "vojtech@ucw.cz" <vojtech@ucw.cz> wrote: > > Hi. > > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer > sleep doesn't matter. In the initilization sequence - first chunk of > your patch - a way too long delay could in theory make the device fail > to initialize. What's critical is that the mdelay() calls are precise. > > One day I'll open my box of old joystick and re-test these drivers to > see if they survived the years of kernel infrastructure updates ... > > Vojtech > Well, it seems to me that there is some kind of confusion, so I'd like to clarify things about this patch. As you have mentioned that in the initialization sequence, long delay could in theory make the device fail to initialize - This patch actually solves this problem. msleep is built on jiffies / legacy timers and usleep_range is built on top of hrtimers so the wakeup will be precise. Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt For example in initialization sequence, if we use msleep(4), then the process could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on machine architecture. Like on a machine with tick rate / HZ is defined to be 100 so msleep(4) will make the process to sleep for minimum 10 ms. Whereas usleep_range(4000, 4100) will make sure that the process do not sleep for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is not. Originally, I added you in this patch to request you if you could help to test this patch or provide contact points of individuals who could help to test this patch as we do not have the hardware available with us? Like this driver, we also need to test other joystick drivers as well like gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have similar problem. Original patch link - https://patchwork.kernel.org/patch/9446201/ As we do not have hardware available, so we decided to split patch into individual drivers and request to person who could support to test this patch I have also applied the same patch in our driver whose hardware is available with me and I found that wake up time became precise indeed and so I decided to apply the same fix in other input subsystem drivers as well. Thank you! BR, Aniroop Mathur > On Mon, Nov 28, 2016 at 11:43:56AM +0000, Aniroop Mathur wrote: > > msleep(1~20) may not do what the caller intends, and will often sleep longer. > > (~20 ms actual sleep for any value given in the 1~20ms range) > > This is not the desired behaviour for many cases like device resume time, > > device suspend time, device enable time, data reading time, etc. > > Thus, change msleep to usleep_range for precise wakeups. > > > > Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> > > --- > > joystick/adi.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/joystick/adi.c b/joystick/adi.c > > index d09cefa..6799bd9 100644 > > --- a/joystick/adi.c > > +++ b/joystick/adi.c > > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL"); > > > > #define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */ > > #define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */ > > -#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */ > > -#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */ > > +#define ADI_INIT_DELAY 10000 /* Delay after init packet [10ms] */ > > +#define ADI_DATA_DELAY 4000 /* Delay after data packet [4000us] */ > > > > #define ADI_MAX_LENGTH 256 > > #define ADI_MIN_LENGTH 8 > > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport) > > for (i = 0; seq[i]; i++) { > > gameport_trigger(gameport); > > if (seq[i] > 0) > > - msleep(seq[i]); > > + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100); > > if (seq[i] < 0) { > > mdelay(-seq[i]); > > udelay(-seq[i]*14); /* It looks like mdelay() is off by approx 1.4% */ > > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv) > > gameport_set_poll_handler(gameport, adi_poll); > > gameport_set_poll_interval(gameport, 20); > > > > - msleep(ADI_INIT_DELAY); > > + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100); > > if (adi_read(port)) { > > - msleep(ADI_DATA_DELAY); > > + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100); > > adi_read(port); > > } > > > > -- > > 2.6.4.windows.1 > > > -- > Vojtech Pavlik ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs 2016-11-28 13:49 ` Aniroop Mathur @ 2016-11-29 6:59 ` vojtech 2016-12-03 17:50 ` Aniroop Mathur 0 siblings, 1 reply; 6+ messages in thread From: vojtech @ 2016-11-29 6:59 UTC (permalink / raw) To: Aniroop Mathur Cc: dmitry.torokhov, linux-input, linux-kernel, SAMUEL SEQUEIRA, Rahul Mahale, aniroop.mathur On Mon, Nov 28, 2016 at 01:49:31PM +0000, Aniroop Mathur wrote: > Hello Mr. Vojtech Pavlik, > > On 28 Nov 2016 17:23, "vojtech@ucw.cz" <vojtech@ucw.cz> wrote: > > > > Hi. > > > > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer > > sleep doesn't matter. In the initilization sequence - first chunk of > > your patch - a way too long delay could in theory make the device fail > > to initialize. What's critical is that the mdelay() calls are precise. > > > > One day I'll open my box of old joystick and re-test these drivers to > > see if they survived the years of kernel infrastructure updates ... > > > > Vojtech > > > > Well, it seems to me that there is some kind of confusion, so I'd like to > clarify things about this patch. > As you have mentioned that in the initialization sequence, long delay could > in theory make the device fail to initialize - This patch actually solves > this problem. > msleep is built on jiffies / legacy timers and usleep_range is built on top > of hrtimers so the wakeup will be precise. > Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > > For example in initialization sequence, if we use msleep(4), then the process > could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on > machine architecture. Like on a machine with tick rate / HZ is defined to be > 100 so msleep(4) will make the process to sleep for minimum 10 ms. > Whereas usleep_range(4000, 4100) will make sure that the process do not sleep > for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is not. > > Originally, I added you in this patch to request you if you could help to > test this patch or provide contact points of individuals who could help > to test this patch as we do not have the hardware available with us? > Like this driver, we also need to test other joystick drivers as well like > gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have > similar problem. > Original patch link - https://patchwork.kernel.org/patch/9446201/ > As we do not have hardware available, so we decided to split patch into > individual drivers and request to person who could support to test this patch > > I have also applied the same patch in our driver whose hardware is available > with me and I found that wake up time became precise indeed and so I > decided to apply the same fix in other input subsystem drivers as well. I do understand what you're trying to achieve. Both ADI_DATA_DELAY and ADI_INIT_DELAY are specified as minimum delays. Waiting longer doesn't cause any trouble, so the patch doesn't need to change that. In the initialization sequence, it probably doesn't matter either whether we wait longer, hence the distinction between msleep() and mdelay() based on positive/negative numbers. The mdelay() needs to be exact and the msleep() can be longer. How much longer before it disturbs the init sequence I'm not sure, probably quite a bit. The driver was written a long time before hrtimers existed and as such it was written expecting that msleep() can take a longer time. So your patch is most likely not needed, but I should find an ADI device and see what happens if I make the sleeps in the init sequence much longer. It'd also be interesting to see if the mdelay()s could be replaced with hrtimer-based delays instead, as that would be nicer to the system - if they can be precise enough. Also, preemption and maybe interrupts should be disabled around the mdelays I suppose - that was not an issue when the drivers were written. Vojtech > > Thank you! > > BR, > Aniroop Mathur > > > On Mon, Nov 28, 2016 at 11:43:56AM +0000, Aniroop Mathur wrote: > > > msleep(1~20) may not do what the caller intends, and will often sleep longer. > > > (~20 ms actual sleep for any value given in the 1~20ms range) > > > This is not the desired behaviour for many cases like device resume time, > > > device suspend time, device enable time, data reading time, etc. > > > Thus, change msleep to usleep_range for precise wakeups. > > > > > > Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> > > > --- > > > joystick/adi.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/joystick/adi.c b/joystick/adi.c > > > index d09cefa..6799bd9 100644 > > > --- a/joystick/adi.c > > > +++ b/joystick/adi.c > > > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL"); > > > > > > #define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */ > > > #define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */ > > > -#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */ > > > -#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */ > > > +#define ADI_INIT_DELAY 10000 /* Delay after init packet [10ms] */ > > > +#define ADI_DATA_DELAY 4000 /* Delay after data packet [4000us] */ > > > > > > #define ADI_MAX_LENGTH 256 > > > #define ADI_MIN_LENGTH 8 > > > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport) > > > for (i = 0; seq[i]; i++) { > > > gameport_trigger(gameport); > > > if (seq[i] > 0) > > > - msleep(seq[i]); > > > + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100); > > > if (seq[i] < 0) { > > > mdelay(-seq[i]); > > > udelay(-seq[i]*14); /* It looks like mdelay() is off by approx 1.4% */ > > > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv) > > > gameport_set_poll_handler(gameport, adi_poll); > > > gameport_set_poll_interval(gameport, 20); > > > > > > - msleep(ADI_INIT_DELAY); > > > + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100); > > > if (adi_read(port)) { > > > - msleep(ADI_DATA_DELAY); > > > + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100); > > > adi_read(port); > > > } > > > > > > -- > > > 2.6.4.windows.1 > > > > > > -- > > Vojtech Pavlik -- Vojtech Pavlik ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs 2016-11-29 6:59 ` vojtech @ 2016-12-03 17:50 ` Aniroop Mathur 2017-01-19 19:34 ` Aniroop Mathur 0 siblings, 1 reply; 6+ messages in thread From: Aniroop Mathur @ 2016-12-03 17:50 UTC (permalink / raw) To: vojtech Cc: Aniroop Mathur, dmitry.torokhov, linux-input, linux-kernel, SAMUEL SEQUEIRA, Rahul Mahale On Tue, Nov 29, 2016 at 12:29 PM, vojtech@ucw.cz <vojtech@ucw.cz> wrote: > On Mon, Nov 28, 2016 at 01:49:31PM +0000, Aniroop Mathur wrote: >> Hello Mr. Vojtech Pavlik, >> >> On 28 Nov 2016 17:23, "vojtech@ucw.cz" <vojtech@ucw.cz> wrote: >> > >> > Hi. >> > >> > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer >> > sleep doesn't matter. In the initilization sequence - first chunk of >> > your patch - a way too long delay could in theory make the device fail >> > to initialize. What's critical is that the mdelay() calls are precise. >> > >> > One day I'll open my box of old joystick and re-test these drivers to >> > see if they survived the years of kernel infrastructure updates ... >> > >> > Vojtech >> > >> >> Well, it seems to me that there is some kind of confusion, so I'd like to >> clarify things about this patch. >> As you have mentioned that in the initialization sequence, long delay could >> in theory make the device fail to initialize - This patch actually solves >> this problem. >> msleep is built on jiffies / legacy timers and usleep_range is built on top >> of hrtimers so the wakeup will be precise. >> Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt >> >> For example in initialization sequence, if we use msleep(4), then the process >> could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on >> machine architecture. Like on a machine with tick rate / HZ is defined to be >> 100 so msleep(4) will make the process to sleep for minimum 10 ms. >> Whereas usleep_range(4000, 4100) will make sure that the process do not sleep >> for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is not. >> >> Originally, I added you in this patch to request you if you could help to >> test this patch or provide contact points of individuals who could help >> to test this patch as we do not have the hardware available with us? >> Like this driver, we also need to test other joystick drivers as well like >> gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have >> similar problem. >> Original patch link - https://patchwork.kernel.org/patch/9446201/ >> As we do not have hardware available, so we decided to split patch into >> individual drivers and request to person who could support to test this patch >> >> I have also applied the same patch in our driver whose hardware is available >> with me and I found that wake up time became precise indeed and so I >> decided to apply the same fix in other input subsystem drivers as well. > > I do understand what you're trying to achieve. Both ADI_DATA_DELAY and > ADI_INIT_DELAY are specified as minimum delays. Waiting longer doesn't > cause any trouble, so the patch doesn't need to change that. > > In the initialization sequence, it probably doesn't matter either > whether we wait longer, hence the distinction between msleep() and > mdelay() based on positive/negative numbers. The mdelay() needs to be > exact and the msleep() can be longer. How much longer before it disturbs > the init sequence I'm not sure, probably quite a bit. > > The driver was written a long time before hrtimers existed and as such > it was written expecting that msleep() can take a longer time. > Well I agree that waiting longer shall not cause any trouble. However, using usleep_range does not cause any harm here either. In fact, usleep_range is more advantageous to use here as it makes the wake up more precise than before. So we have little improved connection / initialization time than before which is a good thing indeed as it improves response time. Also, same is being used by new device drivers and now some old device drivers have also changed to ulseep_range for 1- 10 ms range. Also, it is recommended and mentioned in the kernel documentation to use usleep_range for 1-10 ms delays. Explained originally here to why not use msleep for 1-20 ms: http://lkml.org/lkml/2007/8/3/250 So how about using usleep_range api for short delays here? > So your patch is most likely not needed, but I should find an ADI device > and see what happens if I make the sleeps in the init sequence much > longer. > So has it been possible so far to check behavior on large sleeps? > It'd also be interesting to see if the mdelay()s could be replaced with > hrtimer-based delays instead, as that would be nicer to the system - if > they can be precise enough. Also, preemption and maybe interrupts should > be disabled around the mdelays I suppose - that was not an issue when > the drivers were written. > Absolutely. I was also submitting the next patch to change mdelay to usleep_range for appropriate delays because mdelay should be ideally used in atomic context and not in non-atomic context because of busy-waiting. In our device drivers, we did change mdelay to usleep_range and we found it to be working great. Thanks. BR, Aniroop Mathur > Vojtech > >> >> Thank you! >> >> BR, >> Aniroop Mathur >> >> > On Mon, Nov 28, 2016 at 11:43:56AM +0000, Aniroop Mathur wrote: >> > > msleep(1~20) may not do what the caller intends, and will often sleep longer. >> > > (~20 ms actual sleep for any value given in the 1~20ms range) >> > > This is not the desired behaviour for many cases like device resume time, >> > > device suspend time, device enable time, data reading time, etc. >> > > Thus, change msleep to usleep_range for precise wakeups. >> > > >> > > Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> >> > > --- >> > > joystick/adi.c | 10 +++++----- >> > > 1 file changed, 5 insertions(+), 5 deletions(-) >> > > >> > > diff --git a/joystick/adi.c b/joystick/adi.c >> > > index d09cefa..6799bd9 100644 >> > > --- a/joystick/adi.c >> > > +++ b/joystick/adi.c >> > > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL"); >> > > >> > > #define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */ >> > > #define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */ >> > > -#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */ >> > > -#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */ >> > > +#define ADI_INIT_DELAY 10000 /* Delay after init packet [10ms] */ >> > > +#define ADI_DATA_DELAY 4000 /* Delay after data packet [4000us] */ >> > > >> > > #define ADI_MAX_LENGTH 256 >> > > #define ADI_MIN_LENGTH 8 >> > > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport) >> > > for (i = 0; seq[i]; i++) { >> > > gameport_trigger(gameport); >> > > if (seq[i] > 0) >> > > - msleep(seq[i]); >> > > + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100); >> > > if (seq[i] < 0) { >> > > mdelay(-seq[i]); >> > > udelay(-seq[i]*14); /* It looks like mdelay() is off by approx 1.4% */ >> > > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv) >> > > gameport_set_poll_handler(gameport, adi_poll); >> > > gameport_set_poll_interval(gameport, 20); >> > > >> > > - msleep(ADI_INIT_DELAY); >> > > + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100); >> > > if (adi_read(port)) { >> > > - msleep(ADI_DATA_DELAY); >> > > + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100); >> > > adi_read(port); >> > > } >> > > >> > > -- >> > > 2.6.4.windows.1 >> > >> > >> > -- >> > Vojtech Pavlik > > > -- > Vojtech Pavlik ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs 2016-12-03 17:50 ` Aniroop Mathur @ 2017-01-19 19:34 ` Aniroop Mathur 0 siblings, 0 replies; 6+ messages in thread From: Aniroop Mathur @ 2017-01-19 19:34 UTC (permalink / raw) To: vojtech Cc: Aniroop Mathur, dmitry.torokhov, linux-input, linux-kernel, SAMUEL SEQUEIRA, Rahul Mahale Hello Mr. Vojtech Pavlik, So then is the the behavior being checked on large sleeps as you mentioned? Along with this, like you said that replacing mdelay with usleep_range would be even more interesting so if you would like a patch for that as well to check the behavior then that could be sent to you as well. Thank you. -- Aniroop Mathur On Sat, Dec 3, 2016 at 11:20 PM, Aniroop Mathur <aniroop.mathur@gmail.com> wrote: > On Tue, Nov 29, 2016 at 12:29 PM, vojtech@ucw.cz <vojtech@ucw.cz> wrote: >> On Mon, Nov 28, 2016 at 01:49:31PM +0000, Aniroop Mathur wrote: >>> Hello Mr. Vojtech Pavlik, >>> >>> On 28 Nov 2016 17:23, "vojtech@ucw.cz" <vojtech@ucw.cz> wrote: >>> > >>> > Hi. >>> > >>> > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer >>> > sleep doesn't matter. In the initilization sequence - first chunk of >>> > your patch - a way too long delay could in theory make the device fail >>> > to initialize. What's critical is that the mdelay() calls are precise. >>> > >>> > One day I'll open my box of old joystick and re-test these drivers to >>> > see if they survived the years of kernel infrastructure updates ... >>> > >>> > Vojtech >>> > >>> >>> Well, it seems to me that there is some kind of confusion, so I'd like to >>> clarify things about this patch. >>> As you have mentioned that in the initialization sequence, long delay could >>> in theory make the device fail to initialize - This patch actually solves >>> this problem. >>> msleep is built on jiffies / legacy timers and usleep_range is built on top >>> of hrtimers so the wakeup will be precise. >>> Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt >>> >>> For example in initialization sequence, if we use msleep(4), then the process >>> could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on >>> machine architecture. Like on a machine with tick rate / HZ is defined to be >>> 100 so msleep(4) will make the process to sleep for minimum 10 ms. >>> Whereas usleep_range(4000, 4100) will make sure that the process do not sleep >>> for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is not. >>> >>> Originally, I added you in this patch to request you if you could help to >>> test this patch or provide contact points of individuals who could help >>> to test this patch as we do not have the hardware available with us? >>> Like this driver, we also need to test other joystick drivers as well like >>> gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have >>> similar problem. >>> Original patch link - https://patchwork.kernel.org/patch/9446201/ >>> As we do not have hardware available, so we decided to split patch into >>> individual drivers and request to person who could support to test this patch >>> >>> I have also applied the same patch in our driver whose hardware is available >>> with me and I found that wake up time became precise indeed and so I >>> decided to apply the same fix in other input subsystem drivers as well. >> >> I do understand what you're trying to achieve. Both ADI_DATA_DELAY and >> ADI_INIT_DELAY are specified as minimum delays. Waiting longer doesn't >> cause any trouble, so the patch doesn't need to change that. >> >> In the initialization sequence, it probably doesn't matter either >> whether we wait longer, hence the distinction between msleep() and >> mdelay() based on positive/negative numbers. The mdelay() needs to be >> exact and the msleep() can be longer. How much longer before it disturbs >> the init sequence I'm not sure, probably quite a bit. >> >> The driver was written a long time before hrtimers existed and as such >> it was written expecting that msleep() can take a longer time. >> > > Well I agree that waiting longer shall not cause any trouble. However, using > usleep_range does not cause any harm here either. In fact, usleep_range is > more advantageous to use here as it makes the wake up more precise than > before. So we have little improved connection / initialization time than > before which is a good thing indeed as it improves response time. > Also, same is being used by new device drivers and now some old device > drivers have also changed to ulseep_range for 1- 10 ms range. > Also, it is recommended and mentioned in the kernel documentation to use > usleep_range for 1-10 ms delays. > Explained originally here to why not use msleep for 1-20 ms: > http://lkml.org/lkml/2007/8/3/250 > > So how about using usleep_range api for short delays here? > > >> So your patch is most likely not needed, but I should find an ADI device >> and see what happens if I make the sleeps in the init sequence much >> longer. >> > > So has it been possible so far to check behavior on large sleeps? > >> It'd also be interesting to see if the mdelay()s could be replaced with >> hrtimer-based delays instead, as that would be nicer to the system - if >> they can be precise enough. Also, preemption and maybe interrupts should >> be disabled around the mdelays I suppose - that was not an issue when >> the drivers were written. >> > > Absolutely. I was also submitting the next patch to change mdelay to > usleep_range > for appropriate delays because mdelay should be ideally used in atomic context > and not in non-atomic context because of busy-waiting. > In our device drivers, we did change mdelay to usleep_range and we > found it to be > working great. > > Thanks. > > BR, > Aniroop Mathur > > >> Vojtech >> >>> >>> Thank you! >>> >>> BR, >>> Aniroop Mathur >>> >>> > On Mon, Nov 28, 2016 at 11:43:56AM +0000, Aniroop Mathur wrote: >>> > > msleep(1~20) may not do what the caller intends, and will often sleep longer. >>> > > (~20 ms actual sleep for any value given in the 1~20ms range) >>> > > This is not the desired behaviour for many cases like device resume time, >>> > > device suspend time, device enable time, data reading time, etc. >>> > > Thus, change msleep to usleep_range for precise wakeups. >>> > > >>> > > Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> >>> > > --- >>> > > joystick/adi.c | 10 +++++----- >>> > > 1 file changed, 5 insertions(+), 5 deletions(-) >>> > > >>> > > diff --git a/joystick/adi.c b/joystick/adi.c >>> > > index d09cefa..6799bd9 100644 >>> > > --- a/joystick/adi.c >>> > > +++ b/joystick/adi.c >>> > > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL"); >>> > > >>> > > #define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */ >>> > > #define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */ >>> > > -#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */ >>> > > -#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */ >>> > > +#define ADI_INIT_DELAY 10000 /* Delay after init packet [10ms] */ >>> > > +#define ADI_DATA_DELAY 4000 /* Delay after data packet [4000us] */ >>> > > >>> > > #define ADI_MAX_LENGTH 256 >>> > > #define ADI_MIN_LENGTH 8 >>> > > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport) >>> > > for (i = 0; seq[i]; i++) { >>> > > gameport_trigger(gameport); >>> > > if (seq[i] > 0) >>> > > - msleep(seq[i]); >>> > > + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100); >>> > > if (seq[i] < 0) { >>> > > mdelay(-seq[i]); >>> > > udelay(-seq[i]*14); /* It looks like mdelay() is off by approx 1.4% */ >>> > > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv) >>> > > gameport_set_poll_handler(gameport, adi_poll); >>> > > gameport_set_poll_interval(gameport, 20); >>> > > >>> > > - msleep(ADI_INIT_DELAY); >>> > > + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100); >>> > > if (adi_read(port)) { >>> > > - msleep(ADI_DATA_DELAY); >>> > > + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100); >>> > > adi_read(port); >>> > > } >>> > > >>> > > -- >>> > > 2.6.4.windows.1 >>> > >>> > >>> > -- >>> > Vojtech Pavlik >> >> >> -- >> Vojtech Pavlik ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-19 19:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20161128114356epcms5p836ef0c36a1cbb8b68638ed046afbb777@epcms5p8>
2016-11-28 11:43 ` [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs Aniroop Mathur
2016-11-28 11:53 ` vojtech
2016-11-28 13:49 ` Aniroop Mathur
2016-11-29 6:59 ` vojtech
2016-12-03 17:50 ` Aniroop Mathur
2017-01-19 19:34 ` Aniroop Mathur
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.