On Tue, Jan 04, 2011 at 03:43:24PM -0500, Alan Stern wrote:
> 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.
The trivial fixes from the feedback I got on the other patches I've
already integrated. But I'm not sure what exactly you want fixed for
the design issues.
I think I'm hearing you want some of the shared_hcd references to go
away, with the eventual goal of perhaps getting rid of the pointer out
of the usb_hcd entirely. I agree that dead code like the SAW_IRQ flag
and the check_root_hub_suspended() code should go away, but I don't
think we can get rid of shared_hcd entirely.
You've addressed the use cases in the suspend and resume path, but
there's still an issue with the allocation and freeing. If you agree
that the shared roothub allocation should go in the PCI core because of
the shared resource visibility issue, then the PCI core needs to also
deallocate the shared_hcd. How do you propose to do that when the USB
PCI core doesn't have a reference to the non-primary usb_hcd?
usb_hcd_pci_remove() needs to have a pointer to call usb_put_hcd() with.
Also, in digging around the deallocation path, I found another design
decision I made based on shared_hcd being a part of usb_hcd. Right now,
the shared_hcd pointer is acting like a reference count on the bandwidth
mutex:
/*
* Roothubs that share one PCI device must also share the bandwidth mutex.
* Don't deallocate the bandwidth_mutex until the last shared usb_hcd is
* deallocated.
*
* The first shared roothub that is released will set shared_hcd to NULL. The
* second shared roothub that is released (or a USB host controller driver that
* doesn't set up shared roothubs) will deallocate the bandwidth mutex.
*/
static void hcd_release (struct kref *kref)
{
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
if (hcd->shared_hcd)
hcd->shared_hcd->shared_hcd = NULL;
else
kfree(hcd->bandwidth_mutex);
kfree(hcd);
}
If the USB core has no notion of whether a shared roothub is allocated
(which you seem to be advocating for by making the xHCI driver call into
usb_hcd_pci_probe() and then allocating the shared_hcd), then the USB
core has no idea when to deallocate shared resources.
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
