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

[Hans de Goede <hdegoede@redhat.com>]

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