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

[Jihong Min]

On 5/18/26 06:21, Michal Pecio wrote:
> Instead of the X86 heuristic, would it be possible to build glue
> code if and only if SENSORS_PROM21_XHCI is enabled?
>
> This seems to work:
>
>   config SENSORS_PROM21_XHCI
>          tristate "AMD Promontory 21 xHCI temperature sensor"
> -       depends on USB_XHCI_PCI_PROM21
> +       depends on USB_XHCI_PCI
>
>   config USB_XHCI_PCI_PROM21
>          tristate
> -       depends on X86
>          depends on USB_XHCI_PCI
> -       default USB_XHCI_PCI
> +       default USB_XHCI_PCI if SENSORS_PROM21_XHCI != 'n'
>          select AUXILIARY_BUS
>
> I don't know if it's the best way, perhaps it would be preferable for
> the hwmon driver to select the glue, but then I'm not sure how to force
> glue to become 'y' when xhci-pci is 'y'.

I think I should keep the current hidden glue option for now.

The PROM21 PCI glue is part of the PCI binding path for the xHCI controller
when enabled, while SENSORS_PROM21_XHCI is only the optional user-visible
hwmon driver. Tying the glue to the hwmon option would make the sensor 
option
affect which driver binds the USB controller. As Guenter pointed out, that
would be too strong; the USB controller should not depend on whether the
optional hwmon driver is enabled.

So I would prefer to keep USB_XHCI_PCI_PROM21 as hidden plumbing that 
follows
USB_XHCI_PCI, and keep SENSORS_PROM21_XHCI as the user-visible sensor 
option.

> +static int prom21_xhci_create_auxdev(struct pci_dev *pdev)
> +{
> +	struct prom21_xhci_auxdev *prom21_auxdev;
> +	struct usb_hcd *hcd = pci_get_drvdata(pdev);
> +
> +	if (!hcd)
> +		return -ENODEV;
>
> Shouldn't be necessary after successful xhci_pci_common_probe().

Agreed. I removed the unnecessary NULL check from
prom21_xhci_create_auxdev() locally for v7.

> +	prom21_auxdev->id = ida_alloc(&prom21_xhci_auxdev_ida, GFP_KERNEL);
> +	if (prom21_auxdev->id < 0) {
> +		int ret = prom21_auxdev->id;
> +
> +		devres_free(prom21_auxdev);
> +		return ret;
> +	}
> +
> +	prom21_auxdev->auxdev = auxiliary_device_create(&pdev->dev,
> +							KBUILD_MODNAME, "hwmon",
> +							&prom21_auxdev->pdata,
> +							prom21_auxdev->id);
> +	if (!prom21_auxdev->auxdev) {
> +		ida_free(&prom21_xhci_auxdev_ida, prom21_auxdev->id);
> +		devres_free(prom21_auxdev);
> +		return -ENOMEM;
>
> The usual "goto error" pattern could be used instead of increasingly
> long sequences of xxx_free() calls.

Agreed. I changed prom21_xhci_create_auxdev() to use a goto-based 
cleanup path
locally for v7.

> It seems that these three functions above are everything that you truly
> want to add; the rest is boilerplate required by this two-module scheme
> to work, plus ID tables which must be duplicated and kept in sync.
>
> I wonder if a separate module is really justified, as opposed to simply
> linking this file into xhci_pci.ko when directed by Kconfig.
>
> The downside would be slightly higher memory usage on systems where the
> hwmon driver is enabled but not needed. OTOH, same systems would likely
> see reduced disk waste.

I understand the concern. Linking the PROM21 auxiliary-device publisher
into xhci_pci.ko would reduce some boilerplate and avoid the extra PCI
driver, while still keeping the hwmon driver itself separate.

The reason I kept the current split is that the earlier review direction
was to keep the hwmon functionality out of xhci-pci and bind a
drivers/hwmon driver through an auxiliary device. The current PROM21 PCI
glue keeps the PROM21-specific auxiliary-device lifetime handling outside
the common xhci-pci driver and leaves xhci-pci.c with only the PCI ID
handoff, similar in spirit to the Renesas handoff path.

That said, I agree this is a tradeoff. If Mathias or the USB maintainers
prefer the PROM21 auxiliary-device publisher to be linked into xhci_pci.ko
instead of being a separate PCI glue driver, I can rework it in that
direction while still keeping the hwmon driver under drivers/hwmon and
bound through the auxiliary bus.


Sincerely,
Jihong Min