On Thu, 2010-03-04 at 13:59 -0800, Sarah Sharp wrote: > On Thu, Mar 04, 2010 at 06:18:51PM +0800, Andiry Xu wrote: > > >From 61d6ce45d425475c51bff04fe7f8c86be51c94ce Mon Sep 17 00:00:00 2001 > > From: Andiry Xu <andiry.xu@xxxxxxx> > > Date: Thu, 4 Mar 2010 17:52:48 +0800 > > Subject: [PATCH RFC] xHCI command: get port bandwidth > > > > > > Please refer to xHCI spec 4.6.15. > > In general, can all of you explain what's in the xHCI specification > instead of citing it? There's a couple reasons why that's useful: > > - Most Linux developers don't have access to the specification right > now. > - It's highly likely the section numbers will change across revisions. > - Even when developers have the specification, it's much easier to have > a summary of the specification in the commit message, rather than > requiring someone to dig up and download the spec. > - You may be interpreting the specification differently than someone > else. > > Copy-pasting from the spec probably isn't so great, but a summary is > fine. In my commit messages, I try to detail why I'm issuing a command > (including why the USB core or other driver might care about this), why > the hardware would give error codes back, what I do with the response. > See commit ID ac1c1b7 in linus' tree for an example commit message: > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ac1c1b7f16ed287fcec5bcfae06d0165c3941ec3 OK. I'll modify the description. > > This patch implemented the Get Port Bandwidth Command. > > > > The Get Port Bandwidth Command is issued by software to retrieve the > > percentage of Total Bandwidth on each Root Hub Port of the xHC. > > This information can be used by system software to recommend topology > > changes to the user if they were unable to enumerate a device due to > > a Bandwidth Error. > > Do you have plans for how the USB core should use this information? The > user has no idea which port numbers correspond to which physical ports > on their system, so this info is a bit cryptic. Well, I implemented the command for developer at this moment. I wonder if end user really need this information. For example, the xHC controller I used has 4 ports but only 2 physical connectors. An end user will only get confused when he sees 4 ports in log message and only find 2 "ports" on his board. I'll modify the description accordingly. > Also, you allocate a whole DMA pool for the bandwidth contexts. Is that > really necessary? I think you only need one bandwidth context per xHCI > host controller. The only way the USB core can re-assign bandwidth is > through the usb_hcd_alloc_bandwidth() function. That function is > protected by the hcd->bandwidth_mutex, so only one configure endpoint > command will be completing at a time. Thus, you'll only use one > bandwidth context at a time, which is a waste of dma resources. Why not > allocate one with pci_alloc_consistent() and stuff it in xhci_hcd? OK. I'll try that. > I'd like to test this patch, so how did you trigger the bandwidth warning? > Adding a bunch of webcams with isochronous endpoints? I can trigger the bandwidth warning with a webcam(093a:2620). Just plug in and run some cam software, like cheese. Please check dmesg attached. > There's more comments inline. > > > > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx> > > --- > > @@ -1087,6 +1089,34 @@ static int xhci_configure_endpoint_result(struct xhci_hcd *xhci, > > case COMP_BW_ERR: > > dev_warn(&udev->dev, "Not enough bandwidth " > > "for new device state.\n"); > > + /* Get port bandwidth when a Bandwidth Error occurs */ > > + switch (udev->speed) { > > + case USB_SPEED_FULL: > > + xhci_info(xhci, "xHCI: get port bandwidth for " > > + "Full speed device:\n"); > > This should be xhci_dbg instead of xhci_info. The user isn't interested > in what speed the device is operating at. They can figure it out from > the sysfs entries, or earlier log file entries. OK. > > + spin_lock_irqsave(&xhci->lock, flags); > > + ret = xhci_queue_get_port_bw(xhci, port_bw_ctx_ptr, speed); > > + if (ret) { > > + spin_unlock_irqrestore(&xhci->lock, flags); > > + xhci_port_bw_ctx_free(xhci, port_bw_ctx); > > + xhci_dbg(xhci, "get port bandwidth command issue error\n"); > > This error message should say "FIXME allocate a new command ring segment". > There's nothing wrong with the port bandwidth command, it's just that > queue_command() has indicated there's no more room on the ring. OK. > > + return; > > + } > > + xhci_ring_cmd_db(xhci); > > + spin_unlock_irqrestore(&xhci->lock, flags); > > + > > + timeleft = wait_for_completion_interruptible_timeout(&xhci->addr_dev, > > + USB_CTRL_SET_TIMEOUT); > > Please allocate an xhci_command with xhci_alloc_command() instead of using > xhci->addr_dev. I'm trying to make all new commands allocate their own > completions instead of using the shared host controller completion. It will cut > down on future bugs where two commands are submitted that try to use > xhci->addr_dev. See xhci_reset_device() for an example of using > xhci_alloc_command(). OK. Seems I implemented it based on your old solution. I'll use xhci_alloc_command() instead. > > + if (timeleft <= 0) { > > + xhci_warn(xhci, "%s while waiting for get port bw complete\n", > > + timeleft == 0 ? "Timeout" : "Signal"); > > + xhci_port_bw_ctx_free(xhci, port_bw_ctx); > > + return; > > + } > > + > > + for (i = 1; i <= ports; i++) { > > + xhci_info(xhci, "xHCI: port %d bandwidth: 0x%x\n", i, > > + port_bw_ctx->bytes[i]); > > What does this hex number mean to the user? Is it a percentage? I can > find the answer in the spec, but random users won't bother looking it > up. If it is a percentage, you should format the string to make it look > like a percentage. If it's not a percentage, you need to find some way > to present it so the user to understand why they have no bandwidth. > Your message must be more clear to the user to justify the usage of > xhci_info. If it's just for xHCI developers, use xhci_dbg. OK. use percentage instead. I used xhci_info here because it'll not be triggered unless bandwidth error occurs. A warning message(Not enough bandwidth for new device state) is already printed at this moment. Indeed, xhci_dbg is used too often and shows too much unnecessary information. > > + ctx->dma = dma; > > + ctx->ports = ports; > > + size = (1 + ports / 4) * 4; > > Can you add a comment explaining your math? OK.
[ 783.390163] xhci_hcd 0000:02:00.0: Can't reset device (slot ID 1) in enabled/disabled state [ 783.390171] xhci_hcd 0000:02:00.0: Not freeing device rings. [ 783.390196] usb 8-4: new full speed USB device using xhci_hcd and address 0 [ 783.514745] usb 8-4: configuration #1 chosen from 1 choice [ 783.514770] usb 8-4: ep 0x83 - rounding interval to 256 microframes [ 783.514781] usb 8-4: ep 0x4 - rounding interval to 256 microframes [ 783.595647] gspca: main v2.7.0 registered [ 783.615025] gspca: probing 093a:2620 [ 783.639627] gspca: probe ok [ 783.639653] gspca: probing 093a:2620 [ 783.639672] gspca: probing 093a:2620 [ 783.639716] usbcore: registered new interface driver pac7311 [ 783.639790] pac7311: registered [ 783.778583] usbcore: registered new interface driver snd-usb-audio [ 783.846593] cannot submit datapipe for urb 0, error -22: internal error [ 795.590069] usb 8-4: ep 0x83 - rounding interval to 256 microframes [ 795.590082] usb 8-4: ep 0x4 - rounding interval to 256 microframes [ 795.590460] usb 8-4: Not enough bandwidth for new device state. [ 795.590469] xhci_hcd 0000:02:00.0: xHCI: get port bandwidth for Full speed device: [ 795.590595] xhci_hcd 0000:02:00.0: xHCI: port 1 bandwidth: 0% [ 795.590602] xhci_hcd 0000:02:00.0: xHCI: port 2 bandwidth: 0% [ 795.590608] xhci_hcd 0000:02:00.0: xHCI: port 3 bandwidth: 88% [ 795.590613] xhci_hcd 0000:02:00.0: xHCI: port 4 bandwidth: 88% [ 795.590627] usb 8-4: Not enough bandwidth for altsetting 8 [ 795.590632] gspca: set alt 8 err -28
