On Mon, 3 Jan 2011, Sarah Sharp wrote:
> > 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);
We shouldn't have to change this. It's not necessary to check that the
root hubs are suspended; the PM core is now smart enough never to
suspend a controller if the root hubs aren't already suspended. It
isn't even really necessary to avoid races between suspending the
controller and remote wakeup of the root hubs; the code you see above
is merely an optimization. It could be removed (all but the first and
last lines).
Regardless, I'm pretty sure that having a "shared_hcd" pointer isn't
the way to go. Originally we assumed that one controller = one bus.
xHCI has broken that assumption; it seems awfully foolish now to make
the assumption that one controller = one or two buses.
One possibility would be to allow each hcd to have a list of buses.
But I think this just adds unnecessary complexity. In addition, the
dividing line between a controller and a bus isn't clear. For example,
xHCI uses a single IRQ line to report events on either of the buses,
but it's easy to imagine a controller using a different IRQ for each
bus.
> 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?
What other functions? My suggestion is that we figure out a way to
change them so that they don't need to look at the secondary hcd or to
remove them entirely.
> 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.
You don't have to worry about it if there is no shared_hcd pointer.
:-)
> 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.
That can't happen; devices are never suspended while being probed.
> 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.
Be more specific. Why exactly do you think you need this?
> 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.
I still think you should be able to call usb_hcd_pci_probe for the
primary hcd, let it be registered and started, and then call
usb_add_shared_hcd for the other one.
> > > +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.
It's not a big deal. Doing it this way just means you have to make two
tests instead of only one.
Alan Stern
--
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
