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

[PATCH v2] 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 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>
---

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

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

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 88af0dacf351..19b08f3653ee 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,22 @@ 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++;
+		pcie_device_init(dev, service, irqs[i]);
 	}
-	if (!nr_service)
-		goto error_cleanup_irqs;
 
+out:
+	/* With no child services, we shouldn't need bus mastering. */
+	if (!capabilities)
+		pci_clear_master(dev);
 	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.53.0.239.g8d8fc8a987-goog

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

[Bjorn Helgaas]

On Mon, Feb 09, 2026 at 05:15:35PM -0800, Brian Norris wrote:
> 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 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>
> ---
> 
> Changes in v2:
>  * clear master when we have no child services
> 
>  drivers/pci/pcie/portdrv.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 88af0dacf351..19b08f3653ee 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,22 @@ 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++;
> +		pcie_device_init(dev, service, irqs[i]);
>  	}
> -	if (!nr_service)
> -		goto error_cleanup_irqs;
>  
> +out:
> +	/* With no child services, we shouldn't need bus mastering. */
> +	if (!capabilities)
> +		pci_clear_master(dev);

I'm curious about this part because we pci_set_master()
unconditionally just above:

  pci_set_master(dev);
  pcie_init_service_irqs(dev, irqs, capabilities);
  for (i = 0; ...; i++)
    pcie_device_init(dev, service, irqs[i]);
  if (!capabilities)
    pci_clear_master(dev);

Bus mastering on a bridge must be enabled for DMA from downstream
devices to work, but I think that's done by pci_enable_bridge() when a
driver calls pci_enable_device() for an endpoint.

I assume the reason we call pci_set_master() here is so MSI/MSI-X from
the bridge will work, even if there is no downstream device.

I don't think either pcie_init_service_irqs() or pcie_device_init()
requires bus mastering, so I don't know why we enable it here.  It
seems like we should do it when we set up MSI/MSI-X interrupts.

If we want to do it in pcie_port_device_register() (instead of in
service driver when they set up an interrupt), maybe we should drop
the initial pci_set_master() and do it conditionally, e.g.,

  if (capabilities)
    pci_set_master(dev);

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

[Brian Norris]

Hi,

On Thu, Feb 19, 2026 at 04:25:14PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 09, 2026 at 05:15:35PM -0800, Brian Norris wrote:
> > 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 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>
> > ---
> > 
> > Changes in v2:
> >  * clear master when we have no child services
> > 
> >  drivers/pci/pcie/portdrv.c | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> > index 88af0dacf351..19b08f3653ee 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,22 @@ 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++;
> > +		pcie_device_init(dev, service, irqs[i]);
> >  	}
> > -	if (!nr_service)
> > -		goto error_cleanup_irqs;
> >  
> > +out:
> > +	/* With no child services, we shouldn't need bus mastering. */
> > +	if (!capabilities)
> > +		pci_clear_master(dev);
> 
> I'm curious about this part because we pci_set_master()
> unconditionally just above:
> 
>   pci_set_master(dev);
>   pcie_init_service_irqs(dev, irqs, capabilities);
>   for (i = 0; ...; i++)
>     pcie_device_init(dev, service, irqs[i]);
>   if (!capabilities)
>     pci_clear_master(dev);
> 
> Bus mastering on a bridge must be enabled for DMA from downstream
> devices to work, but I think that's done by pci_enable_bridge() when a
> driver calls pci_enable_device() for an endpoint.
> 
> I assume the reason we call pci_set_master() here is so MSI/MSI-X from
> the bridge will work, even if there is no downstream device.
> 
> I don't think either pcie_init_service_irqs() or pcie_device_init()
> requires bus mastering, so I don't know why we enable it here.  It
> seems like we should do it when we set up MSI/MSI-X interrupts.

I'm no expert here, but by code inspection, pcie_init_service_irqs() may
call pci_msi_set_enable() which sets PCI_MSI_FLAGS_ENABLE. Could that
confuse a device to see MSI enabled but bus mastering disabled?

Brian

> If we want to do it in pcie_port_device_register() (instead of in
> service driver when they set up an interrupt), maybe we should drop
> the initial pci_set_master() and do it conditionally, e.g.,
> 
>   if (capabilities)
>     pci_set_master(dev);

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

[Bjorn Helgaas]

