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. > > > > Port Resume: > > Write '0' in PLS field, device will transition to running state. > > Ring an endpoints' doorbell to restart it. > > > > System trigger suspend/resume via: > > echo suspend > /sys/bus/usb/devices/../power/level > > You need to echo "auto" here, rather than suspend. "suspend" does not > work on kernels newer than 2.6.32. Using "auto" allows the kernel to > suspend the device when it's not idle, rather than forcing it into > suspend immediately. Most distros will echo auto here and let the USB > core do the work. You can't properly test your patches without setting > this to auto. Our development and test is based on 2.6.32. We will refine the patch set based on 2.6.34. We use "echo suspend > " just for testing. For 2.6.32, it is OK to suspend the device to use "echo suspend > ..." and we have done some test on it, it works well. We will change the comments and do the test based on your suggestion as "suspend" does not work on kernels newer than 2.6.32. > > > echo on > /sys/bus/usb/devices/../power/level > > Did you test remote wakeup at all? I'm seeing issues with external hubs > when you plug in a mouse when the hub is suspended (I'll send dmesg > separately). We did the remote wakeup test. For the remote wakeup, you must use the second patch: 0002-xHCI-port-remote-wakeup-implementation.patch. Without the patch, remote wakeup does not work. Without the second patch, to make the device resume, you must run "echo on > ..." command. > > > 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. > > + * Stop device > > + * It issues stop endpoint command for EP 0 to 30. And wait the last > command > > + * to complete. > > + * suspend will set to 1, if suspend bit need to set in command. > > + * Require xhci spin lock and will release it to wait commands finish. > > + */ > > +static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int > suspend) > > +{ > > + struct xhci_virt_device *virt_dev; > > + struct xhci_command *cmd; > > + unsigned long flags; > > + int timeleft; > > + int ret; > > + int i; > > + > > + ret = 0; > > + virt_dev = xhci->devs[slot_id]; > > + cmd = xhci_alloc_command(xhci, false, true, GFP_NOIO); > > + if (!cmd) { > > + xhci_dbg(xhci, "Couldn't allocate command structure.\n"); > > + return -ENOMEM; > > + } > > + > > + spin_lock_irqsave(&xhci->lock, flags); > > + for (i = 0; i < LAST_EP_INDEX; i++) > > + xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); > > It seems like you're issuing a stop endpoint command for all 30 > endpoints in the device context, regardless of if they are actually used > by the device. > > Say a device declares endpoints 0x03 and 0x86. Then the endpoint > contexts that are used will be at indexes 0, 5, and 12. But you issue a > stop endpoint command for all 30 endpoint indexes. That means you're > issuing 3 out of 30 useful commands, and waiting for all of them to > complete before suspending the device. > > That seems a bit wasteful. Why not look at the device output context > and only issue the stop endpoint command if the dequeue pointer is > non-null? To make sure you enqueue a command to the command list before > issuing the last stop endpoint command, you can save the default control > endpoint (index 0) for last. All devices must have that endpoint, so it > should be fine. OK. I will change the code based on your suggestion. > > > + cmd->command_trb = xhci->cmd_ring->enqueue; > > + list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list); > > + xhci_queue_stop_endpoint(xhci, slot_id, LAST_EP_INDEX, suspend); > > + xhci_ring_cmd_db(xhci); > > + spin_unlock_irqrestore(&xhci->lock, flags); > > + > > + /* Wait for last stop endpoint command to finish */ > > + timeleft = wait_for_completion_interruptible_timeout( > > + cmd->completion, > > + USB_CTRL_SET_TIMEOUT); > > + if (timeleft <= 0) { > > + xhci_warn(xhci, "%s while waiting for reset device command\n", > > + timeleft == 0 ? "Timeout" : "Signal"); > > + spin_lock_irqsave(&xhci->lock, flags); > > + /* The timeout might have raced with the event ring handler, > so > > + * only delete from the list if the item isn't poisoned. > > + */ > > + if (cmd->cmd_list.next != LIST_POISON1) > > + list_del(&cmd->cmd_list); > > + spin_unlock_irqrestore(&xhci->lock, flags); > > + ret = -ETIME; > > + goto command_cleanup; > > + } > > + > > +command_cleanup: > > + xhci_free_command(xhci, cmd); > > + return ret; > > +} > > + > > +/* > > + * Ring device, it rings the all doorbells unconditionally. > > + */ > > +static void xhci_ring_device(struct xhci_hcd *xhci, int slot_id) > > +{ > > + int i; > > + > > + for (i = 0; i < LAST_EP_INDEX + 1; i++) > > + ring_ep_doorbell(xhci, slot_id, i); > > + > > + return; > > +} > > + > > Please only ring the doorbells for endpoints that are actively being > used. Hosts are supposed to ignore endpoints that are disabled, but I > could see some buggy hosts dereferencing NULL dequeue pointers when the > doorbell is rung. OK. > > + 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(). Regards, Libin ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
