On Tue, 4 Jan 2011, Sarah Sharp wrote:
> 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.
I'm sorry you got that impression. It wasn't deliberate. Large
changes (and this is not a small one) often provoke many rounds of
back-and-forth argumentation before they are accepted.
And anyway, there shouldn't be too much farther to go. You're down to
solving the last few remaining issues.
> 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?
In fact nothing needs to be changed here. We can keep this code as is;
it will detect races with the primary bus but not with the secondary
bus. That's okay; if the secondary bus has a pending wakeup request
then the controller will be resumed again right after it is suspended.
> > 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?
No, they are the snows of yesteryear -- you can sort of tell from
reading the comment in suspend_common, where it talks about
power/state. check_root_hub_suspended isn't needed any more at all; it
ought to be safe to remove all references to it. Would you like me to
do this?
The check in resume_common is a little more complicated. The idea here
is that we don't want to try to resume a host controller after it has
died. That test should be okay the way it is, right?
> > 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.
Okay, I'll buy this. However it may still turn out that shared_hcd
isn't needed.
> > 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.
Maybe. I might feel better about it if you replaced shared_hcd with a
list_head, so we could accomodate an arbitrary number of buses. But
that can be done later.
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
