Alan,
I feel like your need to make this patchset "perfect" is getting in the
way of merging perfectly "good" code. I feel like no matter how many
RFCs I make, you'll still come with some optimization that needs to be
done, because the USB core is your territory. Instead, can we please
submit the code as-is for 2.6.38 and try to optimize it later? If we
end up ripping out all the references to shared_hcd later, that's fine,
because we don't guarantee a stable kernel API.
On Tue, Jan 04, 2011 at 11:22:30AM -0500, Alan Stern wrote:
> 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).
I don't feel like I have enough experience with the USB core and those
flags to make that sort of patch. Perhaps you could make that patch?
> Be more specific. Why exactly do you think you need this?
There's also the calls in check_root_hub_suspended() and resume_common()
to check the HCD state. Are those also just an optimization?
> 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.
I think this falls into the zero, one, many category. The patchset
changes the USB core to accept the fact that there can be more than one
bus per controller. It should be easy to extend it to "many" buses
after that.
> 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.
As we discussed before, that approach would open up a potential can of
worms if someone decided to add a new resource, like the bandwidth
mutex, that would need to be shared across both roothubs.
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
