[PATCH v4] PCI/portdrv: Allow probing even without child services

[PATCH v4] PCI/portdrv: Allow probing even without child services

[Brian Norris]

The PCIe port driver fails to probe if it finds no child services,
presumably under the assumption that the driver is not useful in that case.
However, the driver *can* still be useful for power management support --
namely, it still configures the port for runtime PM / D3, which may be
important for allowing a bridge to enter low power modes.

Thus, allow probe to succeed even if no IRQs and no child services are
available. This also mirrors existing behavior for ports that don't support
any portdrv services (PCIe hotplug, AER, DPC, PME, bwctrl), where we'd also
probe successfully.

This change is a bit more important after commit f5cd8a929c82 ("PCI: dwc:
Remove MSI/MSIX capability for Root Port if iMSI-RX is used as MSI
controller"), because it's common for some DWC-based systems to:

  1. have only the "aer" and "pcie_pme" port services available and

  2. not define legacy INTx interrupts properly in their device tree.

After commit f5cd8a929c82, such systems may fail pcie_init_service_irqs()
and so exit with -ENODEV.

Link: https://lore.kernel.org/all/nyada24tqwlkzdceyoxbzitzygvp4elvj5oajnqdwb33xkcdwk@76vnrx45fsfd/
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v4:
 * Between sashiko-bot / Mani / me / Bjorn, we agreed that deferring
   pci_set_master() may cause us to drop some MSIs:
     https://lore.kernel.org/all/af574zaysKnXjx15@google.com/
   So we revert to the v2 approach, with some small changes
 * Address some of Lukas's review comments:
     https://lore.kernel.org/all/al68lFtbq_Q4RCL2@google.com/
   Specifically:
     - ensure pcie_device_init() return code is checked
     - release IRQs (pci_free_irq_vectors()) when no services registered
       properly
 * Borrow some of Bjorn's commit message tweaks

Changes in v3:
 * Defer pci_set_master() until we really know we have some child
   service

Changes in v2:
 * Clear master when we have no child services

 drivers/pci/pcie/portdrv.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 2d6aa488fe7b..18ee190acc6e 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -330,7 +330,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
  */
 static int pcie_port_device_register(struct pci_dev *dev)
 {
-	int status, capabilities, i, nr_service;
+	int status, capabilities, i;
 	int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
 
 	/* Enable PCI Express port device */
@@ -355,29 +355,29 @@ static int pcie_port_device_register(struct pci_dev *dev)
 	if (status) {
 		capabilities &= PCIE_PORT_SERVICE_HP;
 		if (!capabilities)
-			goto error_disable;
+			goto out;
 	}
 
 	/* Allocate child services if any */
-	status = -ENODEV;
-	nr_service = 0;
 	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
 		int service = 1 << i;
 		if (!(capabilities & service))
 			continue;
-		if (!pcie_device_init(dev, service, irqs[i]))
-			nr_service++;
+		if (pcie_device_init(dev, service, irqs[i]))
+			capabilities &= ~service;
 	}
-	if (!nr_service)
-		goto error_cleanup_irqs;
 
-	return 0;
+out:
+	/*
+	 * With no child services, we shouldn't need bus mastering or any IRQ
+	 * vectors we allocated.
+	 */
+	if (!capabilities) {
+		pci_free_irq_vectors(dev);
+		pci_clear_master(dev);
+	}
 
-error_cleanup_irqs:
-	pci_free_irq_vectors(dev);
-error_disable:
-	pci_disable_device(dev);
-	return status;
+	return 0;
 }
 
 typedef int (*pcie_callback_t)(struct pcie_device *);
-- 
2.55.0.229.g6434b31f56-goog

Re: [PATCH v4] PCI/portdrv: Allow probing even without child services

[Lukas Wunner]

On Wed, Jul 22, 2026 at 04:09:42PM -0700, Brian Norris wrote:
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -330,7 +330,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
>   */
>  static int pcie_port_device_register(struct pci_dev *dev)
>  {
> -	int status, capabilities, i, nr_service;
> +	int status, capabilities, i;
>  	int irqs[PCIE_PORT_DEVICE_MAXSERVICES];

Nit:  Move the amended line below the irqs[] declaration to sustain
inverse Christmas tree ordering.

Otherwise this is
Reviewed-by: Lukas Wunner <lukas@wunner.de>

One caveat, I note there is a pre-existing issue on Loongson LS7A,
which (according to 62b6dee1b44a) suffers from a defect wherein
disabling bus mastering prevents forwarding of MMIO requests downstream.
This happens right now (and continues to happen after the present patch)
if the error path in pcie_port_device_register() is taken.

I also note that pci_enable_device() is only called because per
1ce5e83063bf, it was necessary in 2009 for legacy interrupt initialization
"on some architectures", which sounds fairly vague to me.  In any case,
it seems to suggest that pci_disable_device() should be called if
pcie_init_service_irqs() fails.  Right now that is done, after the patch
it isn't.  I guess portdrv would have to remember on probe whether
pci_enable_device() was called and only then call pci_disable_device()
on remove.  I guess it would be possible to abuse dev_set_drvdata()
to store this status bit (as a fake pointer).  Then again, I'm not
sure pci_enable_device() is still necessary for legacy interrupt
initalization today.  I guess "legacy interrupt" refers to INTx.
A lot has changed in generic and PCI irq code since 2009.
I assume we otherwise don't have a need to access BARs on PCIe ports
currently, but perhaps new port services may come along which do?

Thanks,

Lukas