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 > 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. 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? I'd like to test this patch, so how did you trigger the bandwidth warning? Adding a bunch of webcams with isochronous endpoints? There's more comments inline. > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx> > --- > drivers/usb/host/xhci-hcd.c | 79 ++++++++++++++++++++++++++++++++++++++++++ > drivers/usb/host/xhci-mem.c | 61 +++++++++++++++++++++++++++++++- > drivers/usb/host/xhci-ring.c | 13 +++++++ > drivers/usb/host/xhci.h | 20 ++++++++++ > 4 files changed, 171 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/xhci-hcd.c b/drivers/usb/host/xhci-hcd.c > index 4cb69e0..93e896f 100644 > --- a/drivers/usb/host/xhci-hcd.c > +++ b/drivers/usb/host/xhci-hcd.c > @@ -1072,6 +1072,8 @@ static void xhci_zero_in_ctx(struct xhci_hcd *xhci, struct xhci_virt_device *vir > } > } > > +static void xhci_get_port_bandwidth(struct xhci_hcd *xhci, int speed); > + > static int xhci_configure_endpoint_result(struct xhci_hcd *xhci, > struct usb_device *udev, int *cmd_status) > { > @@ -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. > + xhci_get_port_bandwidth(xhci, 1); > + break; > + case USB_SPEED_LOW: > + xhci_info(xhci, "xHCI: get port bandwidth for " > + "Low speed device:\n"); Same here. > + xhci_get_port_bandwidth(xhci, 2); > + break; > + case USB_SPEED_HIGH: > + xhci_info(xhci, "xHCI: get port bandwidth for " > + "High speed device:\n"); And here. > + xhci_get_port_bandwidth(xhci, 3); > + break; > + case USB_SPEED_SUPER: > + xhci_info(xhci, "xHCI: get port bandwidth for " > + "Super speed device:\n"); Should be "SuperSpeed". I don't like CamelCase, but let's be consistent with the USB 3.0 bus spec. > + xhci_get_port_bandwidth(xhci, 4); > + break; > + default: > + /* Unknown speed? */ > + xhci_info(xhci, "xHCI: cannot get port bandwidth for " > + "unknown speed device.\n"); > + break; > + } > ret = -ENOSPC; > /* FIXME: can we go back to the old state? */ > break; > @@ -1869,6 +1899,55 @@ int xhci_get_frame(struct usb_hcd *hcd) > return xhci_readl(xhci, &xhci->run_regs->microframe_index) >> 3; > } > > +/* > + * Get port bandwidth information. > + */ > +static void xhci_get_port_bandwidth(struct xhci_hcd *xhci, int speed) > +{ > + struct xhci_port_bw_ctx *port_bw_ctx; > + unsigned long flags = GFP_KERNEL; > + dma_addr_t port_bw_ctx_ptr; > + int timeleft; > + int ret, i; > + int ports; > + > + port_bw_ctx = xhci_port_bw_ctx_alloc(xhci, flags); > + if (!port_bw_ctx) { > + xhci_dbg(xhci, "failed to allocate a port bandwidth context\n"); > + return; > + } > + > + port_bw_ctx_ptr = port_bw_ctx->dma; > + ports = port_bw_ctx->ports; > + > + 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. > + 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(). > + 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. > + } > + > + xhci_port_bw_ctx_free(xhci, port_bw_ctx); > +} > + > MODULE_DESCRIPTION(DRIVER_DESC); > MODULE_AUTHOR(DRIVER_AUTHOR); > MODULE_LICENSE("GPL"); > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 8045bc6..fc2d370 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -72,6 +72,52 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg) > kfree(seg); > } > > +struct xhci_port_bw_ctx *xhci_port_bw_ctx_alloc(struct xhci_hcd *xhci, > + gfp_t flags) > +{ > + struct xhci_port_bw_ctx *ctx; > + dma_addr_t dma; > + int ports, size; > + > + ctx = kzalloc(sizeof(*ctx), flags); > + if (!ctx) > + return NULL; > + > + ctx->bytes = dma_pool_alloc(xhci->port_bw_pool, flags, &dma); > + if (!ctx->bytes) { > + kfree(ctx); > + return NULL; > + } > + > + ports = HCS_MAX_PORTS(xhci->hcs_params1); > + if (ports <= 0) { > + xhci_dbg(xhci, "ERROR: no ports?\n"); > + return NULL; > + } > + ctx->dma = dma; > + ctx->ports = ports; > + size = (1 + ports / 4) * 4; Can you add a comment explaining your math? > + > + memset(ctx->bytes, 0, size); > + > + return ctx; > +} > + > +void xhci_port_bw_ctx_free(struct xhci_hcd *xhci, struct xhci_port_bw_ctx *ctx) > +{ > + if (!ctx) > + return; > + if (ctx->bytes) { > + xhci_dbg(xhci, "Freeing DMA segment at %p (virtual) 0x%llx" > + " (DMA)\n", > + ctx->bytes, (unsigned long long)ctx->dma); > + dma_pool_free(xhci->port_bw_pool, ctx->bytes, ctx->dma); > + ctx->bytes = NULL; > + } > + xhci_dbg(xhci, "Freeing priv port bw context structure at %p\n", ctx); > + kfree(ctx); > +} > + > /* > * Make the prev segment point to the next segment. > * > @@ -932,6 +978,11 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) > xhci->device_pool = NULL; > xhci_dbg(xhci, "Freed device context pool\n"); > > + if (xhci->port_bw_pool) > + dma_pool_destroy(xhci->port_bw_pool); > + xhci->port_bw_pool = NULL; > + xhci_dbg(xhci, "Freed port bandwidth context pool\n"); > + > xhci_write_64(xhci, 0, &xhci->op_regs->dcbaa_ptr); > if (xhci->dcbaa) > pci_free_consistent(pdev, sizeof(*xhci->dcbaa), > @@ -1108,7 +1159,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > u64 val_64; > struct xhci_segment *seg; > u32 page_size; > - int i; > + int i, ports, size; > > page_size = xhci_readl(xhci, &xhci->op_regs->page_size); > xhci_dbg(xhci, "Supported page size register = 0x%x\n", page_size); > @@ -1165,7 +1216,13 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > /* See Table 46 and Note on Figure 55 */ > xhci->device_pool = dma_pool_create("xHCI input/output contexts", dev, > 2112, 64, xhci->page_size); > - if (!xhci->segment_pool || !xhci->device_pool) > + > + ports = HCS_MAX_PORTS(xhci->hcs_params1); > + size = (1 + ports / 4) * 4; > + xhci->port_bw_pool = dma_pool_create("xHCI port bandwidth contexts", > + dev, size, 32, xhci->page_size); > + > + if (!xhci->segment_pool || !xhci->device_pool || !xhci->port_bw_pool) > goto fail; > > /* Set up the command ring to have one segments for now. */ > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 6ba841b..2c12a7e 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -950,6 +950,9 @@ bandwidth_change: > case TRB_TYPE(TRB_CMD_NOOP): > ++xhci->noops_handled; > break; > + case TRB_TYPE(TRB_GET_BW): > + complete(&xhci->addr_dev); > + break; > case TRB_TYPE(TRB_RESET_EP): > handle_reset_ep_completion(xhci, event, xhci->cmd_ring->dequeue); > break; > @@ -2273,3 +2276,13 @@ int xhci_queue_reset_ep(struct xhci_hcd *xhci, int slot_id, > return queue_command(xhci, 0, 0, 0, trb_slot_id | trb_ep_index | type, > false); > } > + > +/* Queue an get port bandwidth command TRB */ > +int xhci_queue_get_port_bw(struct xhci_hcd *xhci, dma_addr_t port_bw_ctx_ptr, > + int dev_speed) > +{ > + return queue_command(xhci, lower_32_bits(port_bw_ctx_ptr), > + upper_32_bits(port_bw_ctx_ptr), 0, > + TRB_TYPE(TRB_GET_BW) | DEV_SPEED_FOR_TRB(dev_speed), > + false); > +} > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index e5eb09b..936cc7d 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -487,6 +487,16 @@ struct xhci_slot_ctx { > u32 reserved[4]; > }; > > +/* Port Bandwidth Context > + * xHCI Spec 6.2.6 > + */ > +struct xhci_port_bw_ctx { > + int ports; > + dma_addr_t dma; > + > + u8 *bytes; > +}; > + > /* dev_info bitmasks */ > /* Route String - 0:19 */ > #define ROUTE_STRING_MASK (0xfffff) > @@ -829,6 +839,10 @@ struct xhci_event_cmd { > /* Port ID - bits 31:24 */ > #define GET_PORT_ID(p) (((p) & (0xff << 24)) >> 24) > > +/* Get Port Bandwidth Command TRB fields */ > +/* Dev Speed - bits 19:16 */ > +#define DEV_SPEED_FOR_TRB(p) (((p) & 0x0f) << 16) > + > /* Normal TRB fields */ > /* transfer_len bitmasks - bits 0:16 */ > #define TRB_LEN(p) ((p) & 0x1ffff) > @@ -1084,6 +1098,7 @@ struct xhci_hcd { > /* DMA pools */ > struct dma_pool *device_pool; > struct dma_pool *segment_pool; > + struct dma_pool *port_bw_pool; > > #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING > /* Poll the rings - for debugging */ > @@ -1243,6 +1258,9 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci, > gfp_t mem_flags); > void xhci_free_command(struct xhci_hcd *xhci, > struct xhci_command *command); > +struct xhci_port_bw_ctx *xhci_port_bw_ctx_alloc(struct xhci_hcd *xhci, > + gfp_t flags); > +void xhci_port_bw_ctx_free(struct xhci_hcd *xhci, struct xhci_port_bw_ctx *ctx); > > #ifdef CONFIG_PCI > /* xHCI PCI glue */ > @@ -1299,6 +1317,8 @@ int xhci_queue_configure_endpoint(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr, > u32 slot_id, bool command_must_succeed); > int xhci_queue_evaluate_context(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr, > u32 slot_id); > +int xhci_queue_get_port_bw(struct xhci_hcd *xhci, dma_addr_t port_bw_ctx_ptr, > + int dev_speed); > int xhci_queue_reset_ep(struct xhci_hcd *xhci, int slot_id, > unsigned int ep_index); > int xhci_queue_reset_device(struct xhci_hcd *xhci, u32 slot_id); > -- > 1.6.3.3 > > > -- 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
