[PATCH] staging: greybus: Use fsleep() for delay

[PATCH] staging: greybus: Use fsleep() for delay

[pavankumaryalagada]

From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>

In the loopback function, usleep_range() is used for short delays
and msleep() for longer delays. This requires choosing the delay
function based on the requested time and converting microseconds
to milliseconds when using msleep().

Replace both functions with fsleep(), which selects a suitable
delay method based on the requested time. This avoids the need
to choose the delay function and convert the time manually.

Suggested-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
---
 drivers/staging/greybus/loopback.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index efc69c10cffe..2fe845eaef29 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -898,12 +898,8 @@ static int gb_loopback_fn(void *data)
 		gb->send_count++;
 		mutex_unlock(&gb->mutex);
 
-		if (us_wait) {
-			if (us_wait < 20000)
-				usleep_range(us_wait, us_wait + 100);
-			else
-				msleep(us_wait / 1000);
-		}
+		if (us_wait)
+			fsleep(us_wait);
 	}
 
 	gb_pm_runtime_put_autosuspend(bundle);
-- 
2.43.0

Re: [PATCH] staging: greybus: Use fsleep() for delay

[Johan Hovold]

On Thu, Oct 22, 2026 at 04:32:38PM +0530, pavankumaryalagada@gmail.com wrote:
> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> 
> In the loopback function, usleep_range() is used for short delays
> and msleep() for longer delays. This requires choosing the delay
> function based on the requested time and converting microseconds
> to milliseconds when using msleep().

Please include the module in Subject since this is not a subsystem wide
change:

	staging: greybus: loopback: ...
 
> Replace both functions with fsleep(), which selects a suitable
> delay method based on the requested time. This avoids the need
> to choose the delay function and convert the time manually.

It probably doesn't matter here, but I think you should mentioned that
fsleep() will likely make short sleeps longer (25% instead of 100 us
slack).

Johan

Re: [PATCH] staging: greybus: Use fsleep() for delay

[Alex Elder]

On 10/22/26 6:02 AM, pavankumaryalagada@gmail.com wrote:
> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> 
> In the loopback function, usleep_range() is used for short delays
> and msleep() for longer delays. This requires choosing the delay
> function based on the requested time and converting microseconds
> to milliseconds when using msleep().
> 
> Replace both functions with fsleep(), which selects a suitable
> delay method based on the requested time. This avoids the need
> to choose the delay function and convert the time manually.
> 
> Suggested-by: Alex Elder <elder@riscstar.com>
> Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>

Your description above is pretty good, but I agree with
Johan's comment.  In fact, you should think about and
understand exactly how that behavior changes with the
switch to using fsleep().
- When would the delay be different from before?
- When it differs--by how much does it differ?
- Is this amount significant?
And the third question gets at exactly what Johan was
saying should be included in the patch description.

We want to know not just the mechanics of the change,
but at a higher level, what is its effect?

You don't need to provide a super high level of detail
in this particular change, but you should demonstrate
somehow that you've thought about these things.

Please submit a new version of the patch and reword
the description a bit to concisely capture why it
makes sense to make the change, and why you believe
that the change in behavior is likely harmless.

Thanks.

					-Alex

> ---
>   drivers/staging/greybus/loopback.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index efc69c10cffe..2fe845eaef29 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -898,12 +898,8 @@ static int gb_loopback_fn(void *data)
>   		gb->send_count++;
>   		mutex_unlock(&gb->mutex);
>   
> -		if (us_wait) {
> -			if (us_wait < 20000)
> -				usleep_range(us_wait, us_wait + 100);
> -			else
> -				msleep(us_wait / 1000);
> -		}
> +		if (us_wait)
> +			fsleep(us_wait);
>   	}
>   
>   	gb_pm_runtime_put_autosuspend(bundle);

Re: [PATCH] staging: greybus: Use fsleep() for delay

[Yalagada Pavan Kumar]

On Thu, Oct 22, 2026 at 08:28:23AM -0500, Alex Elder wrote:
> On 10/22/26 6:02 AM, pavankumaryalagada@gmail.com wrote:
> > From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > 
> > In the loopback function, usleep_range() is used for short delays
> > and msleep() for longer delays. This requires choosing the delay
> > function based on the requested time and converting microseconds
> > to milliseconds when using msleep().
> > 
> > Replace both functions with fsleep(), which selects a suitable
> > delay method based on the requested time. This avoids the need
> > to choose the delay function and convert the time manually.
> > 
> > Suggested-by: Alex Elder <elder@riscstar.com>
> > Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> 
> Your description above is pretty good, but I agree with
> Johan's comment.  In fact, you should think about and
> understand exactly how that behavior changes with the
> switch to using fsleep().
> - When would the delay be different from before?
> - When it differs--by how much does it differ?

The current code uses usleep_range() for delays below 20
milliseconds and msleep() for longer delays. When using
msleep(), it converts microseconds to milliseconds by
rounding down, which can make the delay shorter than
requested.

For example, a requested delay of 20,500 microseconds
becomes msleep(20), which requests a 20 millisecond
sleep.

With fsleep(), the kernel selects the delay method
based on the requested time. For a 1000 microsecond
delay, the current code allows 100 microseconds of
extra time, while fsleep() allows up to 250 microseconds.

> - Is this amount significant?
> And the third question gets at exactly what Johan was
> saying should be included in the patch description.
> 
> We want to know not just the mechanics of the change,
> but at a higher level, what is its effect?

The change may affect the time between loopback operations
and the reported performance. The code does not seem to
need an exact delay, so i think using fsleep() is reasonable.
However, testing is needed to confirm its actual effect.

