Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1557502 > unrolled thread

[PATCH RFC] usb: dwc2: host: use msleep() for long delay

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC] usb: dwc2: host: use msleep() for long delay Nicholas Mc Guire <hofrat@osadl.org> - 2017-01-12 16:20 +0100
    Re: [PATCH RFC] usb: dwc2: host: use msleep() for long delay John Youn <John.Youn@synopsys.com> - 2017-01-13 01:40 +0100

#1557502 — [PATCH RFC] usb: dwc2: host: use msleep() for long delay

ulseep_range() uses hrtimers and provides no advantage over msleep()
for larger delays. Fix up the 100ms delays here passing the adjusted "min"
value to msleep(). This helps reduce the load on the hrtimer subsystem.

Link: http://lkml.org/lkml/2017/1/11/377
Fixes: commit 2938fc63e0c2 ("usb: dwc2: Properly account for the force mode delays")
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
Problem was found by cocinelle script.

Note that this originally was an msleep(25) then commit 2938fc63e0c2 
("usb: dwc2: Properly account for the force mode delays") changed this 
to usleep_range(100000,110000) with the comment in the function head:
<snip>
 After clearing the bits, wait up to 100 ms to account for any
 potential IDDIG filter delay...
<snip>
but without explaining why the API was switched to usleep_range() here
It does not look like there is any reason for using a high resolution 
timer here - the stated requirement is clearly satisfied by msleep(100)
as well. But it originally was usleep_range(150000, 160000) in
commit 09c96980dc72 ("usb: dwc2: Add functions to set and clear force mode")
so not sure if I am not overlooking some reason for the highrestimer here ?

This needs to be clarified by someone that knows the details and history
of this device/driver.

Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2

Patch is aginast 4.10-rc3 (localversion-next is next-20170112)

 drivers/usb/dwc2/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 11d8ae9..439a21b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -455,7 +455,7 @@ void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
 	dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
 
 	if (dwc2_iddig_filter_enabled(hsotg))
-		usleep_range(100000, 110000);
+		msleep(100);
 }
 
 /*
-- 
2.1.4

[toc] | [next] | [standalone]

#1557935

On 1/12/2017 7:15 AM, Nicholas Mc Guire wrote:
> ulseep_range() uses hrtimers and provides no advantage over msleep()
> for larger delays. Fix up the 100ms delays here passing the adjusted "min"
> value to msleep(). This helps reduce the load on the hrtimer subsystem.
> 
> Link: https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.org_lkml_2017_1_11_377&d=DgIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=dvQDbbH_QF2kZjARIDPimFAK95z8eOnw04feJuHCrJg&s=Prc0m1oUG4u8n8TFK-fdgzitMzE2ep6pM3L6y_DQeRs&e= 
> Fixes: commit 2938fc63e0c2 ("usb: dwc2: Properly account for the force mode delays")
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> Problem was found by cocinelle script.
> 
> Note that this originally was an msleep(25) then commit 2938fc63e0c2 
> ("usb: dwc2: Properly account for the force mode delays") changed this 
> to usleep_range(100000,110000) with the comment in the function head:
> <snip>
>  After clearing the bits, wait up to 100 ms to account for any
>  potential IDDIG filter delay...
> <snip>
> but without explaining why the API was switched to usleep_range() here
> It does not look like there is any reason for using a high resolution 
> timer here - the stated requirement is clearly satisfied by msleep(100)
> as well. But it originally was usleep_range(150000, 160000) in
> commit 09c96980dc72 ("usb: dwc2: Add functions to set and clear force mode")
> so not sure if I am not overlooking some reason for the highrestimer here ?
> 
> This needs to be clarified by someone that knows the details and history
> of this device/driver.
> 
> Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2
> 
> Patch is aginast 4.10-rc3 (localversion-next is next-20170112)
> 
>  drivers/usb/dwc2/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 11d8ae9..439a21b 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -455,7 +455,7 @@ void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>  	dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
>  
>  	if (dwc2_iddig_filter_enabled(hsotg))
> -		usleep_range(100000, 110000);
> +		msleep(100);
>  }
>  
>  /*
> 

+Felipe

Acked-by: John Youn <johnyoun@synopsys.com>

John

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web