[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Li, Zhen-Hua]

This patch is trying to fix this bug on SLES11 SP2:
    https://bugzilla.novell.com/show_bug.cgi?id=817035

On a large HP system with 64T memory and 60 logical cpus, when usb
driver inits the iLo Virtual USB Controller, there comes a warning
"Controller not stopped yet!". It is because the HP iLo virtual usb
device requires a longer delay.

Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
---
 drivers/usb/host/uhci-hcd.c |    5 +++++
 drivers/usb/host/uhci-hub.c |   12 +++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 4a86b63..af30517 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -355,6 +355,11 @@ __acquires(uhci->lock)
 		if (uhci->dead)
 			return;
 	}
+
+	/* HP's iLo Virtual USB Controller requires a longer delay. */
+	if (uhci->wait_for_hp)
+		wait_for_HP(uhci, USBSTS, USBSTS_HCH, 1000);
+
 	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
 		dev_warn(uhci_dev(uhci), "Controller not stopped yet!\n");
 
diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
index f87bee6..c3f772c 100644
--- a/drivers/usb/host/uhci-hub.c
+++ b/drivers/usb/host/uhci-hub.c
@@ -120,14 +120,15 @@ static void uhci_finish_suspend(struct uhci_hcd *uhci, int port,
 }
 
 /* Wait for the UHCI controller in HP's iLO2 server management chip.
- * It can take up to 250 us to finish a reset and set the CSC bit.
+ * It can take up to max_wait us to finish the operation.
  */
