[PATCH] staging: rtl8712: usleep_range is preferred over udelay

[PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Amitoj Kaur Chawla]

Fix checkpatch.pl issue: "CHECK: usleep_range is preferred over udelay;
see Documentation/timers/timers-howto.txt".
Replace `udelay()` with a call to `usleep_range()` with a reasonable upper limit.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/rtl8712/hal_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
index 469c566..897dc77 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -237,7 +237,7 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
 		i = 5;
 		tmp16 = r8712_read16(padapter, TCR);
 		while (((tmp16 & _EMEM_CODE_DONE) == 0) && (i > 0)) {
-			udelay(10);
+			usleep_range(10, 1000);
 			tmp16 = r8712_read16(padapter, TCR);
 			i--;
 		}
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Julia Lawall]

On Tue, 6 Oct 2015, Amitoj Kaur Chawla wrote:

> Fix checkpatch.pl issue: "CHECK: usleep_range is preferred over udelay;
> see Documentation/timers/timers-howto.txt".
> Replace `udelay()` with a call to `usleep_range()` with a reasonable upper limit.

What is the definition of reasonable?  Do maybe other similar drivers use
these values?

julia

>
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/staging/rtl8712/hal_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> index 469c566..897dc77 100644
> --- a/drivers/staging/rtl8712/hal_init.c
> +++ b/drivers/staging/rtl8712/hal_init.c
> @@ -237,7 +237,7 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
>  		i = 5;
>  		tmp16 = r8712_read16(padapter, TCR);
>  		while (((tmp16 & _EMEM_CODE_DONE) == 0) && (i > 0)) {
> -			udelay(10);
> +			usleep_range(10, 1000);
>  			tmp16 = r8712_read16(padapter, TCR);
>  			i--;
>  		}
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20151005195012.GA21881%40amitoj-Inspiron-3542.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Amitoj Kaur Chawla]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

>> Fix checkpatch.pl issue: "CHECK: usleep_range is preferred over udelay;
>> see Documentation/timers/timers-howto.txt".
>> Replace `udelay()` with a call to `usleep_range()` with a reasonable
upper limit.
>
> What is the definition of reasonable?  Do maybe other similar drivers use
> these values?
>

I checked over here
<https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=120bdac7376a36418eb1d55e0161dc0e660a45c3>
 and here
<https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=3fe6929bbbca11b015d182d4864d92d0bf5c244d>.
They both used 1000. However, this
<https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=f98a20068dfcc16d1292d3e4d0d690e123da1538>
uses
20. But, the first two drivers seemed more similar to me. I couldn't find
anything in the documentation specifying the selection of range.

> julia
>
>>

[-- Attachment #2: Type: text/html, Size: 1160 bytes --]

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1127 bytes --]

> >> Fix checkpatch.pl issue: "CHECK: usleep_range is preferred over udelay;
> >> see Documentation/timers/timers-howto.txt".
> >> Replace `udelay()` with a call to `usleep_range()` with a reasonable
> upper limit.
> >
> > What is the definition of reasonable?  Do maybe other similar drivers use
> > these values?
> >
>
> I checked over here and here. They both used 1000. However, this uses 20.
> But, the first two drivers seemed more similar to me. I couldn't find
> anything in the documentation specifying the selection of range.

OK.

julia

> > julia
> >
> >>
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CA%2B5yK5Hh%3D0vgxfVb1Wz
> Xiy4w6z9GG9mJEHC21Bsvd9zdGzCJMw%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Arnd Bergmann]

On Tuesday 06 October 2015 08:12:58 Julia Lawall wrote:
> > >> Fix checkpatch.pl issue: "CHECK: usleep_range is preferred over udelay;
> > >> see Documentation/timers/timers-howto.txt".
> > >> Replace `udelay()` with a call to `usleep_range()` with a reasonable
> > upper limit.
> > >
> > > What is the definition of reasonable?  Do maybe other similar drivers use
> > > these values?
> > >
> >
> > I checked over here and here. They both used 1000. However, this uses 20.
> > But, the first two drivers seemed more similar to me. I couldn't find
> > anything in the documentation specifying the selection of range.
> 
> OK.

