Re: [PATCH v5 1/2] usb: xhci-pci: add AMD Promontory 21 PCI glue

[Jihong Min <hurryman2212@icloud.com>]

On 5/14/26 10:20, Guenter Roeck wrote:
> On 5/13/26 17:40, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential 
>> issue(s) to consider:
>> - [High] The Kconfig dependency `depends on USB_XHCI_PCI=y` 
>> completely disables PROM21 support for systems where the core driver 
>> is built as a module.
>> - [Low] Signed integer overflow when calculating the auxiliary device 
>> ID.
>> -- 
>>
>> commit 20f4e35b2f88ebe2b8988b4f07fe7cc28199096b
>> Author: Jihong Min <hurryman2212@gmail.com>
>> Subject: usb: xhci-pci: add AMD Promontory 21 PCI glue
>>
>> This commit adds a small PCI glue driver for AMD Promontory 21 xHCI
>> controllers. It binds the controllers, delegates USB host operation to
>> the common xHCI PCI core, and creates an auxiliary device for optional
>> temperature sensor support.
>>
>>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>>> --- a/drivers/usb/host/Kconfig
>>> +++ b/drivers/usb/host/Kconfig
>>
>> [ ... ]
>>
>>> @@ -42,6 +42,26 @@ config USB_XHCI_PCI
>>>       depends on USB_PCI
>>>       default y
>>>   +config USB_XHCI_PCI_PROM21
>>> +    bool "AMD Promontory 21 xHCI PCI support"
>>> +    depends on X86
>>> +    depends on USB_XHCI_PCI=y
>>
>> Does depending strictly on USB_XHCI_PCI=y cause a configuration 
>> regression
>> for most distributions where the core driver is configured as a module?
>>
>> The driver source appears fully capable of being built as a module, 
>> using
>> module-related macros and module_pci_driver().
>>
>> If the intent is to prevent an early boot mismatch where xhci-pci is
>> built-in but the PROM21 glue is a module, could this instead be a 
>> tristate
>> option using depends on USB_XHCI_PCI and depends on !(USB_XHCI_PCI=y 
>> && m)?
>> This would enforce the boot constraint while preserving support for 
>> modular
>> kernel builds.
>>
>
> That makes sense to me. That would be stronger than using default (which
> was my idea of dealing with the problem). Restricting the code to 
> USB_XHCI_PCI=y
> seems to be a bit extreme.

Hi Guenter,

I looked more closely at the Kconfig part of the Sashiko suggestion.

The concern itself is valid: the v5 dependency on USB_XHCI_PCI=y is too
strict because it prevents the PROM21 PCI glue from being built when
USB_XHCI_PCI=m.

However, I do not think the suggested expression works as intended:

   depends on !(USB_XHCI_PCI=y && m)

With Kconfig tristate logic, `m` is a tristate literal, not "this symbol is
being built as a module". If USB_XHCI_PCI=y, then:

   USB_XHCI_PCI=y && m  => m
   !(USB_XHCI_PCI=y && m) => m

So the dependency becomes `m`, which limits USB_XHCI_PCI_PROM21 to `m` 
instead
of allowing `y`. That is the opposite of what we need for the built-in
xhci-pci case.

The combinations I think we want are:

   USB_XHCI_PCI=y  -> USB_XHCI_PCI_PROM21=y or n, but not m
   USB_XHCI_PCI=m  -> USB_XHCI_PCI_PROM21=m or n
   USB_XHCI_PCI=n  -> USB_XHCI_PCI_PROM21=n

I see a few possible ways to handle this:

1. Keep USB_XHCI_PCI_PROM21 as a visible tristate:

      config USB_XHCI_PCI_PROM21
        tristate "AMD Promontory 21 xHCI PCI support"
        depends on X86
        depends on USB_XHCI_PCI
        default USB_XHCI_PCI
        select AUXILIARY_BUS

    This supports USB_XHCI_PCI=m, but it still lets a user select the unsafe
    USB_XHCI_PCI=y / USB_XHCI_PCI_PROM21=m combination.

2. Keep it visible and rely on help text to warn users not to select `m` 
when
    USB_XHCI_PCI=y.

    This is simple, but it does not actually prevent the bad combination.

3. Use IS_REACHABLE() in xhci-pci.c instead of IS_ENABLED().

    That would prevent built-in xhci-pci from rejecting PROM21 devices 
when the
    PROM21 glue is only available as a module. However, it also means the
    PROM21 glue/hwmon path would not be used in that configuration 
unless the
    PCI device is rebound later, so I do not think it is ideal.

4. Make USB_XHCI_PCI_PROM21 a hidden tristate that follows USB_XHCI_PCI:

      config USB_XHCI_PCI_PROM21
        tristate
        depends on X86
        depends on USB_XHCI_PCI
        default USB_XHCI_PCI
        select AUXILIARY_BUS

    Then the value follows the common xhci-pci driver:

      USB_XHCI_PCI=y  -> USB_XHCI_PCI_PROM21=y
      USB_XHCI_PCI=m  -> USB_XHCI_PCI_PROM21=m
      USB_XHCI_PCI=n  -> USB_XHCI_PCI_PROM21=n

    This prevents the unsafe y/m split while still supporting modular
    USB_XHCI_PCI builds. The actual user-visible sensor option remains
    SENSORS_PROM21_XHCI.

My current preference is option 4, because the PROM21 PCI glue is not 
really a
user-facing feature by itself. It exists so the common xhci-pci driver 
can hand
PROM21 devices to the PROM21-specific glue and so the optional hwmon 
driver can
bind through the auxiliary device. The user-visible part remains the hwmon
sensor driver.

Does that sound reasonable to you, or would you prefer one of the other
approaches?

Thanks,
Jihong