Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs

[Aniroop Mathur]

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