On Thu, Aug 06, 2020 at 02:21:50PM +0200, Hans de Goede wrote: > 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. That's fine, duplication should not cause a problem, and really, we could leave the list in the kernel for "forever" and just add a comment that says "do not add to this!". thanks, greg k-h
