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

[Jihong Min <hurryman2212@icloud.com>]

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?

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