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

[sashiko-bot@kernel.org]

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.

[ ... ]

> diff --git a/drivers/usb/host/xhci-pci-prom21.c b/drivers/usb/host/xhci-pci-prom21.c
> --- /dev/null
> +++ b/drivers/usb/host/xhci-pci-prom21.c

[ ... ]

> +static int prom21_xhci_create_auxdev(struct pci_dev *pdev)
> +{
> +	struct prom21_xhci_auxdev *prom21_auxdev;
> +	struct usb_hcd *hcd = pci_get_drvdata(pdev);

[ ... ]

> +	prom21_auxdev->auxdev =
> +		auxiliary_device_create(&pdev->dev, KBUILD_MODNAME, "hwmon",
> +					&prom21_auxdev->pdata,
> +					(pci_domain_nr(pdev->bus) << 16) |
> +						pci_dev_id(pdev));

Does left-shifting pci_domain_nr() introduce a signed integer overflow
regression?

The function pci_domain_nr() returns a signed int, which can be up to 0xFFFF
on ACPI systems. Left-shifting a value of 0x8000 or greater by 16 bits
alters the sign bit of a 32-bit int, resulting in undefined behavior in C.

Would casting the return value to an unsigned integer, such as
(u32)pci_domain_nr(pdev->bus) << 16, avoid this overflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512213910.871859-1-hurryman2212@gmail.com?part=1