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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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.
>
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;
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
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
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
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
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