| Message ID | 20250127151051.1a91bbe6@foxbook (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: Restore xhci_pci support for Renesas HCs | expand |
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
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
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); }
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(+)