I do not have Greybus hardware to test the driver at runtime,
so my assessment is based on reviewing the code and fsleep()
implementation.

If possible, could you please check the runtime behavior or
and let me know if you find any issues with this change?

Thanks,
Pavan

> 
> You don't need to provide a super high level of detail
> in this particular change, but you should demonstrate
> somehow that you've thought about these things.
> 
> Please submit a new version of the patch and reword
> the description a bit to concisely capture why it
> makes sense to make the change, and why you believe
> that the change in behavior is likely harmless.
> 
> Thanks.
> 
> 					-Alex
> 
> > ---
> >   drivers/staging/greybus/loopback.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index efc69c10cffe..2fe845eaef29 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -898,12 +898,8 @@ static int gb_loopback_fn(void *data)
> >   		gb->send_count++;
> >   		mutex_unlock(&gb->mutex);
> > -		if (us_wait) {
> > -			if (us_wait < 20000)
> > -				usleep_range(us_wait, us_wait + 100);
> > -			else
> > -				msleep(us_wait / 1000);
> > -		}
> > +		if (us_wait)
> > +			fsleep(us_wait);
> >   	}
> >   	gb_pm_runtime_put_autosuspend(bundle);
> 

Re: [PATCH] staging: greybus: Use fsleep() for delay

[Alex Elder]

On 10/22/26 2:14 PM, Yalagada Pavan Kumar wrote:
> On Thu, Oct 22, 2026 at 08:28:23AM -0500, Alex Elder wrote:
>> On 10/22/26 6:02 AM, pavankumaryalagada@gmail.com wrote:
>>> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>>
>>> In the loopback function, usleep_range() is used for short delays
>>> and msleep() for longer delays. This requires choosing the delay
>>> function based on the requested time and converting microseconds
>>> to milliseconds when using msleep().
>>>
>>> Replace both functions with fsleep(), which selects a suitable
>>> delay method based on the requested time. This avoids the need
>>> to choose the delay function and convert the time manually.
>>>
>>> Suggested-by: Alex Elder <elder@riscstar.com>
>>> Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>
>> Your description above is pretty good, but I agree with
>> Johan's comment.  In fact, you should think about and
>> understand exactly how that behavior changes with the
>> switch to using fsleep().
>> - When would the delay be different from before?
>> - When it differs--by how much does it differ?

FYI, I was not really looking for a response here
(other than you sending a new version of the patch
that provides some additional information).  I was
simply suggesting the way you should approach
making a change like this.

> The current code uses usleep_range() for delays below 20
> milliseconds and msleep() for longer delays. When using
> msleep(), it converts microseconds to milliseconds by
> rounding down, which can make the delay shorter than
> requested.
> 
> For example, a requested delay of 20,500 microseconds
> becomes msleep(20), which requests a 20 millisecond
> sleep.
> 
> With fsleep(), the kernel selects the delay method
> based on the requested time. For a 1000 microsecond
> delay, the current code allows 100 microseconds of
> extra time, while fsleep() allows up to 250 microseconds.

Well it depends on the value of HZ, but I think it's
typically 1000.

>> - Is this amount significant?
>> And the third question gets at exactly what Johan was
>> saying should be included in the patch description.
>>
>> We want to know not just the mechanics of the change,
>> but at a higher level, what is its effect?
> 
> The change may affect the time between loopback operations
> and the reported performance. The code does not seem to
> need an exact delay, so i think using fsleep() is reasonable.
> However, testing is needed to confirm its actual effect.

This is technically right, but you have already shown
a little intuition about what the change does.  And
my point was that you should approach making changes
to the code with this kind of analysis in mind.

Simple fixups like this are great, but ideally they
should produce *zero* change in behavior.  In this
particular case, there *is* a change, and you should
make sure you understand it, and as Johan suggested,
make a comment on it.

> I do not have Greybus hardware to test the driver at runtime,
> so my assessment is based on reviewing the code and fsleep()
> implementation.
> 
> If possible, could you please check the runtime behavior or
> and let me know if you find any issues with this change?

I am not set up to do any testing like this either.
I don't think you strictly need to test this (other
than doing compile testing and things like checkpatch).
I'm just trying to suggest good practice for you to
follow when proposing changes.

					-Alex>
> Thanks,
> Pavan
> 
>>
>> You don't need to provide a super high level of detail
>> in this particular change, but you should demonstrate
>> somehow that you've thought about these things.
>>
>> Please submit a new version of the patch and reword
>> the description a bit to concisely capture why it
>> makes sense to make the change, and why you believe
>> that the change in behavior is likely harmless.
>>
>> Thanks.
>>
>> 					-Alex
>>
>>> ---
>>>    drivers/staging/greybus/loopback.c | 8 ++------
>>>    1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
>>> index efc69c10cffe..2fe845eaef29 100644
>>> --- a/drivers/staging/greybus/loopback.c
>>> +++ b/drivers/staging/greybus/loopback.c
>>> @@ -898,12 +898,8 @@ static int gb_loopback_fn(void *data)
>>>    		gb->send_count++;
>>>    		mutex_unlock(&gb->mutex);
>>> -		if (us_wait) {
>>> -			if (us_wait < 20000)
>>> -				usleep_range(us_wait, us_wait + 100);
>>> -			else
>>> -				msleep(us_wait / 1000);
>>> -		}
>>> +		if (us_wait)
>>> +			fsleep(us_wait);
>>>    	}
>>>    	gb_pm_runtime_put_autosuspend(bundle);
>>