Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs
From: Aniroop Mathur
Date: Thu Jan 19 2017 - 14:36:09 EST
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