-static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr)
+static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr,
+		u16 status, int max_wait)
 {
 	int i;
 
-	for (i = 10; i < 250; i += 10) {
-		if (uhci_readw(uhci, port_addr) & USBPORTSC_CSC)
+	for (i = 10; i < max_wait; i += 10) {
+		if (uhci_readw(uhci, port_addr) & status)
 			return;
 		udelay(10);
 	}
@@ -151,7 +152,8 @@ static void uhci_check_ports(struct uhci_hcd *uhci)
 				/* HP's server management chip requires
 				 * a longer delay. */
 				if (uhci->wait_for_hp)
-					wait_for_HP(uhci, port_addr);
+					wait_for_HP(uhci, port_addr,
+						USBPORTSC_CSC, 250);
 
 				/* If the port was enabled before, turning
 				 * reset on caused a port enable change.
-- 
1.7.10.4

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

There is a function  wait_for_HP() in uhci-hub.c. In this
patch, it is used in suspend_rh(),  I think this can be a
solution. And I have tested this patch, it can fix the bug.

I think there is another patch needed. As Alan said in another
mail, in the UHCI_RH_RUNNING_NODEVS case, it should not be stopped
if the uhci device is HP iLo virtual usb.


Thanks
Zhen-Hua

On 04/26/2013 03:38 PM, Li, Zhen-Hua wrote:
> This patch is trying to fix this bug on SLES11 SP2:
>      https://bugzilla.novell.com/show_bug.cgi?id=817035
>
> On a large HP system with 64T memory and 60 logical cpus, when usb
> driver inits the iLo Virtual USB Controller, there comes a warning
> "Controller not stopped yet!". It is because the HP iLo virtual usb
> device requires a longer delay.
>
> Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
> ---
>   drivers/usb/host/uhci-hcd.c |    5 +++++
>   drivers/usb/host/uhci-hub.c |   12 +++++++-----
>   2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> index 4a86b63..af30517 100644
> --- a/drivers/usb/host/uhci-hcd.c
> +++ b/drivers/usb/host/uhci-hcd.c
> @@ -355,6 +355,11 @@ __acquires(uhci->lock)
>   		if (uhci->dead)
>   			return;
>   	}
> +
> +	/* HP's iLo Virtual USB Controller requires a longer delay. */
> +	if (uhci->wait_for_hp)
> +		wait_for_HP(uhci, USBSTS, USBSTS_HCH, 1000);
> +
>   	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
>   		dev_warn(uhci_dev(uhci), "Controller not stopped yet!\n");
>   
> diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
> index f87bee6..c3f772c 100644
> --- a/drivers/usb/host/uhci-hub.c
> +++ b/drivers/usb/host/uhci-hub.c
> @@ -120,14 +120,15 @@ static void uhci_finish_suspend(struct uhci_hcd *uhci, int port,
>   }
>   
>   /* Wait for the UHCI controller in HP's iLO2 server management chip.
> - * It can take up to 250 us to finish a reset and set the CSC bit.
> + * It can take up to max_wait us to finish the operation.
>    */
> -static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr)
> +static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr,
> +		u16 status, int max_wait)
>   {
>   	int i;
>   
> -	for (i = 10; i < 250; i += 10) {
> -		if (uhci_readw(uhci, port_addr) & USBPORTSC_CSC)
> +	for (i = 10; i < max_wait; i += 10) {
> +		if (uhci_readw(uhci, port_addr) & status)
>   			return;
>   		udelay(10);
>   	}
> @@ -151,7 +152,8 @@ static void uhci_check_ports(struct uhci_hcd *uhci)
>   				/* HP's server management chip requires
>   				 * a longer delay. */
>   				if (uhci->wait_for_hp)
> -					wait_for_HP(uhci, port_addr);
> +					wait_for_HP(uhci, port_addr,
> +						USBPORTSC_CSC, 250);
>   
>   				/* If the port was enabled before, turning
>   				 * reset on caused a port enable change.

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

On 04/26/2013 03:50 PM, ZhenHua wrote:

> I think there is another patch needed. As Alan said in another
> mail, in the UHCI_RH_RUNNING_NODEVS case, it should not be stopped
> if the uhci device is HP iLo virtual usb.
>
correct: it should not be auto stopped if the uhci device is HP iLo 
virtual usb.
>
> Thanks
> Zhen-Hua
>
> On 04/26/2013 03:38 PM, Li, Zhen-Hua wrote:
>> This patch is trying to fix this bug on SLES11 SP2:
>>      https://bugzilla.novell.com/show_bug.cgi?id=817035
>>
>> On a large HP system with 64T memory and 60 logical cpus, when usb
>> driver inits the iLo Virtual USB Controller, there comes a warning
>> "Controller not stopped yet!". It is because the HP iLo virtual usb
>> device requires a longer delay.
>>
>> Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
>> ---
>>   drivers/usb/host/uhci-hcd.c |    5 +++++
>>   drivers/usb/host/uhci-hub.c |   12 +++++++-----
>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
>> index 4a86b63..af30517 100644
>> --- a/drivers/usb/host/uhci-hcd.c
>> +++ b/drivers/usb/host/uhci-hcd.c
>> @@ -355,6 +355,11 @@ __acquires(uhci->lock)
>>           if (uhci->dead)
>>               return;
>>       }
>> +
>> +    /* HP's iLo Virtual USB Controller requires a longer delay. */
>> +    if (uhci->wait_for_hp)
>> +        wait_for_HP(uhci, USBSTS, USBSTS_HCH, 1000);
>> +
>>       if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
>>           dev_warn(uhci_dev(uhci), "Controller not stopped yet!\n");
>>   diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
>> index f87bee6..c3f772c 100644
>> --- a/drivers/usb/host/uhci-hub.c
>> +++ b/drivers/usb/host/uhci-hub.c
>> @@ -120,14 +120,15 @@ static void uhci_finish_suspend(struct uhci_hcd 
>> *uhci, int port,
>>   }
>>     /* Wait for the UHCI controller in HP's iLO2 server management chip.
>> - * It can take up to 250 us to finish a reset and set the CSC bit.
>> + * It can take up to max_wait us to finish the operation.
>>    */
>> -static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr)
>> +static void wait_for_HP(struct uhci_hcd *uhci, unsigned long port_addr,
>> +        u16 status, int max_wait)
>>   {
>>       int i;
>>   -    for (i = 10; i < 250; i += 10) {
>> -        if (uhci_readw(uhci, port_addr) & USBPORTSC_CSC)
>> +    for (i = 10; i < max_wait; i += 10) {
>> +        if (uhci_readw(uhci, port_addr) & status)
>>               return;
>>           udelay(10);
>>       }
>> @@ -151,7 +152,8 @@ static void uhci_check_ports(struct uhci_hcd *uhci)
>>                   /* HP's server management chip requires
>>                    * a longer delay. */
>>                   if (uhci->wait_for_hp)
>> -                    wait_for_HP(uhci, port_addr);
>> +                    wait_for_HP(uhci, port_addr,
>> +                        USBPORTSC_CSC, 250);
>>                     /* If the port was enabled before, turning
>>                    * reset on caused a port enable change.
>

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Fri, 26 Apr 2013, ZhenHua wrote:

> There is a function  wait_for_HP() in uhci-hub.c. In this
> patch, it is used in suspend_rh(),  I think this can be a
> solution. And I have tested this patch, it can fix the bug.
> 
> I think there is another patch needed. As Alan said in another
> mail, in the UHCI_RH_RUNNING_NODEVS case, it should not be stopped
> if the uhci device is HP iLo virtual usb.

I believe that if you change the UHCI_RH_RUNNING_NODEVS case, you will 
find that this patch (calling wait_for_HP) isn't needed.

In fact, the patch is so easy that I am including it below.  Please 
test this (without either of your patches) to see if it works.

Alan Stern




Index: usb-3.9/drivers/usb/host/uhci-hub.c
===================================================================
--- usb-3.9.orig/drivers/usb/host/uhci-hub.c
+++ usb-3.9/drivers/usb/host/uhci-hub.c
@@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
 		/* auto-stop if nothing connected for 1 second */
 		if (any_ports_active(uhci))
 			uhci->rh_state = UHCI_RH_RUNNING;
-		else if (time_after_eq(jiffies, uhci->auto_stop_time))
+		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
+				!uhci->wait_for_hp)
 			suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
 		break;
 

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

On 04/27/2013 12:51 AM, Alan Stern wrote:
> On Fri, 26 Apr 2013, ZhenHua wrote:
>
>> There is a function  wait_for_HP() in uhci-hub.c. In this
>> patch, it is used in suspend_rh(),  I think this can be a
>> solution. And I have tested this patch, it can fix the bug.
>>
>> I think there is another patch needed. As Alan said in another
>> mail, in the UHCI_RH_RUNNING_NODEVS case, it should not be stopped
>> if the uhci device is HP iLo virtual usb.
> I believe that if you change the UHCI_RH_RUNNING_NODEVS case, you will
> find that this patch (calling wait_for_HP) isn't needed.
>
> In fact, the patch is so easy that I am including it below.  Please
> test this (without either of your patches) to see if it works.
>
> Alan Stern
>
>
>
>
> Index: usb-3.9/drivers/usb/host/uhci-hub.c
> ===================================================================
> --- usb-3.9.orig/drivers/usb/host/uhci-hub.c
> +++ usb-3.9/drivers/usb/host/uhci-hub.c
> @@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
>   		/* auto-stop if nothing connected for 1 second */
>   		if (any_ports_active(uhci))
>   			uhci->rh_state = UHCI_RH_RUNNING;
> -		else if (time_after_eq(jiffies, uhci->auto_stop_time))
> +		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
> +				!uhci->wait_for_hp)
>   			suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
>   		break;
>   
>
I have tested the UHCI_RH_RUNNING_NODEVS case yeasterday, and it works.
But the function suspend_rh is also called in other places, so I think 
it only fixes
the warning when auto stop is called, but not fix the warning when 
uhci's bus_suspend
is called,  it will come out again.

And if you add uhci->wait_for_hp check in the UHCI_RH_RUNNING_NODEVS case,
all hp uhci devices will not auto stop, not only the virtual devices. I 
guess it may waste resource.

Thanks
Zhen-Hua

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Sat, 27 Apr 2013, ZhenHua wrote:

> On 04/27/2013 12:51 AM, Alan Stern wrote:
> > On Fri, 26 Apr 2013, ZhenHua wrote:
> >
> >> There is a function  wait_for_HP() in uhci-hub.c. In this
> >> patch, it is used in suspend_rh(),  I think this can be a
> >> solution. And I have tested this patch, it can fix the bug.
> >>
> >> I think there is another patch needed. As Alan said in another
> >> mail, in the UHCI_RH_RUNNING_NODEVS case, it should not be stopped
> >> if the uhci device is HP iLo virtual usb.
> > I believe that if you change the UHCI_RH_RUNNING_NODEVS case, you will
> > find that this patch (calling wait_for_HP) isn't needed.
> >
> > In fact, the patch is so easy that I am including it below.  Please
> > test this (without either of your patches) to see if it works.
> >
> > Alan Stern
> >
> >
> >
> >
> > Index: usb-3.9/drivers/usb/host/uhci-hub.c
> > ===================================================================
> > --- usb-3.9.orig/drivers/usb/host/uhci-hub.c
> > +++ usb-3.9/drivers/usb/host/uhci-hub.c
> > @@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
> >   		/* auto-stop if nothing connected for 1 second */
> >   		if (any_ports_active(uhci))
> >   			uhci->rh_state = UHCI_RH_RUNNING;
> > -		else if (time_after_eq(jiffies, uhci->auto_stop_time))
> > +		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
> > +				!uhci->wait_for_hp)
> >   			suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
> >   		break;
> >   
> >
> I have tested the UHCI_RH_RUNNING_NODEVS case yeasterday, and it works.
> But the function suspend_rh is also called in other places, so I think 
> it only fixes
> the warning when auto stop is called, but not fix the warning when 
> uhci's bus_suspend
> is called,  it will come out again.

