diff mbox series

[1/8] staging: vchiq_arm: replace sleep() with usleep_range()

Message ID 20210914213532.396654-1-gascoar@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/8] staging: vchiq_arm: replace sleep() with usleep_range() | expand

Commit Message

Gaston Gonzalez Sept. 14, 2021, 9:35 p.m. UTC
usleep_range() should be used instead of sleep() when sleepings range
from 10 us to 20 ms, [1].

Reported by checkpatch.pl

[1] Documentation/timers/timers-howto.txt
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stefan Wahren Sept. 15, 2021, 5:21 a.m. UTC | #1
Hi,

Am 14.09.21 um 23:35 schrieb Gaston Gonzalez:
> usleep_range() should be used instead of sleep() when sleepings range
> from 10 us to 20 ms, [1].
>
> Reported by checkpatch.pl
>
> [1] Documentation/timers/timers-howto.txt
> ---
>  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 b25369a13452..0214ae37e01f 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -824,7 +824,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
>  		if (status != VCHIQ_RETRY)
>  			break;
>  
> -		msleep(1);
> +		usleep_range(1000, 1100);

from my understanding the usage of usleep_range() and hrtimers isn't
necessary here. The intention is to sleep a little bit and not "exactly"
1 ms.

@Phil Elwell: what is your opinion?

>  	}
>  
>  	return status;
> @@ -861,7 +861,7 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
>  		if (status != VCHIQ_RETRY)
>  			break;
>  
> -		msleep(1);
> +		usleep_range(1000, 1100);
dito
>  	}
>  
>  	return status;
Dan Carpenter Sept. 15, 2021, 7:29 a.m. UTC | #2
On Tue, Sep 14, 2021 at 06:35:26PM -0300, Gaston Gonzalez wrote:
> usleep_range() should be used instead of sleep() when sleepings range
> from 10 us to 20 ms, [1].
> 
> Reported by checkpatch.pl
> 
> [1] Documentation/timers/timers-howto.txt

For this particular warning, you should probably just ignore it, if you
can't test it...

You need a Signed-off-by.  Please run checkpatch.pl on your patches.

regards,
dan carpenter
Phil Elwell Sept. 15, 2021, 8:06 a.m. UTC | #3
Hi Stefan,

On 15/09/2021 06:21, Stefan Wahren wrote:
> Hi,
> 
> Am 14.09.21 um 23:35 schrieb Gaston Gonzalez:
>> usleep_range() should be used instead of sleep() when sleepings range
>> from 10 us to 20 ms, [1].
>>
>> Reported by checkpatch.pl
>>
>> [1] Documentation/timers/timers-howto.txt
>> ---
>>   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 b25369a13452..0214ae37e01f 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -824,7 +824,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
>>   		if (status != VCHIQ_RETRY)
>>   			break;
>>   
>> -		msleep(1);
>> +		usleep_range(1000, 1100);
> 
> from my understanding the usage of usleep_range() and hrtimers isn't
> necessary here. The intention is to sleep a little bit and not "exactly"
> 1 ms.
> 
> @Phil Elwell: what is your opinion?

Exactly - the aim is just to stop it spinning.

Phil
Arnd Bergmann Sept. 15, 2021, 2:19 p.m. UTC | #4
On Wed, Sep 15, 2021 at 10:06 AM Phil Elwell <phil@raspberrypi.com> wrote:
>
> Hi Stefan,
>
> On 15/09/2021 06:21, Stefan Wahren wrote:
> > Hi,
> >
> > Am 14.09.21 um 23:35 schrieb Gaston Gonzalez:
> >> usleep_range() should be used instead of sleep() when sleepings range
> >> from 10 us to 20 ms, [1].
> >>
> >> Reported by checkpatch.pl
> >>
> >> [1] Documentation/timers/timers-howto.txt
> >> ---
> >>   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 b25369a13452..0214ae37e01f 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> @@ -824,7 +824,7 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> >>              if (status != VCHIQ_RETRY)
> >>                      break;
> >>
> >> -            msleep(1);
> >> +            usleep_range(1000, 1100);
> >
> > from my understanding the usage of usleep_range() and hrtimers isn't
> > necessary here. The intention is to sleep a little bit and not "exactly"
> > 1 ms.
> >
> > @Phil Elwell: what is your opinion?
>
> Exactly - the aim is just to stop it spinning.

This is usually a sign for something else being wrong in the thing it's
waiting for. I can see multiple 'return VCHIQ_RETRY' statements in
vchiq_bulk_transfer(), however these all happen when the task
has received a signal and wait_for_completion_interruptible()
or mutex_lock_killable() has returned an error.

I don't see why one of them would return on any signal and the other
one only fatal signals, as you usually want those conditions to be the
same, but in either case, the loop is broken because as soon as
you get a signal, those interfaces will keep returning the same error
and you can never break out of the loop any more.

I don't know how to properly fix it, but it's clear that vchiq_bulk_transmit()
needs to propagate the -EINTR or -ERESTSTARTSYS back to user space
to let the calling task handle the signal before retrying.

        Arnd
Gaston Gonzalez Sept. 15, 2021, 8:09 p.m. UTC | #5
On Wed, Sep 15, 2021 at 10:29:04AM +0300, Dan Carpenter wrote:
> On Tue, Sep 14, 2021 at 06:35:26PM -0300, Gaston Gonzalez wrote:
> > usleep_range() should be used instead of sleep() when sleepings range
> > from 10 us to 20 ms, [1].
> > 
> > Reported by checkpatch.pl
> > 
> > [1] Documentation/timers/timers-howto.txt
> 
> For this particular warning, you should probably just ignore it, if you
> can't test it...
> 
> You need a Signed-off-by.  Please run checkpatch.pl on your patches.
>

Yes, my bad...

Will drop this one and resend the rest of the series.

Thanks,

Gaston

> regards,
> dan carpenter
>
diff mbox series

Patch

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 b25369a13452..0214ae37e01f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -824,7 +824,7 @@  vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
 		if (status != VCHIQ_RETRY)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 1100);
 	}
 
 	return status;
@@ -861,7 +861,7 @@  enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
 		if (status != VCHIQ_RETRY)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 1100);
 	}
 
 	return status;