Re: [PATCH v3 1/2] usb: xhci-pci: add generic auxiliary device interface

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

On 5/8/26 00:04, Jihong Min wrote:
> Hi Mathias,
> 
> I tried the xhci-pci-prom21.c approach you suggested, with a PROM21-specific
> PCI glue driver calling xhci_pci_common_probe() and creating the auxiliary
> hwmon child device from that driver.
> 
> While doing that I noticed a possible boot-time regression with the module
> case.
> 
> If CONFIG_USB_XHCI_PCI=y and CONFIG_USB_XHCI_PCI_PROM21=m, then generic
> xhci-pci sees CONFIG_USB_XHCI_PCI_PROM21 as enabled and refuses the PROM21
> PCI ID:
> 
>      if (IS_ENABLED(CONFIG_USB_XHCI_PCI_PROM21) &&
>          pci_match_id(pci_ids_prom21, dev))
>              return -ENODEV;
> 
> That means the PROM21 xHCI controller is handled only by
> xhci-pci-prom21.ko. If that module is not present in the initramfs or is not
> loaded early enough, the PROM21 xHCI controller remains unbound during early
> boot. Devices behind that controller, such as a USB keyboard used for early
> boot or disk unlock, would not work even though the generic xhci-pci driver is
> built in and could otherwise operate the controller.
> 
> This seems different from the Renesas case, where the separate PCI driver is
> needed for controller-specific firmware handling. For PROM21, the USB/xHCI
> operation itself is still generic; the only extra function is publishing an
> optional hwmon child device.
> 
> So I am not sure what the preferred direction should be:
> 
>    1. Keep the separate xhci-pci-prom21.c PCI glue driver and make
>       USB_XHCI_PCI_PROM21 built-in only, or otherwise constrain the Kconfig so
>       the generic xhci-pci handoff cannot break early boot.
> 
>    2. Keep PROM21 handled by generic xhci-pci and add only a small
>       PROM21-specific auxiliary-device creation hook in xhci-pci after the
>       common probe succeeds. In that model, failure to create the optional hwmon
>       auxiliary device would not affect USB operation.
> 
>    3. Some other split that keeps PROM21-specific sensor code outside
>       xhci-pci, but does not prevent generic xhci-pci from binding the
>       controller when the optional PROM21 glue is not available early.
> 
> Do you still prefer the separate xhci-pci-prom21.c PCI driver for this case,
> or would the minimal xhci-pci auxiliary-device hook be more appropriate given
> the built-in xhci-pci / modular PROM21 glue case?
> 

Maybe I am missing something, but it seems to me that CONFIG_USB_XHCI_PCI_PROM21
should be just as built-in as CONFIG_USB_XHCI_PCI.

Thanks,
Guenter