Have you tried this?  I expect the warning will not occur when the
bus_suspend routine is called, because then there will be a 1-ms delay,
not just a 400-us delay.

> And if you add uhci->wait_for_hp check in the UHCI_RH_RUNNING_NODEVS case,
> all hp uhci devices will not auto stop, not only the virtual devices. I 
> guess it may waste resource.

If you want, you can add a new flag specifically for virtual
controllers.  But it shouldn't matter -- as long as your kernels are
built with CONFIG_PM_RUNTIME enabled, there won't be any significant
waste of resources.

Alan Stern

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

On 04/27/2013 11:14 PM, Alan Stern wrote:
> On Sat, 27 Apr 2013, ZhenHua wrote:
>
>> On 04/27/2013 12:51 AM, Alan Stern wrote:
>>> On Fri, 26 Apr 2013, ZhenHua wrote:
>>>
>>>> There is a function  wait_for_HP() in uhci-hub.c. In this
>>>> patch, it is used in suspend_rh(),  I think this can be a
>>>> solution. And I have tested this patch, it can fix the bug.
>>>>
>>>> I think there is another patch needed. As Alan said in another
>>>> mail, in the UHCI_RH_RUNNING_NODEVS case, it should not be stopped
>>>> if the uhci device is HP iLo virtual usb.
>>> I believe that if you change the UHCI_RH_RUNNING_NODEVS case, you will
>>> find that this patch (calling wait_for_HP) isn't needed.
>>>
>>> In fact, the patch is so easy that I am including it below.  Please
>>> test this (without either of your patches) to see if it works.
>>>
>>> Alan Stern
>>>
>>>
>>>
>>>
>>> Index: usb-3.9/drivers/usb/host/uhci-hub.c
>>> ===================================================================
>>> --- usb-3.9.orig/drivers/usb/host/uhci-hub.c
>>> +++ usb-3.9/drivers/usb/host/uhci-hub.c
>>> @@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
>>>    		/* auto-stop if nothing connected for 1 second */
>>>    		if (any_ports_active(uhci))
>>>    			uhci->rh_state = UHCI_RH_RUNNING;
>>> -		else if (time_after_eq(jiffies, uhci->auto_stop_time))
>>> +		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
>>> +				!uhci->wait_for_hp)
>>>    			suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
>>>    		break;
>>>    
>>>
>> I have tested the UHCI_RH_RUNNING_NODEVS case yeasterday, and it works.
>> But the function suspend_rh is also called in other places, so I think
>> it only fixes
>> the warning when auto stop is called, but not fix the warning when
>> uhci's bus_suspend
>> is called,  it will come out again.
> Have you tried this?  I expect the warning will not occur when the
> bus_suspend routine is called, because then there will be a 1-ms delay,
> not just a 400-us delay.
I tested this, and the warning is gone.  Is this patch committed ?
I need to paste the link to suse bugzilla.