On Thu, Feb 19, 2026 at 06:35:42PM -0800, Brian Norris wrote:
> On Thu, Feb 19, 2026 at 04:25:14PM -0600, Bjorn Helgaas wrote:
> > On Mon, Feb 09, 2026 at 05:15:35PM -0800, Brian Norris wrote:
> > > 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 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>
> > > ---
> > > 
> > > Changes in v2:
> > >  * clear master when we have no child services
> > > 
> > >  drivers/pci/pcie/portdrv.c | 21 +++++++--------------
> > >  1 file changed, 7 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> > > index 88af0dacf351..19b08f3653ee 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,22 @@ 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++;
> > > +		pcie_device_init(dev, service, irqs[i]);
> > >  	}
> > > -	if (!nr_service)
> > > -		goto error_cleanup_irqs;
> > >  
> > > +out:
> > > +	/* With no child services, we shouldn't need bus mastering. */
> > > +	if (!capabilities)
> > > +		pci_clear_master(dev);
> > 
> > I'm curious about this part because we pci_set_master()
> > unconditionally just above:
> > 
> >   pci_set_master(dev);
> >   pcie_init_service_irqs(dev, irqs, capabilities);
> >   for (i = 0; ...; i++)
> >     pcie_device_init(dev, service, irqs[i]);
> >   if (!capabilities)
> >     pci_clear_master(dev);
> > 
> > Bus mastering on a bridge must be enabled for DMA from downstream
> > devices to work, but I think that's done by pci_enable_bridge() when a
> > driver calls pci_enable_device() for an endpoint.
> > 
> > I assume the reason we call pci_set_master() here is so MSI/MSI-X from
> > the bridge will work, even if there is no downstream device.
> > 
> > I don't think either pcie_init_service_irqs() or pcie_device_init()
> > requires bus mastering, so I don't know why we enable it here.  It
> > seems like we should do it when we set up MSI/MSI-X interrupts.
> 
> I'm no expert here, but by code inspection, pcie_init_service_irqs() may
> call pci_msi_set_enable() which sets PCI_MSI_FLAGS_ENABLE. Could that
> confuse a device to see MSI enabled but bus mastering disabled?

You're right that this path may set the MSI Enable bit:

  pcie_init_service_irqs
    pcie_port_enable_irq_vec
      pci_alloc_irq_vectors
        pci_alloc_irq_vectors_affinity
          __pci_enable_msi_range
            msi_capability_init
              __msi_capability_init
                pci_msi_set_enable(1)
                  # set PCI_MSI_FLAGS_ENABLE

Setting MSI Enable allows the device to use MSI when it asserts an
interrupt, but my understanding is that it doesn't enable the
interrupt source itself.  The MSI Capability is an interrupt
mechanism, not itself a source -- there's no interrupt handler at this
point.

The sources have their own interrupt enable bits, e.g, Root Control
PME Interrupt Enable, Link Control Link Bandwidth Management Interrupt
Enable, Slot Control Hot-Plug Interrupt Enable, DPC Interrupt Enable,
AER Root Error Command Error Reporting Enable bits, 

The drivers using these interrupts should be setting up their
interrupt handlers before setting their interrupt enable bit.

Bus master is a global thing that affects all kinds of transactions
coming from the device.  There's no problem setting in from various
places, but clearing it in a single place affects all of them, so we
basically need global knowledge to know that it's safe.  That's why I
suggested doing a conditional bus master enable instead of the
unconditional enable followed by a conditional disable.

But I still do think this is not quite the right place even to enable
it.  I think it would make more sense to enable bus mastering in the
drivers that need the MSI/MSI-X, e.g., PME, pciehp, bwctrl, aer, etc.
At this point in pcie_port_device_register(), I don't think we can be
certain that any of those services will actually enable the interrupt.

> > If we want to do it in pcie_port_device_register() (instead of in
> > service driver when they set up an interrupt), maybe we should drop
> > the initial pci_set_master() and do it conditionally, e.g.,
> > 
> >   if (capabilities)
> >     pci_set_master(dev);

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

[Brian Norris]

Hi Bjorn,

Sorry, I've gotten pretty busy, so this didn't top my list of things to
try to reply to for a while. But thanks for your review! See below.