It really depends on what the driver is trying to do. In this case, it
waits for a device register to signal 'done', and it tries this five times
for a maximum of 50 microseconds, which is a relatively short period.

Changing the maximum to 1000 means that it can now sleep for up to
5 milliseconds instead, which is a noticeable time, especially if
it gets called frequently.

The questions to answer here are:

* Is it safe to call a sleeping function from this point at all?
  Look for other sleeping functions (proves that it is allowed to
  sleep) and for spinlocks (if a lock is held, you may not sleep)
  in this function.

* How often do you expect the function to get called? Once per data
  packet means it's not ok to sleep long, once an hour would mean it's
  clearly ok.

* Are there other calls in this function that take longer than a
  millisecond?


	Arnd

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Amitoj Kaur Chawla]

On Tue, Oct 6, 2015 at 1:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> It really depends on what the driver is trying to do. In this case, it
> waits for a device register to signal 'done', and it tries this five times
> for a maximum of 50 microseconds, which is a relatively short period.
>
> Changing the maximum to 1000 means that it can now sleep for up to
> 5 milliseconds instead, which is a noticeable time, especially if
> it gets called frequently.
>
> The questions to answer here are:
>
> * Is it safe to call a sleeping function from this point at all?
>   Look for other sleeping functions (proves that it is allowed to
>   sleep) and for spinlocks (if a lock is held, you may not sleep)
>   in this function.
>

There aren't any other sleeping functions in this function. However,
udelay() has been used one more time in a loop.

> * How often do you expect the function to get called? Once per data
>   packet means it's not ok to sleep long, once an hour would mean it's
>   clearly ok.
>
> * Are there other calls in this function that take longer than a
>   millisecond?

How can I find out these 2 things? Only udelay() is the sleeping function.

-- 


Amitoj

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Arnd Bergmann]

On Thursday 08 October 2015 08:23:10 Amitoj Kaur Chawla wrote:
> On Tue, Oct 6, 2015 at 1:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > It really depends on what the driver is trying to do. In this case, it
> > waits for a device register to signal 'done', and it tries this five times
> > for a maximum of 50 microseconds, which is a relatively short period.
> >
> > Changing the maximum to 1000 means that it can now sleep for up to
> > 5 milliseconds instead, which is a noticeable time, especially if
> > it gets called frequently.
> >
> > The questions to answer here are:
> >
> > * Is it safe to call a sleeping function from this point at all?
> >   Look for other sleeping functions (proves that it is allowed to
> >   sleep) and for spinlocks (if a lock is held, you may not sleep)
> >   in this function.
> >
> 
> There aren't any other sleeping functions in this function. However,
> udelay() has been used one more time in a loop.

udelay() is not a sleeping function, so that doesn't count here.

Try to look further out if you can't find a sleeping function or a spinlock
here, scanning the callers of this function and everything they call for
common sleeping functions like kmalloc(..., GFP_KERNEL), mutex_lock() or
msleep().

> > * How often do you expect the function to get called? Once per data
> >   packet means it's not ok to sleep long, once an hour would mean it's
> >   clearly ok.
> >
> > * Are there other calls in this function that take longer than a
> >   millisecond?
> 
> How can I find out these 2 things? Only udelay() is the sleeping function.

Try understanding what the functions actually do by looking at the call
chain for the first one, it should be fairly easy in this case.

For the second one, look for functions that get called from this one,
if any of them contains a mdelay() or msleep(), or a loop around
udelay(), try guessing how long that can take.

	Arnd

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Amitoj Kaur Chawla]

On Thu, Oct 8, 2015 at 1:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 08 October 2015 08:23:10 Amitoj Kaur Chawla wrote:
>> On Tue, Oct 6, 2015 at 1:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > It really depends on what the driver is trying to do. In this case, it
>> > waits for a device register to signal 'done', and it tries this five times
>> > for a maximum of 50 microseconds, which is a relatively short period.
>> >
>> > Changing the maximum to 1000 means that it can now sleep for up to
>> > 5 milliseconds instead, which is a noticeable time, especially if
>> > it gets called frequently.
>> >
>> > The questions to answer here are:
>> >
>> > * Is it safe to call a sleeping function from this point at all?
>> >   Look for other sleeping functions (proves that it is allowed to
>> >   sleep) and for spinlocks (if a lock is held, you may not sleep)
>> >   in this function.
>> >
>>
>> There aren't any other sleeping functions in this function. However,
>> udelay() has been used one more time in a loop.
>
> udelay() is not a sleeping function, so that doesn't count here.
>
> Try to look further out if you can't find a sleeping function or a spinlock
> here, scanning the callers of this function and everything they call for
> common sleeping functions like kmalloc(..., GFP_KERNEL), mutex_lock() or
> msleep().

