On Tue, Apr 06, 2010 at 05:35:41PM +0800, Yang, Libin wrote: > Hi Sarah, > > Thanks for the comments. Please see my comment inline. > > > -----Original Message----- > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > Sent: Tuesday, April 06, 2010 6:41 AM > > To: Cai, Crane; Yang, Libin; linux-usb@xxxxxxxxxxxxxxx; Xu, Andiry; > > gregkh@xxxxxxx > > Subject: Re: [RESEND] [PATCH RFC 1/5] xhci: port power management > > implementation > > > > Hi Crane, > > > > Comments inline. > > > > I tried to apply this patch, and patches 2-4 that Libin sent, but I > > could only get this one to apply to 2.6.34-rc1. I'm seeing issues with > > this patch when I test with an external hub; I'll send the dmesg > > separately. > > I will send the new patch set based on your latest driver code. Ok, thanks. :) > > > Tested with USB storage device & mouse. > > > > So a USB storage device will never suspend, because the current mass > > storage device driver doesn't support suspend. I think the USB HID > > driver supports suspend though. The pl2303 USB-to-serial driver > > supports suspend too. > > > > You can be sure a device is really auto-suspended if you see a line like > > this in the dmesg: > > > > usb 1-3: usb auto-suspend > > > > Did you see that in your testing? > > In 2.6.32, based on the documentation documentation/usb/Power-management.txt, there are 3 values can be set for power/level: "on", "auto" or "suspend". We did the suspend/resume test manually by setting it "suspend" and "on". We did not use the auto-suspend method to test. Ok. The method of echoing "suspend" and "on" to the files is one way to test device suspend, but it's not what users or distros usually do. You'll exercise more code paths if you also try echoing "auto" to the file and letting the USB device driver and USB core decide when to suspend the device. You can cat the active_duration file in that directory to see if the device is actually suspended, or just watch dmesg. > > > + if (unlikely(TRB_TO_SUSPEND_PORT( > > > + xhci->cmd_ring->dequeue->generic.field[3]))) { > > > + slot_id = TRB_TO_SLOT_ID( > > > + xhci->cmd_ring->dequeue->generic.field[3]); > > > + virt_dev = xhci->devs[slot_id]; > > > + if (virt_dev) > > > + handle_cmd_in_cmd_wait_list(xhci, virt_dev, > > > + event); > > > + else > > > + xhci_warn(xhci, "Stop endpoint command " > > > + "completion for disabled slot %u\n", > > > + slot_id); > > > + } else > > > + handle_stopped_endpoint(xhci, xhci->cmd_ring->dequeue); > > > > Can you please put this code into handle_stopped_endpoint instead of > > sticking it in the switch statement? I want to keep that switch > > statement as small as possible, because it's getting really ugly. > > > > OK, I will change the code to handle it in the function > handle_stopped_endpoint(). > > BTW: in the function handle_stopped_endpoint(), we noticed it will > restart the endpoint at last in the function. I wonder why the > endpoint is restarted here. Can we restart the endpoint when we use > it? We wrote the code if...else... because we are afraid the endpoint > will be restarted in the function handle_stopped_endpoint(). If you put the if block into handle_stopped_endpoint() before the memset and then add a return statement at the end of the if block, you won't restart the endpoint. Or put the if block code in a different function, like handle_suspended_endpoint(). Anything different as long as it doesn't add many more lines to that giant case statement. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