>
>> And if you add uhci->wait_for_hp check in the UHCI_RH_RUNNING_NODEVS case,
>> all hp uhci devices will not auto stop, not only the virtual devices. I
>> guess it may waste resource.
> If you want, you can add a new flag specifically for virtual
> controllers.  But it shouldn't matter -- as long as your kernels are
> built with CONFIG_PM_RUNTIME enabled, there won't be any significant
> waste of resources.
>
> Alan Stern
>
I think we can check the product id to determine whether a device is 
virtual.
Do you know if there is another way to check this?

Thanks
Zhen-Hua

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Sun, 28 Apr 2013, ZhenHua wrote:

> >>> In fact, the patch is so easy that I am including it below.  Please
> >>> test this (without either of your patches) to see if it works.
> >>>
> >>> Alan Stern
> >>>
> >>>
> >>>
> >>>
> >>> Index: usb-3.9/drivers/usb/host/uhci-hub.c
> >>> ===================================================================
> >>> --- usb-3.9.orig/drivers/usb/host/uhci-hub.c
> >>> +++ usb-3.9/drivers/usb/host/uhci-hub.c
> >>> @@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
> >>>    		/* auto-stop if nothing connected for 1 second */
> >>>    		if (any_ports_active(uhci))
> >>>    			uhci->rh_state = UHCI_RH_RUNNING;
> >>> -		else if (time_after_eq(jiffies, uhci->auto_stop_time))
> >>> +		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
> >>> +				!uhci->wait_for_hp)
> >>>    			suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
> >>>    		break;
> >>>    
> >>>
> >> I have tested the UHCI_RH_RUNNING_NODEVS case yeasterday, and it works.
> >> But the function suspend_rh is also called in other places, so I think
> >> it only fixes
> >> the warning when auto stop is called, but not fix the warning when
> >> uhci's bus_suspend
> >> is called,  it will come out again.
> > Have you tried this?  I expect the warning will not occur when the
> > bus_suspend routine is called, because then there will be a 1-ms delay,
> > not just a 400-us delay.
> I tested this, and the warning is gone.  Is this patch committed ?
> I need to paste the link to suse bugzilla.

You must be joking.  I wrote that patch while composing the email
message to you, and nobody except you has tested it.

Since you confirm that it works, I will submit it.  But new patches
like this won't be accepted until after the upcoming merge window
closes, which won't be for more than two weeks.

> >> And if you add uhci->wait_for_hp check in the UHCI_RH_RUNNING_NODEVS case,
> >> all hp uhci devices will not auto stop, not only the virtual devices. I
> >> guess it may waste resource.
> > If you want, you can add a new flag specifically for virtual
> > controllers.  But it shouldn't matter -- as long as your kernels are
> > built with CONFIG_PM_RUNTIME enabled, there won't be any significant
> > waste of resources.
> >
> > Alan Stern
> >
> I think we can check the product id to determine whether a device is 
> virtual.
> Do you know if there is another way to check this?

I don't know anything at all about your virtual UHCI controllers, other
than what you have told me.  In particular, I don't know what product
IDs are used by HP's virtual and non-virtual controllers.  Maybe
somebody else at HP can tell you.

Alan Stern

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Li, Zhen-Hua (USL-China)]

On 04/29/2013 02:55 AM, Alan Stern wrote:
> On Sun, 28 Apr 2013, ZhenHua wrote:
>
>>>>> In fact, the patch is so easy that I am including it below.  Please
>>>>> test this (without either of your patches) to see if it works.
>>>>>
>>>>> Alan Stern
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Index: usb-3.9/drivers/usb/host/uhci-hub.c
>>>>> ===================================================================
>>>>> --- usb-3.9.orig/drivers/usb/host/uhci-hub.c
>>>>> +++ usb-3.9/drivers/usb/host/uhci-hub.c
>>>>> @@ -225,7 +225,8 @@ static int uhci_hub_status_data(struct u
>>>>>     		/* auto-stop if nothing connected for 1 second */
>>>>>     		if (any_ports_active(uhci))
>>>>>     			uhci->rh_state = UHCI_RH_RUNNING;
>>>>> -		else if (time_after_eq(jiffies, uhci->auto_stop_time))
>>>>> +		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
>>>>> +				!uhci->wait_for_hp)
>>>>>     			suspend_rh(uhci, UHCI_RH_AUTO_STOPPED);
>>>>>     		break;
>>>>>     
>>>>>
>>>> I have tested the UHCI_RH_RUNNING_NODEVS case yeasterday, and it works.
>>>> But the function suspend_rh is also called in other places, so I think
>>>> it only fixes
>>>> the warning when auto stop is called, but not fix the warning when
>>>> uhci's bus_suspend
>>>> is called,  it will come out again.
>>> Have you tried this?  I expect the warning will not occur when the
>>> bus_suspend routine is called, because then there will be a 1-ms delay,
>>> not just a 400-us delay.
>> I tested this, and the warning is gone.  Is this patch committed ?
>> I need to paste the link to suse bugzilla.
Not joking. I tested both of them , uhci->wait_for_hp and no_auto_stop , 
for the UHCI_RH_RUNNING_NODEVS
  case. And as uhci->wait_for_hp is 1 on my system, so they got the same 
result.

> You must be joking.  I wrote that patch while composing the email
> message to you, and nobody except you has tested it.
>
> Since you confirm that it works, I will submit it.  But new patches
> like this won't be accepted until after the upcoming merge window
> closes, which won't be for more than two weeks.

