Messages in this thread | | | Date | Fri, 26 Apr 2013 09:10:55 +0800 | From | ZhenHua <> | Subject | Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver |
| |
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
| |