On Wed, 20 Jul 2011, Sarah Sharp wrote:
> Here's the updated RFC for the software bandwidth checking quirk for the
> Intel Panther Point xHCI host.
>
> It's stable now, bug free IFAIK, and seems to track bandwidth properly.
> If I don't use a webcam that always requests a 3060 byte payload
> (requiring uvcvideo to always request the most bandwidth-hungry
> alternate interface setting), I can create a setup with an almost
> maxed-out setup where I can view some lower video resolutions, but not
> all of them. (This is with several LS/FS devices attached to two
> multi-TT hubs, one of them being a true 7-port hub.) All the while,
> previously added devices like mice, keyboards, ethernet adapters, and
> USB-to-serial adapters work correctly.
>
> This same setup under EHCI causes issues, with lots of Xact errors. The
As has been pointed out repeatedly, the EHCI periodic scheduling code
is full of bugs. No doubt you are running across some of them.
> USB-to-serial adapters sometime quit while I got a couple frames of
> video (although the keyboard and mice seem rock solid).
>
>
> Alan, I didn't implement using endpoint descriptor pointers. It looked
> good until I started thinking about devices that change descriptors
> across reset. There's an interesting bit in the USB core where it
> copies the descriptor:
>
> static int usb_reset_and_verify_device(struct usb_device *udev)
> {
> struct usb_device *parent_hdev = udev->parent;
> struct usb_hub *parent_hub;
> struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> struct usb_device_descriptor descriptor = udev->descriptor;
Note that this is the device descriptor, not an endpoint descriptor.
> Then the port undergoes normal enumeration sequence:
>
> ret = hub_port_init(parent_hub, udev, port1, i);
Ideally xhci-hcd would preserve its existing internal data structures
across this call, instead of destroying them when the reset happens and
starting over fresh. But either way, things should work without
problems.
> The port gets reset, which should translate into removing all the device
> endpoints after a successful reset device command. The device
> descriptors will get fetched, and we'll set up the device with the first
> configuration,
Old configuration, not first configuration.
> and alt setting 0 for all interfaces. At that point, we
> would grab pointers to the new endpoint descriptors after adding their
> characteristics to the TT or roothub interval table and used bandwidth
> values.
>
> Now, what happens if the descriptors changed after the device reset?
>
> /* Device might have changed firmware (DFU or similar) */
> if (descriptors_changed(udev, &descriptor)) {
> dev_info(&udev->dev, "device firmware changed\n");
> udev->descriptor = descriptor; /* for disconnect() calls */
> goto re_enumerate;
> }
>
> The USB core happily overwrites the new descriptor with the old descriptor
> values, and logically disconnects the device:
>
> hub_port_logical_disconnect(parent_hub, port1);
>
> I think that will eventually translate into usb_disable_device being
> called (although I'm not certain).
Yes it will. It also will prevent any more URBs from being submitted
to the device.
> At that point, the xHCI driver
> should be removing the endpoints that were added during the
> re-enumeration.
At this point in the re-enumeration, the only endpoint to have been
added is ep0. This occurs before the first call to
usb_hcd_alloc_bandwidth().
> However, it will be using the endpoint values from the
> old descriptor, not the changed descriptor!
Overwriting the device descriptor doesn't change ep0's (synthetic)
endpoint descriptor. The "old" descriptor you refer to doesn't contain
any endpoint values, unless you want to count bMaxPacketSize0.
> This would be fine with EHCI, or even with an xHCI host that doesn't
> need software bandwidth checking (since we would just tell the host to
> drop an endpoint without needing to know what the endpoint
> characteristics are). However, it will result in bogus values being
> removed from the interval table for xHCI hosts that need software
> bandwidth checking.
No it won't. None of the periodic endpoint descriptors have been
changed, and while ep0's descriptor may have been changed by
hub_port_init(), the current value in the descriptor is indeed correct.
(However I doubt whether the XHCI bandwidth code cares about maxpacket
values for async endpoints. It shouldn't.)
> I'm not certain exactly how to fix that issue, and given that the USB
> core does something like overwrite the descriptor values here, I'd
> rather treat descriptors as private to the USB core, and not use the
> endpoint descriptor pointers.
That's up to you. But this is not a genuine problem.
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
