[PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Kushal-kothari]

Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)
in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.

Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>
---

Changes from v1: Reword both the subject and the log message.

 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 6fbafdfe340f..80a7898c5331 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
 		if (status != VCHIQ_RETRY)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 2000);
 	}
 
 	return status;
@@ -894,7 +894,7 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
 		if (status != VCHIQ_RETRY)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 2000);
 	}
 
 	return status;
-- 
2.25.1

Re: [PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Greg KH]

On Sun, Oct 17, 2021 at 02:59:00PM +0530, Kushal-kothari wrote:
> Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
> msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)

No need to put the parameters in a function call.

> in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.
> 
> Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>

We need a "Legal" name here, is that how you sign documents?  Sorry, I
have to ask.

> ---
> 
> Changes from v1: Reword both the subject and the log message.
> 
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 6fbafdfe340f..80a7898c5331 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
>  		if (status != VCHIQ_RETRY)
>  			break;
>  
> -		msleep(1);
> +		usleep_range(1000, 2000);

You can not just randomly pick a range value here without lots of
testing and knowing that the hardware really can support this.

Have you done this here?

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Julia Lawall]

On Sun, 17 Oct 2021, Kushal-kothari wrote:

> Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
> msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)
> in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.

In addition to Greg's comments, the log message is still not really ideal.
There is still the word Fix that doesn't really contribute anything.  The
prototypes of msleep and usleep are also not useful - the reader either
knows them or can easily find them.  A better messge could be something
like:

"Checkpatch warns that an msleep of less than 20ms can sleep for up to
20ms.  Replace msleep(1) by a call to usleep_range that makes the possible
range (1ms - 20ms) explicit."

But the argument to msleep is in milliseconds.  If the appropriate first
argument to usleep_range is 1000, then it would seem that the second
argument should be 20000?

One thing that you can do to try to understand such a problem better is to
see what others have done before to resolve it.  You could try a command
like

git log -S msleep -p

to see commits that have changed the number of calls to msleep.

I reiterate Greg's comments that making these timing changes without being
able to test the result is risky.  These are just suggestions of some
methodologies that you could follow for a change where it is more clear
that the change is correct.

julia

>
> Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>
> ---
>
> Changes from v1: Reword both the subject and the log message.
>
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 6fbafdfe340f..80a7898c5331 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
>  		if (status != VCHIQ_RETRY)
>  			break;
>
> -		msleep(1);
> +		usleep_range(1000, 2000);
>  	}
>
>  	return status;
> @@ -894,7 +894,7 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
>  		if (status != VCHIQ_RETRY)
>  			break;
>
> -		msleep(1);
> +		usleep_range(1000, 2000);
>  	}
>
>  	return status;
> --
> 2.25.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 view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20211017092900.134752-1-kushalkothari285%40gmail.com.
>

Re: [PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Stefan Wahren]

Hi,

Am 17.10.21 um 11:29 schrieb Kushal-kothari:
> Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
> msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)
> in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.
>
> Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>
> ---
>
> Changes from v1: Reword both the subject and the log message.
>
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 6fbafdfe340f..80a7898c5331 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
>  		if (status != VCHIQ_RETRY)
>  			break;
>  
> -		msleep(1);
> +		usleep_range(1000, 2000);

there was a recent discussion about this checkpatch warning [1]

Best regards

[1]
https://lore.kernel.org/linux-staging/260b38b8-6f3f-f6cc-0388-09a269ead507@i2se.com/

Re: [PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Julia Lawall]

On Sun, 17 Oct 2021, Stefan Wahren wrote:

> Hi,
>
> Am 17.10.21 um 11:29 schrieb Kushal-kothari:
> > Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
> > msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)
> > in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.
> >
> > Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>
> > ---
> >
> > Changes from v1: Reword both the subject and the log message.
> >
> >  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 6fbafdfe340f..80a7898c5331 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> >  		if (status != VCHIQ_RETRY)
> >  			break;
> >
> > -		msleep(1);
> > +		usleep_range(1000, 2000);
>
> there was a recent discussion about this checkpatch warning [1]

Should there be a comment about it?

julia

>
> Best regards
>
> [1]
> https://lore.kernel.org/linux-staging/260b38b8-6f3f-f6cc-0388-09a269ead507@i2se.com/
>
>

Re: [PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Stefan Wahren]

Am 17.10.21 um 14:33 schrieb Julia Lawall:
>
> On Sun, 17 Oct 2021, Stefan Wahren wrote:
>
>> Hi,
>>
>> Am 17.10.21 um 11:29 schrieb Kushal-kothari:
>>> Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
>>> msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)
>>> in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.
>>>
>>> Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>
>>> ---
>>>
>>> Changes from v1: Reword both the subject and the log message.
>>>
>>>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> index 6fbafdfe340f..80a7898c5331 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> @@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
>>>  		if (status != VCHIQ_RETRY)
>>>  			break;
>>>
>>> -		msleep(1);
>>> +		usleep_range(1000, 2000);
>> there was a recent discussion about this checkpatch warning [1]
> Should there be a comment about it?

Of course

Best regards

>
> julia
>
>> Best regards
>>
>> [1]
>> https://lore.kernel.org/linux-staging/260b38b8-6f3f-f6cc-0388-09a269ead507@i2se.com/
>>
>>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] staging: vc04_services: replace msleep() by usleep_range()

[Dan Carpenter]

On Sun, Oct 17, 2021 at 02:33:57PM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 17 Oct 2021, Stefan Wahren wrote:
> 
> > Hi,
> >
> > Am 17.10.21 um 11:29 schrieb Kushal-kothari:
> > > Fixed the warning:-msleep < 20ms can sleep for up to 20ms by replacing
> > > msleep(unsigned long msecs) by usleep_range(unsigned long min, unsigned long max)
> > > in usecs as msleep(1ms~20ms) can sleep for upto 20 ms.
> > >
> > > Signed-off-by: Kushal-kothari <kushalkothari285@gmail.com>
> > > ---
> > >
> > > Changes from v1: Reword both the subject and the log message.
> > >
> > >  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > index 6fbafdfe340f..80a7898c5331 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -857,7 +857,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> > >  		if (status != VCHIQ_RETRY)
> > >  			break;
> > >
> > > -		msleep(1);
> > > +		usleep_range(1000, 2000);
> >
> > there was a recent discussion about this checkpatch warning [1]
> 
> Should there be a comment about it?

Even better would be an alternate wrapper around usleep(1) which would
silence the checkpatch warning.  Something like:

#define short_sleep() usleep(1)

regards,
dan carpenter