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

[PATCH] 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, we allow probe to succeed even if no IRQs and no child services
are available. This also mirrors existing behavior for ports that have
no PCIe capabilities, 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 have 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>
---

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

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 38a41ccf79b9..e47901a3e880 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,18 @@ static int pcie_port_device_register(struct pci_dev *dev)
 	if (status) {
 		capabilities &= PCIE_PORT_SERVICE_HP;
 		if (!capabilities)
-			goto error_disable;
+			return 0;
 	}
 
 	/* 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++;
+		pcie_device_init(dev, service, irqs[i]);
 	}
-	if (!nr_service)
-		goto error_cleanup_irqs;
 
 	return 0;
-
-error_cleanup_irqs:
-	pci_free_irq_vectors(dev);
-error_disable:
-	pci_disable_device(dev);
-	return status;
 }
 
 typedef int (*pcie_callback_t)(struct pcie_device *);
-- 
2.52.0.457.g6b5491de43-goog

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

[Lukas Wunner]

On Fri, Jan 09, 2026 at 03:20:13PM -0800, Brian Norris wrote:
> @@ -355,29 +355,18 @@ static int pcie_port_device_register(struct pci_dev *dev)
>  	if (status) {
>  		capabilities &= PCIE_PORT_SERVICE_HP;
>  		if (!capabilities)
> -			goto error_disable;
> +			return 0;
>  	}

This will keep the Bus Master Enable bit set (see call to
pci_set_master() further up in the function), even though
no MSIs are expected from the device.  (I *think* these
would be the only memory writes that a port would perform.)

That doesn't seem right.  If there are no services, it seems
prudent to clear Bus Master Enable again (as is done by
pci_disable_device() right now).

>  	/* 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++;
> +		pcie_device_init(dev, service, irqs[i]);
>  	}
> -	if (!nr_service)
> -		goto error_cleanup_irqs;

Same here.  Why keep Bus Master Enable bit set and MSIs requested
if none of the port services probed successfully?

> 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, we allow probe to succeed even if no IRQs and no child services
> are available. This also mirrors existing behavior for ports that have
> no PCIe capabilities, where we'd also probe successfully.

Nit:  Please use imperative mood, i.e. "Thus, allow probe to succeed..."

Thanks,

Lukas

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

[Brian Norris]

Hi Lukas,

(FYI, I missed your email earlier because of errant spam filters. I'm
sure that's on my end somewhere, or maybe some over-eager DKIM stuff.
I only noticed when I checked lore... I'll try to keep my eyes open.)

On Sat, Jan 10, 2026 at 07:10:52AM +0100, Lukas Wunner wrote:
> On Fri, Jan 09, 2026 at 03:20:13PM -0800, Brian Norris wrote:
> > @@ -355,29 +355,18 @@ static int pcie_port_device_register(struct pci_dev *dev)
> >  	if (status) {
> >  		capabilities &= PCIE_PORT_SERVICE_HP;
> >  		if (!capabilities)
> > -			goto error_disable;
> > +			return 0;
> >  	}
> 
> This will keep the Bus Master Enable bit set (see call to
> pci_set_master() further up in the function), even though
> no MSIs are expected from the device.  (I *think* these
> would be the only memory writes that a port would perform.)
> 
> That doesn't seem right.  If there are no services, it seems
> prudent to clear Bus Master Enable again (as is done by
> pci_disable_device() right now).
> 
> >  	/* 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++;
> > +		pcie_device_init(dev, service, irqs[i]);
> >  	}
> > -	if (!nr_service)
> > -		goto error_cleanup_irqs;
> 
> Same here.  Why keep Bus Master Enable bit set and MSIs requested
> if none of the port services probed successfully?

Seems like a reasonable suggestion. I'll try pci_clear_master() in some
of these no-op non-failure cases.

Do you have the same concerns if pcie_init_service_irqs() falls back to
INTx but does not fail? It seems like a potentially fraught exercise to
guess what child services might need bus mastering though, so maybe it's
better to limit this only to nr_service==0 cases?

> > 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, we allow probe to succeed even if no IRQs and no child services
> > are available. This also mirrors existing behavior for ports that have
> > no PCIe capabilities, where we'd also probe successfully.
> 
> Nit:  Please use imperative mood, i.e. "Thus, allow probe to succeed..."

Ack.

Brian

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

[Lukas Wunner]

On Wed, Jan 14, 2026 at 11:30:19AM -0800, Brian Norris wrote:
> On Sat, Jan 10, 2026 at 07:10:52AM +0100, Lukas Wunner wrote:
> > On Fri, Jan 09, 2026 at 03:20:13PM -0800, Brian Norris wrote:
> > > @@ -355,29 +355,18 @@ static int pcie_port_device_register(struct pci_dev *dev)
> > >  	if (status) {
> > >  		capabilities &= PCIE_PORT_SERVICE_HP;
> > >  		if (!capabilities)
> > > -			goto error_disable;
> > > +			return 0;
> > >  	}
> > 
> > This will keep the Bus Master Enable bit set (see call to
> > pci_set_master() further up in the function), even though
> > no MSIs are expected from the device.  (I *think* these
> > would be the only memory writes that a port would perform.)
> > 
> > That doesn't seem right.  If there are no services, it seems
> > prudent to clear Bus Master Enable again (as is done by
> > pci_disable_device() right now).
> 
> Seems like a reasonable suggestion. I'll try pci_clear_master() in some
> of these no-op non-failure cases.
> 
> Do you have the same concerns if pcie_init_service_irqs() falls back to
> INTx but does not fail? It seems like a potentially fraught exercise to
> guess what child services might need bus mastering though, so maybe it's
> better to limit this only to nr_service==0 cases?

Sounds reasonable to me to constrain to nr_service==0.
Basically just retain the existing behavior.

I note that pcie_portdrv_remove() calls pci_disable_device()
unconditionally.  You may need an extra struct with an extra flag
to remember whether pci_disable_device() needs to be called on remove.

Thanks,

Lukas

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

[Brian Norris]

(Sorry for some delay. Other priorities take over sometimes...)

On Thu, Jan 15, 2026 at 09:44:40AM +0100, Lukas Wunner wrote:
> I note that pcie_portdrv_remove() calls pci_disable_device()
> unconditionally.  You may need an extra struct with an extra flag
> to remember whether pci_disable_device() needs to be called on remove.

Note that pci_enable_device() and pci_set_master() are separate
operations. With the proposed change, we may undo the latter, but not
the former. So remove() will still need to call pci_disable_device(),
which automatically handles clearing the master bit if needed, without
extra state tracking.

So I don't see the problem here.

At any rate, I've sent v2 that I think addresses everything. Feel free
to tell me I'm wrong there if needed :)

Brian

P.S. This whole concern is likely for naught if the port actually has a
device at the other end, since pci_enable_bridge() will set the master
bit anyway.