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
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;
Then the port undergoes normal enumeration sequence:
ret = hub_port_init(parent_hub, udev, port1, i);
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, 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). At that point, the xHCI driver
should be removing the endpoints that were added during the
re-enumeration. However, it will be using the endpoint values from the
old descriptor, not the changed descriptor!
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.
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.
Changes and bug fixes include:
- Fix a bug when inserting an endpoint with the smallest max packet
size in the endpoint list. Thanks Alan!
- Update the various list walking code to use list_for_each_entry since
Alan pointed out it's safe on an empty list.
- use GFP_ATOMIC while holding the xHCI spinlock with IRQs disabled.
- various fixes because xhci_virt_dev->real_port is one-based, but the
roothub port array (rh_bw) is zero-based
- cleaning up return codes to match kernel style (return 0 for success,
or a negative value for error)
- when tracking TTs, store the TT hub's slot ID, not a pointer to the
USB core's usb_tt structure. We need to do this because the xHCI
driver has to free the TT after the USB device slot is disabled, and
the usb_device and usb_tt has already been deallocated.
- Double check the real_port number is non-zero when freeing a TT when
a device is freed. It may not be set if the Enable Slot or Set
Address command fails.
- Fix a bug where the code would delete one more TT from the tt_list
than necessary.
- Make sure to only match the slot ID, not the ttport when a new LS/FS
device is added under a single TT hub. There is only one entry for
single TT hubs in the tt_list, so the port the device is under
doesn't matter.
- Don't remove endpoints when a disable slot command completes.
Instead, rely on usb_disable_device to remove them before
deallocating the device.
- Only update the stored endpoint information using the input context
after the function has removed all endpoints from the interval to be
dropped. Otherwise we leave dropped endpoints in endpoint lists they
shouldn't be in.
- Don't count interval 0 (1 microframe for HS devices or 1 frame for
LS/FS devices) packets twice. The code was adding in the max esit
payload to the bandwith, and then the for loop was counting interval
zero again.
Sarah Sharp (10):
Trivial: xhci: Fix copy-paste error.
xhci: If no endpoints changed, don't issue BW command.
xhci: Rename virt_dev->port to fake_port.
xhci: Refactor endpoint limit checking.
xhci: Store the "real" root port number.
xhci: Store information about roothubs and TTs.
xhci: Store endpoint bandwidth information.
xhci: Track interval bandwidth tables per port/TT.
xhci: Implement HS/FS/LS bandwidth checking.
xhci: Add software BW checking quirk to Intel PPT xHCI
drivers/usb/host/xhci-hub.c | 2 +-
drivers/usb/host/xhci-mem.c | 224 +++++++++++++++++-
drivers/usb/host/xhci-pci.c | 1 +
drivers/usb/host/xhci.c | 567 +++++++++++++++++++++++++++++++++++++++++--
drivers/usb/host/xhci.h | 147 +++++++++++-
5 files changed, 915 insertions(+), 26 deletions(-)
--
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
