On Fri, Dec 31, 2010 at 12:38:37PM -0500, Alan Stern wrote:
> On Thu, 30 Dec 2010, Sarah Sharp wrote:
>
> > Introduce the notion of a PCI device that may be associated with more than
> > one USB host controller driver (struct usb_hcd). This patch is the start
> > of the work to separate the xHCI host controller into two roothubs: a USB
> > 3.0 roothub with SuperSpeed-only ports, and a USB 2.0 roothub with
> > HS/FS/LS ports.
> >
> > One usb_hcd structure is designated to be the "primary HCD", and a pointer
> > is added to the usb_hcd structure to keep track of that. A new function
> > call, usb_hcd_is_primary_hcd() is added to check whether the USB hcd is
> > marked as the primary HCD (or if it is not part of a roothub pair). To
> > allow the USB core and xHCI driver to access either roothub in a pair, a
> > "shared_hcd" pointer is added to the usb_hcd structure.
>
> I'm not sure this "shared_hcd" pointer is needed. Does it ever get
> used in a non-trivial way?
I'm not sure what you mean by "non-trivial". The USB core needs it, for
instance, to make sure that both buses are suspended before suspending
the PCI device:
if (hcd->driver->pci_suspend) {
/* Optimization: Don't suspend if a root-hub wakeup is
* pending and it would cause the HCD to wake up anyway.
*/
if (do_wakeup && HCD_WAKEUP_PENDING(hcd))
return -EBUSY;
if (do_wakeup && hcd->shared_hcd &&
HCD_WAKEUP_PENDING(hcd->shared_hcd))
return -EBUSY;
retval = hcd->driver->pci_suspend(hcd, do_wakeup);
Since there's only one usb_hcd stored in the PCI device pointer, there
needs to be a way to access the second usb_hcd. Other functions also
need to use shared_hcd to set various flags that are part of the usb_hcd
state. Do you have a different suggestion as to how to do that?
> > Add a new function, usb_create_shared_hcd(), that does roothub allocation
> > for paired roothubs. It will act just like usb_create_hcd() did if the
> > primary_hcd pointer argument is NULL. If it is passed a non-NULL
> > primary_hcd pointer, it sets usb_hcd->shared_hcd and usb_hcd->primary_hcd
> > fields. It will also skip the bandwidth_mutex allocation, and set the
> > secondary hcd's bandwidth_mutex pointer to the primary HCD's mutex.
> >
> > IRQs are only allocated once for the primary roothub.
> >
> > Introduce a new usb_hcd driver flag that indicates the host controller
> > driver wants to create two roothubs. If the HCD_SHARED flag is set, then
> > the USB core PCI probe methods will allocate a second roothub, and make
> > sure that second roothub gets freed during rmmod and in initialization
> > error paths.
>
> This doesn't seem like the right approach. The creation of a secondary
> usb_hcd should be handled by the driver that needs it; it isn't such a
> common activity that we need to implement it in the USB core. In
> particular, hcd-pci.c shouldn't be changed.
I have thought about your suggestion to make the xHCI driver call
usb_create_shared_hcd() instead of having the USB PCI layer do the
allocation and registering of the second roothub. However, I wanted to
ensure that the shared_hcd pointer was set before the first usb_hcd was
registered, and I don't think I can do that if I push the call into the
xHCI driver.
I need to set the shared_hcd pointer before the first usb_hcd is
registered so that, for example, the PCI device wouldn't get suspended
until the initialization process completed for both roothubs and both
buses are suspended. Things like flags also need to get set for both
host controllers, and I want to make sure the shared_hcd pointer is set
before the bus is used by the USB core at all.
I could potentially allocate the shared HCD in one of the xHCI
initialization routines, before usb_add_hcd() has a chance to call
register_root_hub(). But I'm not sure how the USB core will react to
being called recursively, i.e. usb_register_bus() calls xhci_run() which
calls usb_alloc_shared_hcd() and usb_register_bus(). And what if
someone decides to add a new lock to the USB core functions later? If
they don't have an xHCI device to test with, then they may not notice
some obscure code in the xHCI driver that calls the functions
recursively.
I agree that it's code that's not likely to be used by any other host
controller, but I think this code needs to be visible in the USB core.
> > If the driver that wants to create two roothubs is an xHCI driver, the
> > roothub that is allocated last is marked as the USB 2.0 roothub.
>
> Didn't we agree earlier that xhci-hcd needs to register its USB-2 root
> hub first?
Yes, you're right, that suggestion got lost in the mass of other changes
I had to make since then. But now that there's a usb_hcd->bcdUSB flag
(or usb_hcd->version, once I rename it), it should be trivial to fix.
>
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 79bad2c..a061c31 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
>
> > @@ -2184,13 +2195,49 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
> > "USB Host Controller";
> > return hcd;
> > }
> > +EXPORT_SYMBOL_GPL(usb_create_shared_hcd);
> > +
> > +/**
> > + * usb_create_hcd - create and initialize an HCD structure
> > + * @driver: HC driver that will use this hcd
> > + * @dev: device for this HC, stored in hcd->self.controller
> > + * @bus_name: value to store in hcd->self.bus_name
> > + * @shared_hcd: a pointer to the usb_hcd structure that is sharing the
> > + * PCI device. Used for allocating two usb_hcd structures
> > + * under one xHCI host controller PCI device (a USB 2.0 bus
> > + * and a USB 3.0 bus).
>
> This last argument doesn't really exist, as you can see below.
You're correct, that's leftover from the previous patchset. I'll remove it.
>
> > + * Context: !in_interrupt()
> > + *
> > + * Allocate a struct usb_hcd, with extra space at the end for the
> > + * HC driver's private data. Initialize the generic members of the
> > + * hcd structure.
> > + *
> > + * If memory is unavailable, returns NULL.
> > + */
> > +struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
> > + struct device *dev, const char *bus_name)
> > +{
> > + return usb_create_shared_hcd(driver, dev, bus_name, NULL);
> > +}
> > EXPORT_SYMBOL_GPL(usb_create_hcd);
>
> > @@ -2209,6 +2256,14 @@ void usb_put_hcd (struct usb_hcd *hcd)
> > }
> > EXPORT_SYMBOL_GPL(usb_put_hcd);
> >
> > +inline int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
> > +{
> > + if (!hcd->primary_hcd)
> > + return 1;
> > + return hcd == hcd->primary_hcd;
>
> Do you really need to set hcd->primary_hcd to point to hcd itself? My
> inclination would be to leave it unset.
I do use it in the xHCI driver initialization code, to ensure I'm only
starting the host controller when the second (i.e. non-primary) host
controller is registered. I would have to see if I can just test for
NULL instead, but I feel that it's more symmetrical somehow to set
primary_hcd for both HCDs.
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
