| 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 |
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;
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
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
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
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 --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;