On Wed, 28 Nov 2012, Sarah Sharp wrote:
> On Wed, Nov 28, 2012 at 12:39:14PM -0500, Alan Stern wrote:
> > On Sat, 17 Nov 2012, Lan Tianyu wrote:
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -1546,6 +1546,25 @@ static int hub_configure(struct usb_hub *hub,
> > > dev_err(hub->intfdev,
> > > "couldn't create port%d device.\n", i + 1);
> > >
> > > + if (!hub_is_superspeed(hdev)) {
> > > + for (i = 1; i <= hdev->maxchild; i++)
> > > + if (hub->ports[i - 1]->connect_type
> > > + == USB_PORT_CONNECT_TYPE_HARD_WIRED)
> > > + hub->descriptor->u.hs.DeviceRemovable[i/8]
> > > + |= 1 << (i%8);
> > > + } else {
> > > + u16 port_removable =
> > > + le16_to_cpu(hub->descriptor->u.ss.DeviceRemovable);
> > > +
> > > + for (i = 1; i <= hdev->maxchild; i++)
> > > + if (hub->ports[i - 1]->connect_type
> > > + == USB_PORT_CONNECT_TYPE_HARD_WIRED)
> > > + port_removable |= 1 << i;
> > > +
> > > + hub->descriptor->u.ss.DeviceRemovable =
> > > + cpu_to_le16(port_removable);
> > > + }
> > > +
> >
> > You've got all this code here, and you added copies of the same thing
> > to ehci-hcd and xhci-hcd. That's wasteful, and it also ignores the
> > other HCDs.
> >
> > Instead, how about sticking the new code into a separate function:
> >
> > void hub_adjust_DeviceRemovable(struct usb_device *hdev,
> > struct usb_hub_descriptor *desc);
> >
> > Call this new function here and at the appropriate place in
> > hcd.c:rh_call_control(). Then no modifications would be needed in
> > ehci-hcd or xhci-hcd.
>
> So you would basically let EHCI and xHCI set the DeviceRemovable bits in
> their hub descriptors and then have the USB core overwrite them?
It's not an overwrite, it's an "|=". Anyway, that's what Tianyu's
patches already do; I'm merely suggesting that he rearrange the code to
avoid duplication.
> If a
> userspace program like lsusb asks for the hub descriptors, would they
> see the updated version, or the original version?
With either Tianyu's original series or my suggested revision, the
program would see the updated version for root hubs and the original
version for non-root hubs.
> The shared code to overwrite the bits should probably print a warning if
> the host and ACPI bits differ.
I'm not so sure about that. For one thing, who wants warnings to be
logged every time they run "lsusb -v"?
Also, this sort of thing might be more common than you might expect.
I'd guess that if the ACPI information contains anything useful at all,
it will be different from the [Ex]HCI information.
Tianyu's patches log such warnings for xHCI but not for EHCI. That
inconsistency is another reason to rework them.
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
