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

[Guenter Roeck <linux@roeck-us.net>]

On 5/14/26 06:44, Jihong Min wrote:
> 
> On 5/14/26 22:38, Jihong Min wrote:
>>
>> 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
>>
> One follow-up to my previous mail:
> 
> If we go with option 4 and make USB_XHCI_PCI_PROM21 a hidden tristate
> symbol, I think its help text should probably be removed as well, since the
> symbol would no longer have a user-visible prompt.
> 
Agreed.

> The user-visible option would remain SENSORS_PROM21_XHCI, where the hwmon
> sensor help text can stay.
> 
> Does that sound right?
> 
Yes.

Thanks,
Guenter