kmalloc(..., GFP_ATOMIC) is being called in the function. Callers of
this function aren't calling these sleeping functions.

>
>> > * How often do you expect the function to get called? Once per data
>> >   packet means it's not ok to sleep long, once an hour would mean it's
>> >   clearly ok.
>> >
>> > * Are there other calls in this function that take longer than a
>> >   millisecond?
>>
>> How can I find out these 2 things? Only udelay() is the sleeping function.
>
> Try understanding what the functions actually do by looking at the call
> chain for the first one, it should be fairly easy in this case.
>
> For the second one, look for functions that get called from this one,
> if any of them contains a mdelay() or msleep(), or a loop around
> udelay(), try guessing how long that can take.

The function is calling msleep(20) and has a loop around it, repeating
hundred times so that means a couple of milliseconds delay, right? Is
usleep_range() only used for short delays and msleep() for the longer
ones?


Amitoj

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Arnd Bergmann]

On Saturday 10 October 2015 00:26:33 Amitoj Kaur Chawla wrote:
> On Thu, Oct 8, 2015 at 1:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 08 October 2015 08:23:10 Amitoj Kaur Chawla wrote:
> >> On Tue, Oct 6, 2015 at 1:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > It really depends on what the driver is trying to do. In this case, it
> >> > waits for a device register to signal 'done', and it tries this five times
> >> > for a maximum of 50 microseconds, which is a relatively short period.
> >> >
> >> > Changing the maximum to 1000 means that it can now sleep for up to
> >> > 5 milliseconds instead, which is a noticeable time, especially if
> >> > it gets called frequently.
> >> >
> >> > The questions to answer here are:
> >> >
> >> > * Is it safe to call a sleeping function from this point at all?
> >> >   Look for other sleeping functions (proves that it is allowed to
> >> >   sleep) and for spinlocks (if a lock is held, you may not sleep)
> >> >   in this function.
> >> >
> >>
> >> There aren't any other sleeping functions in this function. However,
> >> udelay() has been used one more time in a loop.
> >
> > udelay() is not a sleeping function, so that doesn't count here.
> >
> > Try to look further out if you can't find a sleeping function or a spinlock
> > here, scanning the callers of this function and everything they call for
> > common sleeping functions like kmalloc(..., GFP_KERNEL), mutex_lock() or
> > msleep().
> 
> kmalloc(..., GFP_ATOMIC) is being called in the function. Callers of
> this function aren't calling these sleeping functions.

Ah, interesting. Using kmalloc(..., GFP_ATOMIC) is usually a sign that the
function is called in atomic context and is not allowed to sleep. However
based on what you write below, I suspect that is actually a mistake
and it should use GFP_KERNEL instead, because GFP_ATOMIC allocations
are much more likely to fail.

> >> > * How often do you expect the function to get called? Once per data
> >> >   packet means it's not ok to sleep long, once an hour would mean it's
> >> >   clearly ok.
> >> >
> >> > * Are there other calls in this function that take longer than a
> >> >   millisecond?
> >>
> >> How can I find out these 2 things? Only udelay() is the sleeping function.
> >
> > Try understanding what the functions actually do by looking at the call
> > chain for the first one, it should be fairly easy in this case.
> >
> > For the second one, look for functions that get called from this one,
> > if any of them contains a mdelay() or msleep(), or a loop around
> > udelay(), try guessing how long that can take.
> 
> The function is calling msleep(20) and has a loop around it, repeating
> hundred times so that means a couple of milliseconds delay, right?

This means the function can sleep up to 2 seconds, which is basically
an eternity from a driver's perspective, so adding a few shorter sleep
periods is certainly not harmful.

