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@xxxxxxxxx> wrote: > On Tue, Nov 29, 2016 at 12:29 PM, vojtech@xxxxxx <vojtech@xxxxxx> 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@xxxxxx" <vojtech@xxxxxx> 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@xxxxxxxxxxx> >>> > > --- >>> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