> Sincerely,
> Jihong Min
> 
> On 5/7/26 18:31, Mathias Nyman wrote:
>> On 5/7/26 06:31, Jihong Min wrote:
>>> Some xHCI PCI controllers expose controller-specific functionality that is
>>> not part of generic xHCI operation and is better handled by optional child
>>> drivers in other subsystems. Add a small auxiliary device registration path
>>> for selected xHCI PCI controllers.
>>>
>>> The initial PCI ID match table lists AMD Promontory 21 (PROM21) 1022:43fd
>>> controllers. For matching controllers, xhci-pci creates an auxiliary
>>> device and stores it in devres so the remove path destroys it before HCD
>>> teardown.
>>>
>>> Subsystem-specific child drivers can then bind to those devices through
>>> the auxiliary bus and keep their hardware-specific logic outside xhci-pci.
>>>
>>> Assisted-by: Codex:gpt-5.5
>>> Signed-off-by: Jihong Min <hurryman2212@gmail.com>
>>> ---
>>>   drivers/usb/host/Kconfig    | 10 +++++
>>>   drivers/usb/host/xhci-pci.c | 83 +++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 93 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>>> index 0a277a07cf70..e0c2c7ac5c97 100644
>>> --- a/drivers/usb/host/Kconfig
>>> +++ b/drivers/usb/host/Kconfig
>>> @@ -42,6 +42,16 @@ config USB_XHCI_PCI
>>>       depends on USB_PCI
>>>       default y
>>>   +config USB_XHCI_PCI_AUXDEV
>>> +    bool "xHCI PCI auxiliary device support"
>>> +    depends on USB_XHCI_PCI
>>> +    select AUXILIARY_BUS
>>> +    help
>>> +      This enables xHCI PCI support for registering auxiliary devices
>>> +      for selected controllers. It is used by optional child drivers
>>> +      that bind to xHCI PCI controller-specific functionality through
>>> +      the auxiliary bus.
>>> +
>>>   config USB_XHCI_PCI_RENESAS
>>>       tristate "Support for additional Renesas xHCI controller with firmware"
>>>       depends on USB_XHCI_PCI
>>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>>> index 585b2f3117b0..618d6840e108 100644
>>> --- a/drivers/usb/host/xhci-pci.c
>>> +++ b/drivers/usb/host/xhci-pci.c
>>> @@ -8,6 +8,8 @@
>>>    * Some code borrowed from the Linux EHCI driver.
>>>    */
>>>   +#include <linux/auxiliary_bus.h>
>>> +#include <linux/device/devres.h>
>>>   #include <linux/pci.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/module.h>
>>> @@ -80,6 +82,7 @@
>>>   #define PCI_DEVICE_ID_AMD_RAVEN_15E1_XHCI        0x15e1
>>>   #define PCI_DEVICE_ID_AMD_RAVEN2_XHCI            0x15e5
>>>   #define PCI_DEVICE_ID_AMD_RENOIR_XHCI            0x1639
>>> +#define PCI_DEVICE_ID_AMD_PROM21_XHCI            0x43fd
>>>   #define PCI_DEVICE_ID_AMD_PROMONTORYA_4            0x43b9
>>>   #define PCI_DEVICE_ID_AMD_PROMONTORYA_3            0x43ba
>>>   #define PCI_DEVICE_ID_AMD_PROMONTORYA_2            0x43bb
>>> @@ -103,6 +106,80 @@ static int xhci_pci_run(struct usb_hcd *hcd);
>>>   static int xhci_pci_update_hub_device(struct usb_hcd *hcd, struct usb_device *hdev,
>>>                         struct usb_tt *tt, gfp_t mem_flags);
>>>   +static const struct pci_device_id pci_ids_have_aux[] = {
>>> +    { PCI_DEVICE_DATA(AMD, PROM21_XHCI, "prom21_hwmon") },
>>> +    { /* end: all zeroes */ }
>>> +};
>>> +
>>> +struct xhci_pci_aux_devres {
>>> +    struct auxiliary_device *auxdev;
>>> +};
>>> +
>>> +static const char *xhci_pci_aux_dev_name(struct pci_dev *pdev)
>>> +{
>>> +    const struct pci_device_id *id;
>>> +
>>> +    id = pci_match_id(pci_ids_have_aux, pdev);
>>> +    if (!id)
>>> +        return NULL;
>>> +
>>> +    return (const char *)id->driver_data;
>>> +}
>>> +
>>> +static void xhci_pci_aux_devres_release(struct device *dev, void *res)
>>> +{
>>> +    struct xhci_pci_aux_devres *devres = res;
>>> +
>>> +    if (devres->auxdev)
>>> +        auxiliary_device_destroy(devres->auxdev);
>>> +}
>>> +
>>> +static void xhci_pci_try_add_aux_device(struct pci_dev *pdev)
>>> +{
>>> +    struct xhci_pci_aux_devres *devres;
>>> +    struct auxiliary_device *auxdev;
>>> +    const char *aux_dev_name;
>>> +
>>> +    aux_dev_name = xhci_pci_aux_dev_name(pdev);
>>> +    if (!aux_dev_name)
>>> +        return;
>>> +
>>> +    devres = devres_alloc(xhci_pci_aux_devres_release, sizeof(*devres),
>>> +                  GFP_KERNEL);
>>> +    if (!devres) {
>>> +        dev_warn(&pdev->dev,
>>> +             "failed to allocate auxiliary device state\n");
>>> +        return;
>>> +    }
>>> +
>>> +    auxdev = auxiliary_device_create(&pdev->dev, KBUILD_MODNAME,
>>> +                     aux_dev_name, NULL,
>>> +                     (pci_domain_nr(pdev->bus) << 16) |
>>> +                         pci_dev_id(pdev));
>>> +    if (!auxdev) {
>>> +        devres_free(devres);
>>> +        dev_warn(&pdev->dev, "failed to add %s auxiliary device\n",
>>> +             aux_dev_name);
>>> +        return;
>>> +    }
>>> +
>>> +    devres->auxdev = auxdev;
>>> +    devres_add(&pdev->dev, devres);
>>> +}
>>> +
>>> +static void xhci_pci_try_remove_aux_device(struct pci_dev *pdev)
>>> +{
>>> +    struct xhci_pci_aux_devres *devres;
>>> +
>>> +    devres = devres_find(&pdev->dev, xhci_pci_aux_devres_release, NULL,
>>> +                 NULL);
>>> +    if (!devres || !devres->auxdev)
>>> +        return;
>>> +
>>> +    auxiliary_device_destroy(devres->auxdev);
>>> +    devres->auxdev = NULL;
>>> +}
>>> +
>>>   static const struct xhci_driver_overrides xhci_pci_overrides __initconst = {
>>>       .reset = xhci_pci_setup,
>>>       .start = xhci_pci_run,
>>> @@ -677,6 +754,9 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>       if (device_property_read_bool(&dev->dev, "ti,pwron-active-high"))
>>>           pci_clear_and_set_config_dword(dev, 0xE0, 0, 1 << 22);
>>>   +    if (IS_ENABLED(CONFIG_USB_XHCI_PCI_AUXDEV))
>>> +        xhci_pci_try_add_aux_device(dev);
>>> +
>>>       return 0;
>>
>> I think this should be turned around so that the vendor specific code calls the common code.
>> xhci-pci-renesas.c does this nicely.
>>
>> In your case it would be adding something like a xhci-pci-prom21.c pci driver:
>>
>> xhci_pci_prom21_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> {
>>     crate_auxiliary_device(dev);
>>     return xhci_pci_common_probe(dev, id);
>> }
>>
>> xhci_pci_prom21_remove(struct pci_dev *dev)
>> {
>>     destroy_auxiliary_device(dev);
>>     xhci_pci_remove(dev);
>> }
>>
>> static const struct pci_device_id pci_ids[] = {
>>     { PCI_DEVICE(YOUR_AMD_PCI_VENDOR_ID, YOUR_PROM21_DEVICE_ID) },
>>     { /* end: all zeroes */ }
>> };
>> MODULE_DEVICE_TABLE(pci, pci_ids);
>>
>> static struct pci_driver xhci_prom21_pci_driver = {
>>     .name =        "xhci-pci-prom21",
>>     .id_table =    pci_ids,
>>
>>     .probe =    xhci_pci_prom21_probe,
>>     .remove =    xhci_pci_prom21_remove,
>>
>>     .shutdown =     usb_hcd_pci_shutdown,
>>     .driver = {
>>         .pm = pm_ptr(&usb_hcd_pci_pm_ops),
>>     },
>> };
>> module_pci_driver(xhci_prom21_pci_driver);
>>
>> MODULE_DESCRIPTION("AMD Promontory 21 xHCI PCI Host Controller Driver");
>> MODULE_IMPORT_NS("xhci");
>> MODULE_LICENSE("GPL v2");
>>
>> -Mathias
>