Re: XHCI-PCI: existing allowlist for enabling auto-suspend/runtime-pm in the kernel vs a userspace allowlist ?

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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