On Sat, Apr 14, 2012 at 02:54:30AM +0800, Andiry Xu wrote:
> This commit adds a bit-array to xhci bus_state for keeping track of
> which ports are undergoing a resume transition. If any of the bits
> are set when xhci_hub_status_data() is called, the routine will return
> a non-zero value even if no ports have any status changes pending.
> This will allow usbcore to handle races between root-hub suspend and
> port wakeup.
Hi Andiry,
Thanks for doing this! The patch looks fine, but I have one question:
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1362,6 +1362,8 @@ struct xhci_bus_state {
> u32 suspended_ports;
> u32 port_remote_wakeup;
> unsigned long resume_done[USB_MAXCHILDREN];
> + /* which ports have started to resume */
> + unsigned long resuming_ports;
Is there a particular reason why you chose unsigned long for this when
the other two bitmasks for port suspend states (suspended_ports and
port_remote_wakeup) are u32?
I think the USB core limits the number of max ports on a hub to 31, so there's
no sense using an unsigned long. Just use u32. Then if we find an xHCI host
with more than 31 ports and we have to expand the USB core USB_MAXCHILDREN
variable, we can easily search for u32 bit fields.
We should probably add a warning to the xhci-mem.c:xhci_mem_init() and
fail if we find there's more than 31 ports. We print a debug message,
but that won't show up unless CONFIG_USB_XHCI_HCD_DEBUGGING is turned
on:
static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
{
...
if (xhci->num_usb2_ports > USB_MAXCHILDREN) {
xhci_dbg(xhci, "Limiting USB 2.0 roothub ports to %u.\n",
USB_MAXCHILDREN);
xhci->num_usb2_ports = USB_MAXCHILDREN;
}
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