>
>>>> And if you add uhci->wait_for_hp check in the UHCI_RH_RUNNING_NODEVS case,
>>>> all hp uhci devices will not auto stop, not only the virtual devices. I
>>>> guess it may waste resource.
>>> If you want, you can add a new flag specifically for virtual
>>> controllers.  But it shouldn't matter -- as long as your kernels are
>>> built with CONFIG_PM_RUNTIME enabled, there won't be any significant
>>> waste of resources.
>>>
>>> Alan Stern
>>>
>> I think we can check the product id to determine whether a device is
>> virtual.
>> Do you know if there is another way to check this?
> I don't know anything at all about your virtual UHCI controllers, other
> than what you have told me.  In particular, I don't know what product
> IDs are used by HP's virtual and non-virtual controllers.  Maybe
> somebody else at HP can tell you.
>
> Alan Stern
>
> .
So I can only check the product IDs.

Thanks for helping me on this bug.


Regards
Zhen-Hua

[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Li, Zhen-Hua]

This patch is trying to fix this bug on SLES11 SP2:
    https://bugzilla.novell.com/show_bug.cgi?id=817035

On a large HP system with 64T memory and 60 logical cpus, when usb
driver inits the iLo Virtual USB Controller, there comes a warning
"Controller not stopped yet!". It is because driver does not wait
enough time. So driver should wait and retry if found controller 
 not stopped.

Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
Signed-off-by: Tom Vaden <tom.vaden@hp.com>
---
 drivers/usb/host/uhci-hcd.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 4a86b63..d592e22 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
 		uhci->global_suspend_mode_is_broken(uhci) : 0;
 }
 
+#define UHCI_SUSPENDRH_RETRY_MAX      10
+#define UHCI_SUSPENDRH_RETRY_DELAY    100
+
 static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
 __releases(uhci->lock)
 __acquires(uhci->lock)
@@ -284,6 +287,8 @@ __acquires(uhci->lock)
 	int auto_stop;
 	int int_enable, egsm_enable, wakeup_enable;
 	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
+	int try;
+	u16 stopped;
 
 	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
 	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
@@ -355,7 +360,17 @@ __acquires(uhci->lock)
 		if (uhci->dead)
 			return;
 	}
-	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
+
+	for (try = 0; try < UHCI_SUSPENDRH_RETRY_MAX; try++) {
+		/* For some devices, for example, HP iLo usb controller,
+		 * we need to wait for more time and retry. 
+		 */
+		stopped = uhci_readw(uhci, USBSTS) & USBSTS_HCH;
+		if (stopped)
+			break;
+		udelay(UHCI_SUSPENDRH_RETRY_DELAY);
+	}
+	if (!stopped)
 		dev_warn(uhci_dev(uhci), "Controller not stopped yet!\n");
 
 	uhci_get_current_frame_number(uhci);
-- 
1.7.10.4

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

I send out this patch for the second time.
Changed try to int. And modified the comment.

On 04/25/2013 03:11 PM, Li, Zhen-Hua wrote:
> This patch is trying to fix this bug on SLES11 SP2:
>      https://bugzilla.novell.com/show_bug.cgi?id=817035
>
> On a large HP system with 64T memory and 60 logical cpus, when usb
> driver inits the iLo Virtual USB Controller, there comes a warning
> "Controller not stopped yet!". It is because driver does not wait
> enough time. So driver should wait and retry if found controller
>   not stopped.
>
> Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
> Signed-off-by: Tom Vaden <tom.vaden@hp.com>
> ---
>   drivers/usb/host/uhci-hcd.c |   17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> index 4a86b63..d592e22 100644
> --- a/drivers/usb/host/uhci-hcd.c
> +++ b/drivers/usb/host/uhci-hcd.c
> @@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
>   		uhci->global_suspend_mode_is_broken(uhci) : 0;
>   }
>   
> +#define UHCI_SUSPENDRH_RETRY_MAX      10
> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
> +
>   static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
>   __releases(uhci->lock)
>   __acquires(uhci->lock)
> @@ -284,6 +287,8 @@ __acquires(uhci->lock)
>   	int auto_stop;
>   	int int_enable, egsm_enable, wakeup_enable;
>   	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
> +	int try;
> +	u16 stopped;
>   
>   	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
>   	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
> @@ -355,7 +360,17 @@ __acquires(uhci->lock)
>   		if (uhci->dead)
>   			return;
>   	}
> -	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
> +
> +	for (try = 0; try < UHCI_SUSPENDRH_RETRY_MAX; try++) {
> +		/* For some devices, for example, HP iLo usb controller,
> +		 * we need to wait for more time and retry.
> +		 */
> +		stopped = uhci_readw(uhci, USBSTS) & USBSTS_HCH;
> +		if (stopped)
> +			break;
> +		udelay(UHCI_SUSPENDRH_RETRY_DELAY);
> +	}
> +	if (!stopped)
>   		dev_warn(uhci_dev(uhci), "Controller not stopped yet!\n");
>   
>   	uhci_get_current_frame_number(uhci);

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Thu, 25 Apr 2013, ZhenHua wrote:

> I send out this patch for the second time.
> Changed try to int. And modified the comment.

You did not answer my question: How long does it take for the iLo 
controller to go into suspend?

Alan Stern

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Thu, 25 Apr 2013, Alan Stern wrote:

> On Thu, 25 Apr 2013, ZhenHua wrote:
> 
> > I send out this patch for the second time.
> > Changed try to int. And modified the comment.
> 
> You did not answer my question: How long does it take for the iLo 
> controller to go into suspend?

Oops, sorry, I see now that you did answer the question in a different
email message.

Alan Stern

[PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Li, Zhen-Hua]

From: "Li, Zhen-Hua" <zhen-hual@hp.com>

