[PATCH] usb: xhci: Restore xhci_pci support for Renesas HCs

[PATCH] usb: xhci: Restore xhci_pci support for Renesas HCs

[Michal Pecio]

Some Renesas HCs require firmware upload to work, this is handled by the
xhci_pci_renesas driver. Other variants of those chips load firmware from
onboard flash and can work with xhci_pci alone.

A refactor merged in v6.12 broke the latter configuration and users are
finding their hardware not working after upgrading and need to enable the
firmware loader which isn't really necessary on their systems.

Let xhci_pci work with those chips as before when the firmware loader is
disabled by kernel configuration.

Fixes: 25f51b76f90f ("xhci-pci: Make xhci-pci-renesas a proper modular driver")
CC: stable@vger.kernel.org
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219616
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219726
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5a5f0b2dba4d..3d08d6527b1d 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -657,17 +657,21 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
 }
 EXPORT_SYMBOL_NS_GPL(xhci_pci_common_probe, "xhci");
 
+#if IS_ENABLED(CONFIG_USB_XHCI_PCI_RENESAS)
 static const struct pci_device_id pci_ids_reject[] = {
 	/* handled by xhci-pci-renesas */
 	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0014) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0015) },
 	{ /* end: all zeroes */ }
 };
+#endif
 
 static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
+#if IS_ENABLED(CONFIG_USB_XHCI_PCI_RENESAS)
 	if (pci_match_id(pci_ids_reject, dev))
 		return -ENODEV;
+#endif
 
 	return xhci_pci_common_probe(dev, id);
 }
-- 
2.48.1

Re: [PATCH] usb: xhci: Restore xhci_pci support for Renesas HCs

[Greg Kroah-Hartman]

On Mon, Jan 27, 2025 at 03:10:51PM +0100, Michal Pecio wrote:
> Some Renesas HCs require firmware upload to work, this is handled by the
> xhci_pci_renesas driver. Other variants of those chips load firmware from
> onboard flash and can work with xhci_pci alone.
> 
> A refactor merged in v6.12 broke the latter configuration and users are
> finding their hardware not working after upgrading and need to enable the
> firmware loader which isn't really necessary on their systems.

Why isn't it necessary?  Those chips want firmware to be uploaded, and
if the driver doesn't need it, it should be able to handle it if it is
not present.

> Let xhci_pci work with those chips as before when the firmware loader is
> disabled by kernel configuration.
> 
> Fixes: 25f51b76f90f ("xhci-pci: Make xhci-pci-renesas a proper modular driver")
> CC: stable@vger.kernel.org
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219616
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219726
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---
>  drivers/usb/host/xhci-pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 5a5f0b2dba4d..3d08d6527b1d 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -657,17 +657,21 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  }
>  EXPORT_SYMBOL_NS_GPL(xhci_pci_common_probe, "xhci");
>  
> +#if IS_ENABLED(CONFIG_USB_XHCI_PCI_RENESAS)
>  static const struct pci_device_id pci_ids_reject[] = {
>  	/* handled by xhci-pci-renesas */
>  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0014) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0015) },
>  	{ /* end: all zeroes */ }
>  };
> +#endif
>  
>  static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> +#if IS_ENABLED(CONFIG_USB_XHCI_PCI_RENESAS)
>  	if (pci_match_id(pci_ids_reject, dev))
>  		return -ENODEV;
> +#endif

No, please don't put #ifdef in .c files if at all preventable.

If the firmware isn't there, then the driver should be fixed to keep on
working if it determines loading it is not needed.  This type of check
should not be needed in the xhci-pci.c file.

thanks,

greg k-h

Re: [PATCH] usb: xhci: Restore xhci_pci support for Renesas HCs

[Michał Pecio]

On Mon, 27 Jan 2025 18:29:52 +0100, Greg Kroah-Hartman wrote:
> On Mon, Jan 27, 2025 at 03:10:51PM +0100, Michal Pecio wrote:
> > Some Renesas HCs require firmware upload to work, this is handled
> > by the xhci_pci_renesas driver. Other variants of those chips load
> > firmware from onboard flash and can work with xhci_pci alone.
> > 
> > A refactor merged in v6.12 broke the latter configuration and users
> > are finding their hardware not working after upgrading and need to
> > enable the firmware loader which isn't really necessary on their
> > systems.  
> 
> Why isn't it necessary?  Those chips want firmware to be uploaded, and
> if the driver doesn't need it, it should be able to handle it if it is
> not present.

Because their chips have already loaded themselves from attached SPI
flash, so they are ready to use with xhci_pci like any other HC.

The FW isn't optional, we just usually don't need to load it. And FW
for those chips is obscure and not redistributable AFAIK, so people
have little reason to enable xhci_pci_renesas if their board has ROM
and works without it. It is 'N' by default.

> No, please don't put #ifdef in .c files if at all preventable.

Hmm, I suppose I could replace the #ifdef with 'if (IS_ENABLED(...))'
and leave the rest to dead code elimination if that's preferred.

