On Fri, 11 Mar 2011, Sarah Sharp wrote:
> On Tue, Mar 08, 2011 at 11:40:33AM -0500, Alan Stern wrote:
> > On Mon, 7 Mar 2011, Sarah Sharp wrote:
> > > The xHCI driver will need to ensure that HC_STATE_HALT, HC_STATE_RUNNING,
> > > and HC_STATE_QUIESCING will be set for both the roothubs.
> >
> > Fortunately this is no longer necessary, unless xhci-hcd uses
> > hcd->state internally.
>
> Alan, I tried to remove all references to HC_STATE_HALT and
> HC_STATE_RUNNING in the xHCI driver, since it only sets them and never
> tests them.
>
> The problem is, the roothub allocation fails in register_root_hub().
> HC_STATE_HALT is defined to be zero, and the USB core never sets
> HC_STATE_RUNNING in the roothub allocation function, so if the xHCI
> driver never sets HC_STATE_RUNNING itself, the roothub allocation will
> fail here because of the second conditional in the if statement:
>
> static int register_root_hub(struct usb_hcd *hcd)
> {
> ...
> if (retval == 0) {
> spin_lock_irq (&hcd_root_hub_lock);
> hcd->rh_registered = 1;
> spin_unlock_irq (&hcd_root_hub_lock);
>
> /* Did the HC die before the root hub was registered? */
> if (HCD_DEAD(hcd) || hcd->state == HC_STATE_HALT)
> usb_hc_died (hcd); /* This time clean up */
> }
>
> I really want to move the xHCI driver away from having to set
> hcd->state. Can you rework your hcd->state patch to accommodate drivers
> that never touch it?
I thought about this while writing the patch. In the end there were
two places where usbcore still had to use hcd->state. One is the place
you mention above, in register_root_hub(), and the other is in
usb_hcd_irq().
The reason these couldn't be removed is that host controller drivers
can use them as an implicit way of telling the core that a controller
has died. (I know for a fact that uhci-hcd does so.) Undoubtedly this
was not a good design, but until we audit the HCDs there's nothing to
be done about it.
For your purposes, we can work around the problem easily enough. The
one-line patch below ought to get you going without messing anything
else up. Perhaps it should have been included with the original
patch... but there's nothing to stop us from adding it now.
Alan Stern
drivers/usb/core/hcd.c | 1 +
1 file changed, 1 insertion(+)
Index: usb-2.6/drivers/usb/core/hcd.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd.c
+++ usb-2.6/drivers/usb/core/hcd.c
@@ -2327,6 +2327,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
(unsigned long long)hcd->rsrc_start);
}
+ hcd->state = HC_STATE_RUNNING;
if ((retval = hcd->driver->start(hcd)) < 0) {
dev_err(hcd->self.controller, "startup error %d\n", retval);
goto err_hcd_driver_start;
--
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