> Is usleep_range() only used for short delays and msleep() for the longer
> ones?

Yes. There are other differences, but a main one is that msleep() can
not sleep less than a millisecond, while usleep_range() can sleep much
shorter periods.

	Arnd

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Amitoj Kaur Chawla]

On Sat, Oct 10, 2015 at 12:34 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 10 October 2015 00:26:33 Amitoj Kaur Chawla wrote:
>> On Thu, Oct 8, 2015 at 1:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Thursday 08 October 2015 08:23:10 Amitoj Kaur Chawla wrote:
>> >> On Tue, Oct 6, 2015 at 1:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > It really depends on what the driver is trying to do. In this case, it
>> >> > waits for a device register to signal 'done', and it tries this five times
>> >> > for a maximum of 50 microseconds, which is a relatively short period.
>> >> >
>> >> > Changing the maximum to 1000 means that it can now sleep for up to
>> >> > 5 milliseconds instead, which is a noticeable time, especially if
>> >> > it gets called frequently.
>> >> >
>> >> > The questions to answer here are:
>> >> >
>> >> > * Is it safe to call a sleeping function from this point at all?
>> >> >   Look for other sleeping functions (proves that it is allowed to
>> >> >   sleep) and for spinlocks (if a lock is held, you may not sleep)
>> >> >   in this function.
>> >> >
>> >>
>> >> There aren't any other sleeping functions in this function. However,
>> >> udelay() has been used one more time in a loop.
>> >
>> > udelay() is not a sleeping function, so that doesn't count here.
>> >
>> > Try to look further out if you can't find a sleeping function or a spinlock
>> > here, scanning the callers of this function and everything they call for
>> > common sleeping functions like kmalloc(..., GFP_KERNEL), mutex_lock() or
>> > msleep().
>>
>> kmalloc(..., GFP_ATOMIC) is being called in the function. Callers of
>> this function aren't calling these sleeping functions.
>
> Ah, interesting. Using kmalloc(..., GFP_ATOMIC) is usually a sign that the
> function is called in atomic context and is not allowed to sleep. However
> based on what you write below, I suspect that is actually a mistake
> and it should use GFP_KERNEL instead, because GFP_ATOMIC allocations
> are much more likely to fail.
>
>> >> > * How often do you expect the function to get called? Once per data
>> >> >   packet means it's not ok to sleep long, once an hour would mean it's
>> >> >   clearly ok.
>> >> >
>> >> > * Are there other calls in this function that take longer than a
>> >> >   millisecond?
>> >>
>> >> How can I find out these 2 things? Only udelay() is the sleeping function.
>> >
>> > Try understanding what the functions actually do by looking at the call
>> > chain for the first one, it should be fairly easy in this case.
>> >
>> > For the second one, look for functions that get called from this one,
>> > if any of them contains a mdelay() or msleep(), or a loop around
>> > udelay(), try guessing how long that can take.
>>
>> The function is calling msleep(20) and has a loop around it, repeating
>> hundred times so that means a couple of milliseconds delay, right?
>
> This means the function can sleep up to 2 seconds, which is basically
> an eternity from a driver's perspective, so adding a few shorter sleep
> periods is certainly not harmful.
>
>> Is usleep_range() only used for short delays and msleep() for the longer
>> ones?
>
> Yes. There are other differences, but a main one is that msleep() can
> not sleep less than a millisecond, while usleep_range() can sleep much
> shorter periods.
>

Conclusively, Shorter sleep periods means using usleep_range() instead
of msleep() in this case with a small range since the loop is
repeating a 100 times?


Amitoj

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Arnd Bergmann]

On Saturday 10 October 2015 00:41:37 Amitoj Kaur Chawla wrote:
> On Sat, Oct 10, 2015 at 12:34 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > This means the function can sleep up to 2 seconds, which is basically
> > an eternity from a driver's perspective, so adding a few shorter sleep
> > periods is certainly not harmful.
> >
> >> Is usleep_range() only used for short delays and msleep() for the longer
> >> ones?
> >
> > Yes. There are other differences, but a main one is that msleep() can
> > not sleep less than a millisecond, while usleep_range() can sleep much
> > shorter periods.
> >
> 
> Conclusively, Shorter sleep periods means using usleep_range() instead
> of msleep() in this case with a small range since the loop is
> repeating a 100 times?

