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

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

[Hans de Goede]

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?

Regards,

Hans

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

[Greg Kroah-Hartman]

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'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.

thanks,

greg k-h

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

[Hans de Goede]

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

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

[Greg Kroah-Hartman]

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