On Thu, 2 Dec 2010, Sarah Sharp wrote:
> > > Should those be shared across
> > > the two roothubs?
> >
> > That's up to you.
>
> I really only made the usb_hcds share fields that I understood how they
> were used. If you don't think it's a big deal for the usb_hcds to have
> mostly independently allocated resources, I'm not inclined to make them
> share things like DMA pools.
Either way is okay with me. But obviously things like mmio ranges and
IRQ lines have to be shared.
> > I suppose instead of HCD_NO_RESOURCES you could add a "primary_hcd"
> > pointer to the usb_hcd structure. Normally it would be NULL, but for
> > secondary hcd's (where you don't want resources allocated) you set it
> > to point to the associated primary hcd. Then whenever this pointer is
> > set, instead of allocating a new set of DMA pools the core could just
> > copy the pool pointers from the primary hcd.
>
> I already added a "shared_hcd" pointer in the usb_hcd structure. That's needed
> because the xHCI driver needs to let the USB core know when both usb_hcds die,
> or to figure out which bus to kick khubd for when a port status change comes
> in. I already have a "main_hcd" field in the xhci_hcd structure that points to
> the USB 3.0 usb_hcd. I need that because I can't use containerof to get the
> usb_hcd from the xhci_hcd, since the hcd->priv is now a pointer, not a
> statically allocated structure. I could mirror that field into a "primary_hcd"
> field in usb_hcd, but it seems redundant.
Here's what I would do: In xhci_hcd, store pointers to both the USB-2
hcd and the USB-3 hcd. Decide which of those two will be the primary
hcd (presumably the one that gets registered first, which means it has
to be the USB-2 hcd) and set the primary_hcd pointer in the other
accordingly.
> > The advantage over what you posted is that this doesn't make
> > assumptions about dev_get_drvdata(hcd->self.controller); the core
> > shouldn't have anything to do with this -- it's private to the driver.
> > (Some drivers might not want to store a pointer to the hcd there.)
>
> Since the USB core set that data in the PCI probe function, I thought it
> was private to the USB core. But if other non-PCI drivers can set it to
> something else, then yes, the usb_hcd_is_main_hcd() function shouldn't
> check dev_get_drvdata(hcd->self.controller). In that case, it should
> either check the primary_hcd field or the bcdUSB field you suggested.
For purposes of deciding whether to allocate resources, use the
primary_hcd field. For purposes of deciding the type of bus, use the
bcdUSB field. In fact, when copying the the device descriptor to a
buffer, you could fill in the low-order byte of the bcdUSB field from
the hcd itself instead of using the static value in hcd.c.
> I'm leaning towards the bcdUSB field, so I don't have the redundant
> primary_hcd field. Then usb_hcd_is_main_hcd() would look something like
> this:
>
> inline int usb_hcd_is_main_hcd(struct usb_hcd *hcd)
> {
> if ((hcd->driver->flags & HCD_MASK) == HCD_USB3 &&
> hcd->bcdUSB == HCD_USB2)
> return 0;
> return 1;
> }
> EXPORT_SYMBOL_GPL(usb_hcd_is_main_hcd);
But what happens if you change your mind and decide to make the USB-3
hcd the primary one? Or what if someone else wants to write a
different driver and do it the other way around? Don't hard-code
things like this; use the primary_hcd field. That's what I invented it
for.
> bcdUSB could be originally set to xhci->driver->flags & HCD_MASK, and
> then could be over-written in the usb_hcd_pci_probe() function after the
> second usb_hcd function is created.
>
> > It also doesn't make assumptions about whether the primary is USB-2 and
> > the secondary is USB-3 or vice versa.
>
> The USB core really has to know both whether the usb_hcd is the primary
> usb_hcd, and whether the usb_hcd is the USB2 or USB3 roothub.
>
> Of the 6 calls to usb_hcd_is_main_hcd() in the core, 3 are used to
> determine the type of roothub (for figuring out which device descriptors
> or configuration descriptors to return for the roothub, and setting the
> roothub speed in hcd.c). Three callers use usb_hcd_is_main_hcd() to
> decide whether to enable/free IRQs.
Since you have two different kinds of decision to make, you should add
the two new fields.
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
