From: Jihong Min <hurryman2212@icloud.com>
To: Guenter Roeck <linux@roeck-us.net>,
sashiko-reviews@lists.linux.dev,
Jihong Min <hurryman2212@gmail.com>
Cc: linux-hwmon@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v5 1/2] usb: xhci-pci: add AMD Promontory 21 PCI glue
Date: Thu, 14 May 2026 22:44:30 +0900 [thread overview]
Message-ID: <a90ab332-9f86-4486-8243-07c98e3f7578@icloud.com> (raw)
In-Reply-To: <1f296d29-3d43-4dd3-b751-8a0892b4095a@icloud.com>
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.
The user-visible option would remain SENSORS_PROM21_XHCI, where the hwmon
sensor help text can stay.
Does that sound right?
Thanks,
Jihong
next prev parent reply other threads:[~2026-05-14 13:44 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-12 21:39 [PATCH v5 0/2] AMD Promontory 21 xHCI temperature sensor support Jihong Min
2026-05-12 21:39 ` [PATCH v5 1/2] usb: xhci-pci: add AMD Promontory 21 PCI glue Jihong Min
2026-05-14 0:40 ` sashiko-bot
2026-05-14 1:20 ` Guenter Roeck
2026-05-14 13:38 ` Jihong Min
2026-05-14 13:44 ` Jihong Min [this message]
2026-05-15 1:49 ` Guenter Roeck
2026-05-14 14:29 ` Jihong Min
2026-05-14 19:22 ` Guenter Roeck
2026-05-12 21:39 ` [PATCH v5 2/2] hwmon: add AMD Promontory 21 xHCI temperature sensor support Jihong Min
2026-05-14 1:00 ` sashiko-bot
2026-05-14 1:13 ` Guenter Roeck
2026-05-12 21:49 ` [PATCH v5 0/2] " Jihong Min
2026-05-13 14:48 ` Mario Limonciello
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a90ab332-9f86-4486-8243-07c98e3f7578@icloud.com \
--to=hurryman2212@icloud.com \
--cc=hurryman2212@gmail.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).