Annoyingly, the code to be knocked out is in xhci-pci and it has to
stay there to avoid runtime dependency on xhci-pci-renesas, which was
the whole point of the refactor.

> If the firmware isn't there, then the driver should be fixed to keep
> on working if it determines loading it is not needed.  This type of
> check should not be needed in the xhci-pci.c file.

This is a check for kernel configuration, not for firmware. If the
loader is disabled, we don't want to reject those chips expecting them
to be taken care of by the loader, because that is not going to happen.

Regards,
Michal

[PATCH v2] usb: xhci: Restore xhci_pci support for Renesas HCs

[Michal Pecio]

Some Renesas HCs require firmware upload to work, this is handled by the
xhci_pci_renesas driver. Other variants of those chips load firmware from
a SPI flash and are ready to work with xhci_pci alone.

A refactor merged in v6.12 broke the latter configuration so that users
are finding their hardware ignored by the normal driver and are forced to
enable the firmware loader which isn't really necessary on their systems.

Let xhci_pci work with those chips as before when the firmware loader is
disabled by kernel configuration.

Fixes: 25f51b76f90f ("xhci-pci: Make xhci-pci-renesas a proper modular driver")
CC: stable@vger.kernel.org
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219616
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219726
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---

new in v2:
- changed #ifdef CONFIG_XXX to if (IS_ENABLED(CONFIG_XXX))
- renamed the PCI IDs table to clarify code intent
- small commit message improvements

 drivers/usb/host/xhci-pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5a5f0b2dba4d..54460d11f7ee 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -657,8 +657,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
 }
 EXPORT_SYMBOL_NS_GPL(xhci_pci_common_probe, "xhci");
 
-static const struct pci_device_id pci_ids_reject[] = {
-	/* handled by xhci-pci-renesas */
+/* handled by xhci-pci-renesas if enabled */
+static const struct pci_device_id pci_ids_renesas[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0014) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0015) },
 	{ /* end: all zeroes */ }
@@ -666,7 +666,8 @@ static const struct pci_device_id pci_ids_reject[] = {
 
 static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	if (pci_match_id(pci_ids_reject, dev))
+	if (IS_ENABLED(CONFIG_USB_XHCI_PCI_RENESAS) &&
+			pci_match_id(pci_ids_renesas, dev))
 		return -ENODEV;
 
 	return xhci_pci_common_probe(dev, id);
-- 
2.48.1

Re: [PATCH v2] usb: xhci: Restore xhci_pci support for Renesas HCs

[nb]

On 28.1.2025 10:45, Michal Pecio wrote:
> Some Renesas HCs require firmware upload to work, this is handled by 
> the
> xhci_pci_renesas driver. Other variants of those chips load firmware 
> from
> a SPI flash and are ready to work with xhci_pci alone.
> 
> A refactor merged in v6.12 broke the latter configuration so that users
> are finding their hardware ignored by the normal driver and are forced 
> to
> enable the firmware loader which isn't really necessary on their 
> systems.
> 
> Let xhci_pci work with those chips as before when the firmware loader 
> is
> disabled by kernel configuration.
> 
> Fixes: 25f51b76f90f ("xhci-pci: Make xhci-pci-renesas a proper modular 
> driver")
> CC: stable@vger.kernel.org
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219616
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219726
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>

Can confirm that this patch restores the old behavior, where xhci-pci 
supported the Renesas controllers when an external SPI flash is 
attached. I've tested with a PCIe extension card on arm64 and x86 as 
well as with a arm64 (Raspberry CM4) based Revolution Pi Connect 4.

Tested-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks!

Nicolai

> ---
> 
> new in v2:
> - changed #ifdef CONFIG_XXX to if (IS_ENABLED(CONFIG_XXX))
> - renamed the PCI IDs table to clarify code intent
> - small commit message improvements
> 
>  drivers/usb/host/xhci-pci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 5a5f0b2dba4d..54460d11f7ee 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -657,8 +657,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, 
> const struct pci_device_id *id)
>  }
>  EXPORT_SYMBOL_NS_GPL(xhci_pci_common_probe, "xhci");
> 
> -static const struct pci_device_id pci_ids_reject[] = {
> -	/* handled by xhci-pci-renesas */
> +/* handled by xhci-pci-renesas if enabled */
> +static const struct pci_device_id pci_ids_renesas[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0014) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0015) },
>  	{ /* end: all zeroes */ }
> @@ -666,7 +666,8 @@ static const struct pci_device_id pci_ids_reject[] 
> = {
> 
>  static int xhci_pci_probe(struct pci_dev *dev, const struct 
> pci_device_id *id)
>  {
> -	if (pci_match_id(pci_ids_reject, dev))
> +	if (IS_ENABLED(CONFIG_USB_XHCI_PCI_RENESAS) &&
> +			pci_match_id(pci_ids_renesas, dev))
>  		return -ENODEV;
> 
>  	return xhci_pci_common_probe(dev, id);