This patch is trying to fix bug QXCR1001261767.
On some HP platform, when usb driver inits the iLo Virtual USB Controller, there may be a warning "Controller not stopped yet!". It is because driver does not wait enough time.
This patch adds more time waiting and retries.

Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
---
 drivers/usb/host/uhci-hcd.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 4a86b63..514e9d7 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
 		uhci->global_suspend_mode_is_broken(uhci) : 0;
 }
 
+#define UHCI_SUSPENDRH_RETRY_MAX      10
+#define UHCI_SUSPENDRH_RETRY_DELAY    100
+
 static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
 __releases(uhci->lock)
 __acquires(uhci->lock)
@@ -284,6 +287,7 @@ __acquires(uhci->lock)
 	int auto_stop;
 	int int_enable, egsm_enable, wakeup_enable;
 	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
+	u16 try, stopped;
 
 	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
 	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
@@ -355,7 +359,17 @@ __acquires(uhci->lock)
 		if (uhci->dead)
 			return;
 	}
-	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
+
+	for (try = 0; try < UHCI_SUSPENDRH_RETRY_MAX; try++) {
+		/*
+		 * Sometimes we may need to wait more time and try again.
+		 */
+		stopped = uhci_readw(uhci, USBSTS) & USBSTS_HCH;
+		if (stopped)
+			break;
+		udelay(UHCI_SUSPENDRH_RETRY_DELAY);
+	}
+	if (!stopped)
 		dev_warn(uhci_dev(uhci), "Controller not stopped yet!\n");
 
 	uhci_get_current_frame_number(uhci);
-- 
1.7.10.4

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Greg KH]

On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
> From: "Li, Zhen-Hua" <zhen-hual@hp.com>
> 
> This patch is trying to fix bug QXCR1001261767.

What is that bug number?  Where can it be referenced?  If you are going
to put it in a public place (like a kernel changelog), it needs to be
publicly accessible.

> On some HP platform, when usb driver inits the iLo Virtual USB Controller, there may be a warning "Controller not stopped yet!". It is because driver does not wait enough time.

What happened to your line endings?

> This patch adds more time waiting and retries.

Why not only do this for your device?

> 
> Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
> ---
>  drivers/usb/host/uhci-hcd.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> index 4a86b63..514e9d7 100644
> --- a/drivers/usb/host/uhci-hcd.c
> +++ b/drivers/usb/host/uhci-hcd.c
> @@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
>  		uhci->global_suspend_mode_is_broken(uhci) : 0;
>  }
>  
> +#define UHCI_SUSPENDRH_RETRY_MAX      10
> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
> +
>  static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
>  __releases(uhci->lock)
>  __acquires(uhci->lock)
> @@ -284,6 +287,7 @@ __acquires(uhci->lock)
>  	int auto_stop;
>  	int int_enable, egsm_enable, wakeup_enable;
>  	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
> +	u16 try, stopped;
>  
>  	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
>  	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
> @@ -355,7 +359,17 @@ __acquires(uhci->lock)
>  		if (uhci->dead)
>  			return;
>  	}
> -	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
> +
> +	for (try = 0; try < UHCI_SUSPENDRH_RETRY_MAX; try++) {
> +		/*
> +		 * Sometimes we may need to wait more time and try again.
> +		 */

"Sometimes"?  Please be more specific.  Also, a multi-line comment isn't
needed, make it one line please.

thanks,

greg k-h

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Tue, 23 Apr 2013, Greg KH wrote:

> On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
> > From: "Li, Zhen-Hua" <zhen-hual@hp.com>
> > 
> > This patch is trying to fix bug QXCR1001261767.
> 
> What is that bug number?  Where can it be referenced?  If you are going
> to put it in a public place (like a kernel changelog), it needs to be
> publicly accessible.
> 
> > On some HP platform, when usb driver inits the iLo Virtual USB Controller, there may be a warning "Controller not stopped yet!". It is because driver does not wait enough time.
> 
> What happened to your line endings?
> 
> > This patch adds more time waiting and retries.
> 
> Why not only do this for your device?

It won't hurt to do it for all devices, because the wait loop will 
terminate as soon as the controller goes into suspend.  For normal 
controllers this will be on the first iteration.

> > Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
> > ---
> >  drivers/usb/host/uhci-hcd.c |   16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> > index 4a86b63..514e9d7 100644
> > --- a/drivers/usb/host/uhci-hcd.c
> > +++ b/drivers/usb/host/uhci-hcd.c
> > @@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
> >  		uhci->global_suspend_mode_is_broken(uhci) : 0;
> >  }
> >  
> > +#define UHCI_SUSPENDRH_RETRY_MAX      10
> > +#define UHCI_SUSPENDRH_RETRY_DELAY    100

Why is the delay set to 100 us?  Isn't that excessively large?  How 
long does it take for this controller to go into suspend?

> >  static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
> >  __releases(uhci->lock)
> >  __acquires(uhci->lock)
> > @@ -284,6 +287,7 @@ __acquires(uhci->lock)
> >  	int auto_stop;
> >  	int int_enable, egsm_enable, wakeup_enable;
> >  	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
> > +	u16 try, stopped;

Why are these variables u16?  Why not int?

Anyway, a better approach would be not to add a delay loop at all.  
Instead, change this test:

	if (!auto_stop && !(uhci_readw(uhci, USBSTS) & USBSTS_HCH)) {
		uhci->rh_state = UHCI_RH_SUSPENDING;
		spin_unlock_irq(&uhci->lock);
		msleep(1);
		spin_lock_irq(&uhci->lock);
		if (uhci->dead)
			return;
	}

When the iLo controller is present, make the "if" statement always 
succeed.  Then you'll get a whole 1-ms delay.

