On Tue, Jan 14, 2014 at 02:43:40PM -0500, Alan Stern wrote: > On Tue, 14 Jan 2014, Sarah Sharp wrote: > > > Alan, I believe your analysis of the code was incorrect when you asked > > Xenia to create commit 60e102ac73cd40069d077014c93c86dc7205cb68 in > > August: > > > > http://marc.info/?l=linux-usb&m=137780837109921&w=2 > > > > You asked her to set lpm_capable because you thought it simply wasn't > > being set at all. You overlooked the xHCI driver setting lpm_capable in > > drivers/usb/host/xhci-pci.c. > > So I did. > > > The xHCI driver only sets lpm_capable for xHCI host controllers that it > > knows how to calculate the U1/U2 timeout values for. Since those > > timeout values are highly dependent on how the xHCI host hardware > > scheduling works, I've only enabled USB 3.0 Link PM for Intel hosts: > > > > static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > > { > > ... > > if (pdev->vendor == PCI_VENDOR_ID_INTEL) { > > xhci->quirks |= XHCI_LPM_SUPPORT; > > xhci->quirks |= XHCI_INTEL_HOST; > > } > > > > static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) > > { > > ... > > /* We know the LPM timeout algorithms for this host, let the USB core > > * enable and disable LPM for devices under the USB 3.0 roothub. > > */ > > if (xhci->quirks & XHCI_LPM_SUPPORT) > > hcd_to_bus(xhci->shared_hcd)->root_hub->lpm_capable = 1; > > > > lpm_capable shouldn't be set for hosts where we don't know how to > > calculate the U1/U2 timeout values, so commit > > 60e102ac73cd40069d077014c93c86dc7205cb68 should just be reverted. > > Allow me to propose an alternative. > > Let's do our best to treat root hubs the same as external hubs. This > means that udev->lpm_capable should be set the same way in > hub_port_init() and register_root_hub(): by calling > usb_device_supports_lpm(), which gets the information from the BOS > extended capabilities. > > In particular, xhci-hcd shouldn't go messing around with the innards of > the root hub structure. Instead of setting lpm_capable when it knows > how to calculate the timeout values, it should pass the necessary > information through the ss_cap descriptor, just like an external hub > would. Either don't provide that descriptor at all if you don't know > the timeout values, or else set those fields in the descriptor to > something invalid (and make usb_device_supports_lpm() test for it). > > And while we're at it, let's change the comment in > usb_device_supports_lpm() that says "All USB 3.0 must support LPM". > Not only is it ungrammatical, but as you point out, it is wrong for > some root hubs. I agree your alternative is much cleaner, and I agree we should try to treat roothubs like external hubs. I've made a patch (to be sent shortly) that fixes this. However, I would like to see the original bad commit ("usbcore: set lpm_capable field for LPM capable root hubs") reverted from the stable kernels, since it's been backported to 3.10, 3.11, and 3.12. I would rather go for the simple revert, since that's less likely to cause issues than new code. 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
