Hi, On 8/6/20 2:12 PM, Greg Kroah-Hartman wrote:
On Thu, Aug 06, 2020 at 01:55:55PM +0200, Hans de Goede wrote:Hi All, ATM the kernel has a allowlist (coded as a big if) for XHCI-PCI controllers on which to enable auto-suspend. This seems to consist solely of XHCI controllers embedded inside Intel Thunderbolt controllers: if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_ICE_LAKE_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_TIGER_LAKE_XHCI)) xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; I noticed this because it seems that the product-id for the TB controller in the Lenovo T14 gen 1 is missing: 8086:15c1 At the same time we also have a more generic allowlist for enabling auto-suspend/runtime-pm in userspace: https://github.com/systemd/systemd/blob/master/hwdb.d/60-autosuspend.hwdb ATM this only contains USB device ids, but there also is a second hwdb file, auto-generated baed on info from ChromeOS (to avoid having to duplicate this info): https://github.com/systemd/systemd/blob/master/tools/make-autosuspend-rules.py https://github.com/systemd/systemd/blob/master/tools/chromiumos/gen_autosuspend_rules.py As you can see this second file already contains a whole bunch of (mostly Intel vendor) PCI vid:pid pairs and udev will enable runtime-pm on these based on the auto generated hwdb file. To me it seems better for future XHCI controllers on which we want to auto-enable runtime-pm, such as the missing 8086:15c1 model in userspace, together with the allowlist for runtime-pm on other PCI devices in userspace, rather then to add yet another quirk for this to to xhci-pci.c code. The downside would be that this is somewhat inconsistent with how we have done this before, still I believe that it would be better / easier to maintain this in userspace (hwdb) in the future. So I was wondering what other people think about this?Whatever we do, it should all be done in just one place to unify it all please.
I agree. But we are going to be stuck (at least for a while) with the current allowlist (hidden as a big if) in the kernel to avoid regressions. We could duplicate that list in hwdb now and then after some to be decided time period we could consider removing the big if from the xhci-pci.c code.
I'd vote for the hwdb location, as all vendors contribute to it and it's easier to keep up to date than manually changing the kernel all the time when a new device is released.
Using hwdb has my preference too. Regards, Hans