That is a bit oversimplified. What we found so far is 

- This is a function that is allowed to sleep. We know that because
  it calls msleep(). This means your original patch is good, but you
  should explain this in the changelog.

- you found another bug in the same function, which is allowed to
  sleep but calls kmalloc(..., GFP_ATOMIC) when it should use GFP_KERNEL.

- usleep_range(10, 1000) is a huge range (a factor of 100). This is ok
  here, because even the time spent in a 1000 microsecond wait is dwarfed
  by the 100 * 20ms wait time later in the function.

Other points that we have not talked about are:

- using a smaller range would also be ok, typically one would use a factor
  between 1.1 (+10%) and 10 (+1000%), but not a factor of 100.

- the function actually has two copies of the same loop with udelay()
  in it. If you change one of them, it would be logical to change both
  in the same patch.

- the loop you found below is interesting as well. Waiting 100 times for
  20ms means we can actually wait even longer, as each 20ms interval can
  turn into 30 or 40 milliseconds. If we care about the actual maximum
  time, this could be improved by writing it as:

	unsigned long timeout = jiffies + 2 * HZ; /* up to two seconds */
	do {
		tmp16 = r8712_read16(padapter, TCR);
		if ((tmp16 & _DMEM_CODE_DONE)
			break;
		msleep(20);
	} while (time_before(timeout, jiffies));

  We probably don't care about that here though.

	Arnd

Re: [Outreachy kernel] [PATCH] staging: rtl8712: usleep_range is preferred over udelay

[Amitoj Kaur Chawla]

On Sat, Oct 10, 2015 at 1:09 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 10 October 2015 00:41:37 Amitoj Kaur Chawla wrote:
>> On Sat, Oct 10, 2015 at 12:34 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > This means the function can sleep up to 2 seconds, which is basically
>> > an eternity from a driver's perspective, so adding a few shorter sleep
>> > periods is certainly not harmful.
>> >
>> >> Is usleep_range() only used for short delays and msleep() for the longer
>> >> ones?
>> >
>> > Yes. There are other differences, but a main one is that msleep() can
>> > not sleep less than a millisecond, while usleep_range() can sleep much
>> > shorter periods.
>> >
>>
>> Conclusively, Shorter sleep periods means using usleep_range() instead
>> of msleep() in this case with a small range since the loop is
>> repeating a 100 times?
>
> That is a bit oversimplified. What we found so far is
>
> - This is a function that is allowed to sleep. We know that because
>   it calls msleep(). This means your original patch is good, but you
>   should explain this in the changelog.
>
> - you found another bug in the same function, which is allowed to
>   sleep but calls kmalloc(..., GFP_ATOMIC) when it should use GFP_KERNEL.
>

Yes, I'll send a separate patch for this bug.

> - usleep_range(10, 1000) is a huge range (a factor of 100). This is ok
>   here, because even the time spent in a 1000 microsecond wait is dwarfed
>   by the 100 * 20ms wait time later in the function.
>
> Other points that we have not talked about are:
>
> - using a smaller range would also be ok, typically one would use a factor
>   between 1.1 (+10%) and 10 (+1000%), but not a factor of 100.
>
> - the function actually has two copies of the same loop with udelay()
>   in it. If you change one of them, it would be logical to change both
>   in the same patch.
>

I'll send a v2 of this patch changing both udelay() functions.

> - the loop you found below is interesting as well. Waiting 100 times for
>   20ms means we can actually wait even longer, as each 20ms interval can
>   turn into 30 or 40 milliseconds. If we care about the actual maximum
>   time, this could be improved by writing it as:
>
>         unsigned long timeout = jiffies + 2 * HZ; /* up to two seconds */
>         do {
>                 tmp16 = r8712_read16(padapter, TCR);
>                 if ((tmp16 & _DMEM_CODE_DONE)
>                         break;
>                 msleep(20);
>         } while (time_before(timeout, jiffies));
>

This piece of code is actually easier to understand than the original.


>   We probably don't care about that here though.
>

Thanks for all the feedback and patience. Things are way clearer now.

-- 
Amitoj