Alan Stern

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

On 04/23/2013 11:10 PM, Alan Stern wrote:
> On Tue, 23 Apr 2013, Greg KH wrote:
>
>> On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
>>> From: "Li, Zhen-Hua" <zhen-hual@hp.com>
>>>
>>> This patch is trying to fix bug QXCR1001261767.
>> What is that bug number?  Where can it be referenced?  If you are going
>> to put it in a public place (like a kernel changelog), it needs to be
>> publicly accessible.
>>
>>> On some HP platform, when usb driver inits the iLo Virtual USB Controller, there may be a warning "Controller not stopped yet!". It is because driver does not wait enough time.
>> What happened to your line endings?
>>
>>> This patch adds more time waiting and retries.
>> Why not only do this for your device?
> It won't hurt to do it for all devices, because the wait loop will
> terminate as soon as the controller goes into suspend.  For normal
> controllers this will be on the first iteration.
Yes, most devices only need one time check.
>>> Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
>>> ---
>>>   drivers/usb/host/uhci-hcd.c |   16 +++++++++++++++-
>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
>>> index 4a86b63..514e9d7 100644
>>> --- a/drivers/usb/host/uhci-hcd.c
>>> +++ b/drivers/usb/host/uhci-hcd.c
>>> @@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
>>>   		uhci->global_suspend_mode_is_broken(uhci) : 0;
>>>   }
>>>   
>>> +#define UHCI_SUSPENDRH_RETRY_MAX      10
>>> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
> Why is the delay set to 100 us?  Isn't that excessively large?  How
> long does it take for this controller to go into suspend?
This controller will take about 200~400 us, but I am not sure how long 
other devices will take.
I set interval to 100 us,  so it will save more time.

>
>>>   static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
>>>   __releases(uhci->lock)
>>>   __acquires(uhci->lock)
>>> @@ -284,6 +287,7 @@ __acquires(uhci->lock)
>>>   	int auto_stop;
>>>   	int int_enable, egsm_enable, wakeup_enable;
>>>   	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
>>> +	u16 try, stopped;
> Why are these variables u16?  Why not int?
uhci_readw will return u16.
> Anyway, a better approach would be not to add a delay loop at all.
> Instead, change this test:
>
> 	if (!auto_stop && !(uhci_readw(uhci, USBSTS) & USBSTS_HCH)) {
> 		uhci->rh_state = UHCI_RH_SUSPENDING;
> 		spin_unlock_irq(&uhci->lock);
> 		msleep(1);
> 		spin_lock_irq(&uhci->lock);
> 		if (uhci->dead)
> 			return;
> 	}
>
> When the iLo controller is present, make the "if" statement always
> succeed.  Then you'll get a whole 1-ms delay.
This will cause more operation and more time for other devices.
> Alan Stern
>

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Alan Stern]

On Thu, 25 Apr 2013, ZhenHua wrote:

> >>> +#define UHCI_SUSPENDRH_RETRY_MAX      10
> >>> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
> > Why is the delay set to 100 us?  Isn't that excessively large?  How
> > long does it take for this controller to go into suspend?
> This controller will take about 200~400 us, but I am not sure how long 
> other devices will take.
> I set interval to 100 us,  so it will save more time.

A 400-us delay is fairly long.  It would be better to avoid it
entirely.

> > Why are these variables u16?  Why not int?
> uhci_readw will return u16.

That's not a good reason, since u16 fits perfectly well inside an
int.  But never mind...

> > Anyway, a better approach would be not to add a delay loop at all.
> > Instead, change this test:
> >
> > 	if (!auto_stop && !(uhci_readw(uhci, USBSTS) & USBSTS_HCH)) {
> > 		uhci->rh_state = UHCI_RH_SUSPENDING;
> > 		spin_unlock_irq(&uhci->lock);
> > 		msleep(1);
> > 		spin_lock_irq(&uhci->lock);
> > 		if (uhci->dead)
> > 			return;
> > 	}
> >
> > When the iLo controller is present, make the "if" statement always
> > succeed.  Then you'll get a whole 1-ms delay.
> This will cause more operation and more time for other devices.

Actually what I wrote was wrong anyway.  I forgot that when auto_stop 
is set, the routine is not allowed to sleep.

A better way to solve your problem is to change uhci_hub_status_data().
In the UHCI_RH_RUNNING_NODEVS case, change the line that says

		else if (time_after_eq(jiffies, uhci->auto_stop_time))

to

		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
				!uhci->no_auto_stops)

where uhci->no_auto_stops is a new bitflag that you set inside
uhci_pci_init() if you detect that the controller is an iLo virtual
UHCI controller.

This way there will always be a 1-ms delay, so the slow controller will
suspend successfully.  And other types of host controllers won't be
affected, because the no_auto_stops flag won't get set for them.

Alan Stern

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