On Fri, Feb 20, 2026 at 10:40:46AM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 19, 2026 at 06:35:42PM -0800, Brian Norris wrote:
> > On Thu, Feb 19, 2026 at 04:25:14PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Feb 09, 2026 at 05:15:35PM -0800, Brian Norris wrote:
> > > > +out:
> > > > +	/* With no child services, we shouldn't need bus mastering. */
> > > > +	if (!capabilities)
> > > > +		pci_clear_master(dev);
> > > 
> > > I'm curious about this part because we pci_set_master()
> > > unconditionally just above:
> > > 
> > >   pci_set_master(dev);
> > >   pcie_init_service_irqs(dev, irqs, capabilities);
> > >   for (i = 0; ...; i++)
> > >     pcie_device_init(dev, service, irqs[i]);
> > >   if (!capabilities)
> > >     pci_clear_master(dev);
> > > 
> > > Bus mastering on a bridge must be enabled for DMA from downstream
> > > devices to work, but I think that's done by pci_enable_bridge() when a
> > > driver calls pci_enable_device() for an endpoint.
> > > 
> > > I assume the reason we call pci_set_master() here is so MSI/MSI-X from
> > > the bridge will work, even if there is no downstream device.
> > > 
> > > I don't think either pcie_init_service_irqs() or pcie_device_init()
> > > requires bus mastering, so I don't know why we enable it here.  It
> > > seems like we should do it when we set up MSI/MSI-X interrupts.
> > 
> > I'm no expert here, but by code inspection, pcie_init_service_irqs() may
> > call pci_msi_set_enable() which sets PCI_MSI_FLAGS_ENABLE. Could that
> > confuse a device to see MSI enabled but bus mastering disabled?
> 
> You're right that this path may set the MSI Enable bit:
> 
>   pcie_init_service_irqs
>     pcie_port_enable_irq_vec
>       pci_alloc_irq_vectors
>         pci_alloc_irq_vectors_affinity
>           __pci_enable_msi_range
>             msi_capability_init
>               __msi_capability_init
>                 pci_msi_set_enable(1)
>                   # set PCI_MSI_FLAGS_ENABLE
> 
> Setting MSI Enable allows the device to use MSI when it asserts an
> interrupt, but my understanding is that it doesn't enable the
> interrupt source itself.  The MSI Capability is an interrupt
> mechanism, not itself a source -- there's no interrupt handler at this
> point.
> 
> The sources have their own interrupt enable bits, e.g, Root Control
> PME Interrupt Enable, Link Control Link Bandwidth Management Interrupt
> Enable, Slot Control Hot-Plug Interrupt Enable, DPC Interrupt Enable,
> AER Root Error Command Error Reporting Enable bits, 
> 
> The drivers using these interrupts should be setting up their
> interrupt handlers before setting their interrupt enable bit.

Ack, makes sense.

> Bus master is a global thing that affects all kinds of transactions
> coming from the device.  There's no problem setting in from various
> places, but clearing it in a single place affects all of them, so we
> basically need global knowledge to know that it's safe.  That's why I
> suggested doing a conditional bus master enable instead of the
> unconditional enable followed by a conditional disable.
> 
> But I still do think this is not quite the right place even to enable
> it.  I think it would make more sense to enable bus mastering in the
> drivers that need the MSI/MSI-X, e.g., PME, pciehp, bwctrl, aer, etc.
> At this point in pcie_port_device_register(), I don't think we can be
> certain that any of those services will actually enable the interrupt.

I'm not quite sure what to say. I agree that in some sense, we (portdrv)
are not the ones to know who's going to use bus mastering; and so moving
that decision into child services makes a little sense. But then, we
punt the problem of what to do on failure or unwind -- if any child
enables bus mastering, it won't know whether it's ever safe to disable
it on (a) subsequent error/unwind (say, in probe()); or (b) remove().

So is pci_set_master() a one-way operation done in child services (never
call pci_clear_master()), and the only clear is via
pcie_portdrv_remove() -> pci_disable_device()?

Brian

> > > If we want to do it in pcie_port_device_register() (instead of in
> > > service driver when they set up an interrupt), maybe we should drop
> > > the initial pci_set_master() and do it conditionally, e.g.,
> > > 
> > >   if (capabilities)
> > >     pci_set_master(dev);

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

[Brian Norris]

On Thu, Apr 09, 2026 at 04:20:27PM -0700, Brian Norris wrote:
> So is pci_set_master() a one-way operation done in child services (never
> call pci_clear_master()), and the only clear is via
> pcie_portdrv_remove() -> pci_disable_device()?

One other thought about this: it's actually a non-trivial question as to
whether the port is actually going to use MSI (and therefore, need bus
mastering). It depends on the result of pcie_init_service_irqs(),
whether we succeed with MSI or INTx.

We don't actually attempt to answer that question today. But if we did,
we'd have to be scattering that code across all our IRQ-utilizing child
services.

I wonder if all that churn is worthwhile, when it's a relatively
rare/strange case that a port really does *not* want bus mastering
enabled.

Brian