From: "Li, Zhen-Hua" <[email protected]>
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 <[email protected]>
---
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
On Tue, Apr 23, 2013 at 03:15:01PM +0800, Li, Zhen-Hua wrote:
> From: "Li, Zhen-Hua" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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" <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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" <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
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" <[email protected]>
> >>
> >>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
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" <[email protected]>
>>>>
>>>> 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
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" <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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
>
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
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