On 04/25/2013 10:54 PM, Alan Stern wrote:
> On Thu, 25 Apr 2013, ZhenHua wrote:
>
>>>>> +#define UHCI_SUSPENDRH_RETRY_MAX      10
>>>>> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
>>> Why is the delay set to 100 us?  Isn't that excessively large?  How
>>> long does it take for this controller to go into suspend?
>> This controller will take about 200~400 us, but I am not sure how long
>> other devices will take.
>> I set interval to 100 us,  so it will save more time.
> A 400-us delay is fairly long.  It would be better to avoid it
The device needs about 200~400 us to get stopped, not OS.
For other devices, it will not wait.
> entirely.
>
>>> Why are these variables u16?  Why not int?
>> uhci_readw will return u16.
> That's not a good reason, since u16 fits perfectly well inside an
> int.  But never mind...
>
>>> Anyway, a better approach would be not to add a delay loop at all.
>>> Instead, change this test:
>>>
>>> 	if (!auto_stop && !(uhci_readw(uhci, USBSTS) & USBSTS_HCH)) {
>>> 		uhci->rh_state = UHCI_RH_SUSPENDING;
>>> 		spin_unlock_irq(&uhci->lock);
>>> 		msleep(1);
>>> 		spin_lock_irq(&uhci->lock);
>>> 		if (uhci->dead)
>>> 			return;
>>> 	}
>>>
>>> When the iLo controller is present, make the "if" statement always
>>> succeed.  Then you'll get a whole 1-ms delay.
>> This will cause more operation and more time for other devices.
> Actually what I wrote was wrong anyway.  I forgot that when auto_stop
> is set, the routine is not allowed to sleep.
>
> A better way to solve your problem is to change uhci_hub_status_data().
> In the UHCI_RH_RUNNING_NODEVS case, change the line that says
>
> 		else if (time_after_eq(jiffies, uhci->auto_stop_time))
>
> to
>
> 		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
> 				!uhci->no_auto_stops)
>
> where uhci->no_auto_stops is a new bitflag that you set inside
> uhci_pci_init() if you detect that the controller is an iLo virtual
> UHCI controller.
>
> This way there will always be a 1-ms delay, so the slow controller will
> suspend successfully.  And other types of host controllers won't be
> affected, because the no_auto_stops flag won't get set for them.
>
> Alan Stern
>

I  think it is a good idea, and the logic of the code may be more 
clear.  I will do some test on my system.

Thanks
Zhen-Hua

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

On 04/23/2013 10:07 PM, Greg KH wrote:
> On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
>> From: "Li, Zhen-Hua" <zhen-hual@hp.com>
>>
>> This patch is trying to fix bug QXCR1001261767.
Sorry for the bug number. Please ignore this line.

> What is that bug number?  Where can it be referenced?  If you are going
> to put it in a public place (like a kernel changelog), it needs to be
> publicly accessible.
>
>> On some HP platform, when usb driver inits the iLo Virtual USB Controller, there may be a warning "Controller not stopped yet!". It is because driver does not wait enough time.
> What happened to your line endings?
>
>> This patch adds more time waiting and retries.
> Why not only do this for your device?
>
>> Signed-off-by: Li, Zhen-Hua <zhen-hual@hp.com>
>> ---
>>   drivers/usb/host/uhci-hcd.c |   16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
>> index 4a86b63..514e9d7 100644
>> --- a/drivers/usb/host/uhci-hcd.c
>> +++ b/drivers/usb/host/uhci-hcd.c
>> @@ -277,6 +277,9 @@ static int global_suspend_mode_is_broken(struct uhci_hcd *uhci)
>>   		uhci->global_suspend_mode_is_broken(uhci) : 0;
>>   }
>>   
>> +#define UHCI_SUSPENDRH_RETRY_MAX      10
>> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
>> +
>>   static void suspend_rh(struct uhci_hcd *uhci, enum uhci_rh_state new_state)
>>   __releases(uhci->lock)
>>   __acquires(uhci->lock)
>> @@ -284,6 +287,7 @@ __acquires(uhci->lock)
>>   	int auto_stop;
>>   	int int_enable, egsm_enable, wakeup_enable;
>>   	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
>> +	u16 try, stopped;
>>   
>>   	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
>>   	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
>> @@ -355,7 +359,17 @@ __acquires(uhci->lock)
>>   		if (uhci->dead)
>>   			return;
>>   	}
>> -	if (!(uhci_readw(uhci, USBSTS) & USBSTS_HCH))
>> +
>> +	for (try = 0; try < UHCI_SUSPENDRH_RETRY_MAX; try++) {
>> +		/*
>> +		 * Sometimes we may need to wait more time and try again.
>> +		 */
> "Sometimes"?  Please be more specific.  Also, a multi-line comment isn't
> needed, make it one line please.
>
> thanks,
>
> greg k-h

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[Greg KH]

On Wed, Apr 24, 2013 at 07:55:18AM +0800, ZhenHua wrote:
> On 04/23/2013 10:07 PM, Greg KH wrote:
> >On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
> >>From: "Li, Zhen-Hua" <zhen-hual@hp.com>
> >>
> >>This patch is trying to fix bug QXCR1001261767.
> Sorry for the bug number. Please ignore this line.

I don't want to ignore it, I want it to point to something that I, and
others, can reference in the future.  Is there a url you can use for it?

thanks,

greg k-h

Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

[ZhenHua]

I did not found any url you can visit. And we will file a bug on SLES 
bugzilla site if possible.

I   paste the bug discription here:
When booting SLES 11 SP2 on our server , messages like the following are 
logged during boot:
[  254.087187] uhci_hcd 0000:07:00.4: Controller not stopped yet!

Thanks
Zhen-Hua

On 04/24/2013 09:57 AM, Greg KH wrote:
> On Wed, Apr 24, 2013 at 07:55:18AM +0800, ZhenHua wrote:
>> On 04/23/2013 10:07 PM, Greg KH wrote:
>>> On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
>>>> From: "Li, Zhen-Hua" <zhen-hual@hp.com>
>>>>
>>>> This patch is trying to fix bug QXCR1001261767.
>> Sorry for the bug number. Please ignore this line.
> I don't want to ignore it, I want it to point to something that I, and
> others, can reference in the future.  Is there a url you can use for it?
>
> thanks,
>
> greg k-h