> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Friday, June 11, 2010 4:35 AM > To: Yang, Libin > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; Xu, Andiry > Subject: Re: [PATCH 1/4 v4] xHCI: port power management implementation > > Hi Libin, > > This code looks good. I have just a few minor comments below. My > biggest concern is why you're using atomic bit operations when I think > holding the xHCI lock will suffice. Thanks for the comments. Please see my comments below. > > + for (i = 0; i < MAX_HC_SLOTS; i++) { > > + if (!xhci->devs[i]) > > + continue; > > + out_ctx = xhci->devs[i]->out_ctx; > > + slot_ctx = xhci_get_slot_ctx(xhci, out_ctx); > > + if (DEVINFO_TO_ROOT_HUB_PORT(slot_ctx->dev_info2) == port) { > > + slot_id = i; > > + break; > > I feel uncomfortable digging the root hub port out of the output > context, since the hardware has to maintain that, and the xHCI driver > doesn't do any checking to make sure the hardware is doing that > properly. Can you add a variable to the xhci_virt_device that holds the > root hub port instead? It is OK. I will do it. > > + * 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. > > This comment is confusing. Does the function require the xhci->lock to > be held by the caller, or does the function just need to take the > xhci->lock. From your code, it looks like the lock is not held when the > function is called. This comment means that in the function, a lock will be held when waiting for the completion like the below code. If you feel it confuse, I will delete the comment. > > + > > + spin_lock_irqsave(&xhci->lock, flags); > > + for (i = LAST_EP_INDEX; i > 0; i--) { > > + if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) > > + xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); > > + } > > + cmd->command_trb = xhci->cmd_ring->enqueue; > > + list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list); > > + xhci_queue_stop_endpoint(xhci, slot_id, 0, 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"); > > Copy-paste error, this isn't a reset device command. I will change it. > > + && (temp & PORT_POWER)) > > + status |= 1 << USB_PORT_FEAT_SUSPEND; > > + if ((temp & PORT_PLS_MASK) == XDEV_U0 > > + && (temp & PORT_POWER) > > + && test_bit(wIndex, &xhci->suspended_ports)) { > > + clear_bit(wIndex, &xhci->suspended_ports); > > + set_bit(wIndex, &xhci->port_c_suspend); > > + } > > This code is under the xhci->lock, so why do you need atomic bit > operations? What's the concern of using the atomic operations under the lock? I don’t see the side effect except that it may cause the performance a little drop. There are some examples in the codes using the atomic operations under lock. For example: ehci_hub_control() in ehci-hub.c, wdm_in_callback() in cdc-wdm.c, snd_ac97_write_cache() in ac97_codec.c > > > switch (wValue) { > > + case USB_PORT_FEAT_SUSPEND: > > + temp = xhci_readl(xhci, addr); > > + /* In spec software should not attempt to suspend > > + * a port unless the port reports that it is in the > > + * enabled (PED = ‘1’,PLS < ‘3’) state. > > + */ > > + if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) > > + || (temp & PORT_PLS_MASK) >= XDEV_U3) { > > + xhci_dbg(xhci, "goto error status = 0x%x\n", > > + temp); > > Could you make this debugging statement a warning and make it more > clear? If this case is triggered, it means there is a bug in the USB > core because it's attempting to suspend a device that is disconnected, > is still enumerating, or otherwise hasn't stabilized into the "active" > U0 state, or a lower link PM state like U1/U2. How about something > like: > > xhci_warn(xhci, "USB core suspending device not in U0/U1/U2.\n"); That makes sense. I will use the xhci_warn instead of xhci_dbg. > > > > -static void ring_ep_doorbell(struct xhci_hcd *xhci, > > +void ring_ep_doorbell(struct xhci_hcd *xhci, > > Please change this name to xhci_ring_ep_doorbell. Since it isn't a > static function anymore, it needs to have the xhci_ prefix. OK. I will change it to xhci_ring_ep_doorbell. Thanks, Libin ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
