On Fri, Jun 11, 2010 at 11:20:23AM +0800, Yang, Libin wrote: > > -----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. > > > > + * 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. Yes, it's probably better to delete it. Or just change it to something like "This function will acquire and drop the xhci->lock" Maybe the original comment meant "Acquire" instead of "Require"? Sorry for the nit-picking, I just want people to know what the lock behavior is. > > > + > > > + 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 It could cause a performance drop, and I think it means the caches have to be synchronized across multiple processors. That could be an issue when xHCI supports MSI-X. So atomic operations are expensive and should be avoided unless absolutely necessarily. I think the EHCI case is similar to this one, but the other examples could be manipulating the atomic under a lock in one section of the code, and not holding the lock while manipulating the atomic in another section of code. Basically, just because other drivers are inefficient doesn't mean the xHCI driver should be inefficient. > > > -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 for your help! I looked over the remote wake patch and didn't see any issues with it. I'll try to get to the other two patches soon. 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
