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

[PATCH v3] 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>
---
I didn't get an answer to my responses to the last review feedback:
https://lore.kernel.org/all/adg5IPWdCSv1UqFE@google.com/
So I've attempted to address the feedback without churning all the port
service drivers.

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 | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 2d6aa488fe7b..a79a52560849 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 */
@@ -343,7 +343,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
 	if (!capabilities)
 		return 0;
 
-	pci_set_master(dev);
 	/*
 	 * Initialize service irqs. Don't use service devices that
 	 * require interrupts if there is no way to generate them.
@@ -355,29 +354,20 @@ 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;
+	pci_set_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.54.0.563.g4f69b47b94-goog

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving `pci_set_master()` after child device registration causes child drivers to probe with Bus Mastering disabled, risking permanently lost MSI/MSI-X interrupts.
--

commit 9b851bf8faf271ab0b2daad51bfd51f89a1e87d4
Author: Brian Norris <briannorris@chromium.org>

PCI/portdrv: Allow probing even without child services

This commit modifies the PCIe port driver to allow probe to succeed even
if no child services are available. This ensures the port can still be
configured for runtime PM and low power modes on systems that only have
certain port services available or lack legacy INTx interrupts.

Link: https://lore.kernel.org/all/nyada24tqwlkzdceyoxbzitzygvp4elvj5oajnqdwb33xkcdwk@76vnrx45fsfd/

> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -343,7 +343,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
>  	if (!capabilities)
>  		return 0;
>  
> -	pci_set_master(dev);
>  	/*
>  	 * Initialize service irqs. Don't use service devices that
>  	 * require interrupts if there is no way to generate them.

[ ... ]

>  	/* 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;
> +	pci_set_master(dev);

Does moving pci_set_master() after the pcie_device_init() loop risk dropping
MSI/MSI-X interrupts during child driver probes?

The pcie_device_init() function calls device_register(), which synchronously
invokes the probe routines of child service drivers like pciehp or aer.
During probe, these child drivers configure the hardware and enable their
interrupts.

Because PCIe MSI and MSI-X interrupts are memory write transactions, they
require the Bus Master Enable (BME) bit to be set in the PCI Command Register.

If a hardware event occurs immediately after the child driver unmasks the
interrupt but before pci_set_master(dev) is called, will the hardware attempt
to send an MSI and drop the memory write because BME is 0?

Since PCIe interrupts are edge-triggered messages, could these dropped
interrupts result in permanently missed hotplug events or cause hardware error
handling regressions?

> -error_cleanup_irqs:
> -	pci_free_irq_vectors(dev);
> -error_disable:
> -	pci_disable_device(dev);
> -	return status;
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508133629.v3.1.I5fd5d83f518681b3949d8ab2f16ba8244fd3e774@changeid?part=1

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

[Brian Norris]

Hi sashiko-bot and Bjorn,

On Fri, May 08, 2026 at 11:30:22PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Moving `pci_set_master()` after child device registration causes child drivers to probe with Bus Mastering disabled, risking permanently lost MSI/MSI-X interrupts.
> --
> 
> commit 9b851bf8faf271ab0b2daad51bfd51f89a1e87d4
> Author: Brian Norris <briannorris@chromium.org>
> 
> PCI/portdrv: Allow probing even without child services
> 
> This commit modifies the PCIe port driver to allow probe to succeed even
> if no child services are available. This ensures the port can still be
> configured for runtime PM and low power modes on systems that only have
> certain port services available or lack legacy INTx interrupts.
> 
> Link: https://lore.kernel.org/all/nyada24tqwlkzdceyoxbzitzygvp4elvj5oajnqdwb33xkcdwk@76vnrx45fsfd/
> 
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -343,7 +343,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
> >  	if (!capabilities)
> >  		return 0;
> >  
> > -	pci_set_master(dev);
> >  	/*
> >  	 * Initialize service irqs. Don't use service devices that
> >  	 * require interrupts if there is no way to generate them.
> 
> [ ... ]
> 
> >  	/* 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;
> > +	pci_set_master(dev);
> 
> Does moving pci_set_master() after the pcie_device_init() loop risk dropping
> MSI/MSI-X interrupts during child driver probes?
> 
> The pcie_device_init() function calls device_register(), which synchronously
> invokes the probe routines of child service drivers like pciehp or aer.
> During probe, these child drivers configure the hardware and enable their
> interrupts.
> 
> Because PCIe MSI and MSI-X interrupts are memory write transactions, they
> require the Bus Master Enable (BME) bit to be set in the PCI Command Register.
> 
> If a hardware event occurs immediately after the child driver unmasks the
> interrupt but before pci_set_master(dev) is called, will the hardware attempt
> to send an MSI and drop the memory write because BME is 0?
> 
> Since PCIe interrupts are edge-triggered messages, could these dropped
> interrupts result in permanently missed hotplug events or cause hardware error
> handling regressions?

Ha, that's funny. It's all a great question I think, and that's pretty
much what I asked Bjorn here when he suggested moving this:

https://lore.kernel.org/all/aZfIfn9viQRj4uy4@google.com/

He seems to think it's not a problem:

https://lore.kernel.org/all/20260220164046.GA3528004@bhelgaas/

but I'm frankly not very convinced. I think it's perfectly possible that
a child service will try to enable MSI, the device will try to write,
and those writes will be dropped, producing a missing interrupt.

Bjorn, what do you think? Personally, I'd go back to something closer to
v2, where we enable mastering before initializing children. But maybe
you know better than this bot and me.

Frankly, this is all a distraction from the actual goal of my patch, so
I'd rather not make arbitrary changes to the ordering if we're not sure.

Brian

> > -error_cleanup_irqs:
> > -	pci_free_irq_vectors(dev);
> > -error_disable:
> > -	pci_disable_device(dev);
> > -	return status;
> > +	return 0;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260508133629.v3.1.I5fd5d83f518681b3949d8ab2f16ba8244fd3e774@changeid?part=1

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

[David Matlack]

On Fri, May 8, 2026 at 1:36 PM Brian Norris <briannorris@chromium.org> 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>

Hi Brian,

I am interested in this support as well for Live Update scenarios [1].
Allowing pcieport to probe avoids disabling bus mastering during boot
on preserved bridges (failed probe calls  pci_disable_device()). Bus
Mastering is eventually re-enabled when a downstream device calls
pci_enable_device(), but during the window that it is disabled
downstream devices cannot perform any memory transactions.

[1] https://lore.kernel.org/linux-pci/20260522202410.3104264-1-dmatlack@google.com/

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

[Manivannan Sadhasivam]

On Fri, May 08, 2026 at 05:12:19PM -0700, Brian Norris wrote:
> Hi sashiko-bot and Bjorn,
> 
> On Fri, May 08, 2026 at 11:30:22PM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Moving `pci_set_master()` after child device registration causes child drivers to probe with Bus Mastering disabled, risking permanently lost MSI/MSI-X interrupts.
> > --
> > 
> > commit 9b851bf8faf271ab0b2daad51bfd51f89a1e87d4
> > Author: Brian Norris <briannorris@chromium.org>
> > 
> > PCI/portdrv: Allow probing even without child services
> > 
> > This commit modifies the PCIe port driver to allow probe to succeed even
> > if no child services are available. This ensures the port can still be
> > configured for runtime PM and low power modes on systems that only have
> > certain port services available or lack legacy INTx interrupts.
> > 
> > Link: https://lore.kernel.org/all/nyada24tqwlkzdceyoxbzitzygvp4elvj5oajnqdwb33xkcdwk@76vnrx45fsfd/
> > 
> > > --- a/drivers/pci/pcie/portdrv.c
> > > +++ b/drivers/pci/pcie/portdrv.c
> > > @@ -343,7 +343,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
> > >  	if (!capabilities)
> > >  		return 0;
> > >  
> > > -	pci_set_master(dev);
> > >  	/*
> > >  	 * Initialize service irqs. Don't use service devices that
> > >  	 * require interrupts if there is no way to generate them.
> > 
> > [ ... ]
> > 
> > >  	/* 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;
> > > +	pci_set_master(dev);
> > 
> > Does moving pci_set_master() after the pcie_device_init() loop risk dropping
> > MSI/MSI-X interrupts during child driver probes?
> > 
> > The pcie_device_init() function calls device_register(), which synchronously
> > invokes the probe routines of child service drivers like pciehp or aer.
> > During probe, these child drivers configure the hardware and enable their
> > interrupts.
> > 
> > Because PCIe MSI and MSI-X interrupts are memory write transactions, they
> > require the Bus Master Enable (BME) bit to be set in the PCI Command Register.
> > 
> > If a hardware event occurs immediately after the child driver unmasks the
> > interrupt but before pci_set_master(dev) is called, will the hardware attempt
> > to send an MSI and drop the memory write because BME is 0?
> > 
> > Since PCIe interrupts are edge-triggered messages, could these dropped
> > interrupts result in permanently missed hotplug events or cause hardware error
> > handling regressions?
> 
> Ha, that's funny. It's all a great question I think, and that's pretty
> much what I asked Bjorn here when he suggested moving this:
> 
> https://lore.kernel.org/all/aZfIfn9viQRj4uy4@google.com/
> 
> He seems to think it's not a problem:
> 
> https://lore.kernel.org/all/20260220164046.GA3528004@bhelgaas/
> 
> but I'm frankly not very convinced. I think it's perfectly possible that
> a child service will try to enable MSI, the device will try to write,
> and those writes will be dropped, producing a missing interrupt.
> 
> Bjorn, what do you think? Personally, I'd go back to something closer to
> v2, where we enable mastering before initializing children. But maybe
> you know better than this bot and me.
> 
> Frankly, this is all a distraction from the actual goal of my patch, so
> I'd rather not make arbitrary changes to the ordering if we're not sure.
> 

I do agree that this is a valid concern. Quoting Bjorn's reply in v2
https://lore.kernel.org/linux-pci/20260220164046.GA3528004@bhelgaas/:

"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."

This seems not to be true because, pcie_device_init() can invoke the service
driver's probe() like aer_probe() and the probe callback can register the IRQ
handler for the service IRQs and enable the MSIs. So once the IRQ handler is
registered and relevant MSIs are enabled, then we ought to service the MSIs from
the endpoint. But that won't happen because of BME not getting set at this
point and leading to those MSIs getting lost.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

[+cc David, Mani]

On Fri, May 08, 2026 at 05:12:19PM -0700, Brian Norris wrote:
> On Fri, May 08, 2026 at 11:30:22PM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Moving `pci_set_master()` after child device registration causes child drivers to probe with Bus Mastering disabled, risking permanently lost MSI/MSI-X interrupts.
> > --
> > 
> > commit 9b851bf8faf271ab0b2daad51bfd51f89a1e87d4
> > Author: Brian Norris <briannorris@chromium.org>
> > 
> > PCI/portdrv: Allow probing even without child services
> > 
> > This commit modifies the PCIe port driver to allow probe to succeed even
> > if no child services are available. This ensures the port can still be
> > configured for runtime PM and low power modes on systems that only have
> > certain port services available or lack legacy INTx interrupts.
> > 
> > Link: https://lore.kernel.org/all/nyada24tqwlkzdceyoxbzitzygvp4elvj5oajnqdwb33xkcdwk@76vnrx45fsfd/
> > 
> > > --- a/drivers/pci/pcie/portdrv.c
> > > +++ b/drivers/pci/pcie/portdrv.c
> > > @@ -343,7 +343,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
> > >  	if (!capabilities)
> > >  		return 0;
> > >  
> > > -	pci_set_master(dev);
> > >  	/*
> > >  	 * Initialize service irqs. Don't use service devices that
> > >  	 * require interrupts if there is no way to generate them.
> > 
> > [ ... ]
> > 
> > >  	/* 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;
> > > +	pci_set_master(dev);
> > 
> > Does moving pci_set_master() after the pcie_device_init() loop
> > risk dropping MSI/MSI-X interrupts during child driver probes?
> > 
> > The pcie_device_init() function calls device_register(), which
> > synchronously invokes the probe routines of child service drivers
> > like pciehp or aer.  During probe, these child drivers configure
> > the hardware and enable their interrupts.
> > 
> > Because PCIe MSI and MSI-X interrupts are memory write
> > transactions, they require the Bus Master Enable (BME) bit to be
> > set in the PCI Command Register.
> > 
> > If a hardware event occurs immediately after the child driver
> > unmasks the interrupt but before pci_set_master(dev) is called,
> > will the hardware attempt to send an MSI and drop the memory write
> > because BME is 0?
> > 
> > Since PCIe interrupts are edge-triggered messages, could these
> > dropped interrupts result in permanently missed hotplug events or
> > cause hardware error handling regressions?
> 
> Ha, that's funny. It's all a great question I think, and that's
> pretty much what I asked Bjorn here when he suggested moving this:
> 
> https://lore.kernel.org/all/aZfIfn9viQRj4uy4@google.com/
> 
> He seems to think it's not a problem:
> 
> https://lore.kernel.org/all/20260220164046.GA3528004@bhelgaas/
> 
> but I'm frankly not very convinced. I think it's perfectly possible
> that a child service will try to enable MSI, the device will try to
> write, and those writes will be dropped, producing a missing
> interrupt.

I think you're right that if we move the pci_set_master() after
pcie_device_init(), there is a window where MSIs could be dropped.

If pcie_portdrv_probe() is called after pcie_portdrv_init() registers
service drivers (which would certainly happen if a port is hot-added)
the service .probe() will be called inside pcie_device_init(), so BME
must be set before that.

  pcie_portdrv_init                   # device_initcall
    pcie_init_services
      pcie_aer_init
        pcie_port_service_register
          driver_register

  pcie_portdrv_probe
    pcie_port_device_register
      pci_set_master                  # current location
      pcie_init_service_irqs
        # set generic PCI_MSI_FLAGS_ENABLE
      for (i = 0; ...)
        pcie_device_init
          device_register
            aer_probe
              # set PCI_ERR_ROOT_CMD_COR_EN interrupt enable

> Bjorn, what do you think? Personally, I'd go back to something
> closer to v2, where we enable mastering before initializing
> children.

Yes, I agree.  What do you think of the patch below?  It's fairly
similar to your v2.

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 69283cd04a78..6912743df767 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -328,7 +328,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
  * Allocate the port extension structure and register services associated with
  * the port.
  */
-static int pcie_port_device_register(struct pci_dev *dev)
+static void pcie_port_device_register(struct pci_dev *dev)
 {
 	int status, capabilities, i, nr_service;
 	int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
@@ -336,12 +336,12 @@ static int pcie_port_device_register(struct pci_dev *dev)
 	/* Enable PCI Express port device */
 	status = pci_enable_device(dev);
 	if (status)
-		return status;
+		return;
 
 	/* Get and check PCI Express port services */
 	capabilities = get_port_device_capability(dev);
 	if (!capabilities)
-		return 0;
+		return;
 
 	pci_set_master(dev);
 	/*
@@ -359,7 +359,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
 	}
 
 	/* Allocate child services if any */
-	status = -ENODEV;
 	nr_service = 0;
 	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
 		int service = 1 << i;
@@ -368,16 +367,16 @@ static int pcie_port_device_register(struct pci_dev *dev)
 		if (!pcie_device_init(dev, service, irqs[i]))
 			nr_service++;
 	}
+
 	if (!nr_service)
 		goto error_cleanup_irqs;
 
-	return 0;
+	return;
 
 error_cleanup_irqs:
 	pci_free_irq_vectors(dev);
 error_disable:
 	pci_disable_device(dev);
-	return status;
 }
 
 typedef int (*pcie_callback_t)(struct pcie_device *);
@@ -698,9 +697,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	if (type == PCI_EXP_TYPE_RC_EC)
 		pcie_link_rcec(dev);
 
-	status = pcie_port_device_register(dev);
-	if (status)
-		return status;
+	pcie_port_device_register(dev);
 
 	pci_save_state(dev);
 

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

[Brian Norris]

Hi Bjorn,

On Wed, Jul 15, 2026 at 05:15:26PM -0500, Bjorn Helgaas wrote:
> [+cc David, Mani]
> 
> On Fri, May 08, 2026 at 05:12:19PM -0700, Brian Norris wrote:
> > On Fri, May 08, 2026 at 11:30:22PM +0000, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [High] Moving `pci_set_master()` after child device registration causes child drivers to probe with Bus Mastering disabled, risking permanently lost MSI/MSI-X interrupts.
> > > --
> > > 
> > > commit 9b851bf8faf271ab0b2daad51bfd51f89a1e87d4
> > > Author: Brian Norris <briannorris@chromium.org>
> > > 
> > > PCI/portdrv: Allow probing even without child services
> > > 
> > > This commit modifies the PCIe port driver to allow probe to succeed even
> > > if no child services are available. This ensures the port can still be
> > > configured for runtime PM and low power modes on systems that only have
> > > certain port services available or lack legacy INTx interrupts.
> > > 
> > > Link: https://lore.kernel.org/all/nyada24tqwlkzdceyoxbzitzygvp4elvj5oajnqdwb33xkcdwk@76vnrx45fsfd/
> > > 
> > > > --- a/drivers/pci/pcie/portdrv.c
> > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > @@ -343,7 +343,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
> > > >  	if (!capabilities)
> > > >  		return 0;
> > > >  
> > > > -	pci_set_master(dev);
> > > >  	/*
> > > >  	 * Initialize service irqs. Don't use service devices that
> > > >  	 * require interrupts if there is no way to generate them.
> > > 
> > > [ ... ]
> > > 
> > > >  	/* 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;
> > > > +	pci_set_master(dev);
> > > 
> > > Does moving pci_set_master() after the pcie_device_init() loop
> > > risk dropping MSI/MSI-X interrupts during child driver probes?
> > > 
> > > The pcie_device_init() function calls device_register(), which
> > > synchronously invokes the probe routines of child service drivers
> > > like pciehp or aer.  During probe, these child drivers configure
> > > the hardware and enable their interrupts.
> > > 
> > > Because PCIe MSI and MSI-X interrupts are memory write
> > > transactions, they require the Bus Master Enable (BME) bit to be
> > > set in the PCI Command Register.
> > > 
> > > If a hardware event occurs immediately after the child driver
> > > unmasks the interrupt but before pci_set_master(dev) is called,
> > > will the hardware attempt to send an MSI and drop the memory write
> > > because BME is 0?
> > > 
> > > Since PCIe interrupts are edge-triggered messages, could these
> > > dropped interrupts result in permanently missed hotplug events or
> > > cause hardware error handling regressions?
> > 
> > Ha, that's funny. It's all a great question I think, and that's
> > pretty much what I asked Bjorn here when he suggested moving this:
> > 
> > https://lore.kernel.org/all/aZfIfn9viQRj4uy4@google.com/
> > 
> > He seems to think it's not a problem:
> > 
> > https://lore.kernel.org/all/20260220164046.GA3528004@bhelgaas/
> > 
> > but I'm frankly not very convinced. I think it's perfectly possible
> > that a child service will try to enable MSI, the device will try to
> > write, and those writes will be dropped, producing a missing
> > interrupt.
> 
> I think you're right that if we move the pci_set_master() after
> pcie_device_init(), there is a window where MSIs could be dropped.
> 
> If pcie_portdrv_probe() is called after pcie_portdrv_init() registers
> service drivers (which would certainly happen if a port is hot-added)
> the service .probe() will be called inside pcie_device_init(), so BME
> must be set before that.
> 
>   pcie_portdrv_init                   # device_initcall
>     pcie_init_services
>       pcie_aer_init
>         pcie_port_service_register
>           driver_register
> 
>   pcie_portdrv_probe
>     pcie_port_device_register
>       pci_set_master                  # current location
>       pcie_init_service_irqs
>         # set generic PCI_MSI_FLAGS_ENABLE
>       for (i = 0; ...)
>         pcie_device_init
>           device_register
>             aer_probe
>               # set PCI_ERR_ROOT_CMD_COR_EN interrupt enable
> 
> > Bjorn, what do you think? Personally, I'd go back to something
> > closer to v2, where we enable mastering before initializing
> > children.
> 
> Yes, I agree.  What do you think of the patch below?  It's fairly
> similar to your v2.

Thanks for looking! Can you describe what your goals are here vs my v2?
I'm curious what you're aiming for.

I also think there are a few problems, notes below.

> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 69283cd04a78..6912743df767 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -328,7 +328,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
>   * Allocate the port extension structure and register services associated with
>   * the port.
>   */
> -static int pcie_port_device_register(struct pci_dev *dev)
> +static void pcie_port_device_register(struct pci_dev *dev)
>  {
>  	int status, capabilities, i, nr_service;
>  	int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
> @@ -336,12 +336,12 @@ static int pcie_port_device_register(struct pci_dev *dev)
>  	/* Enable PCI Express port device */
>  	status = pci_enable_device(dev);
>  	if (status)
> -		return status;
> +		return;

Are you purposely ignoring pci_enable_state() failures now too? That
wasn't part of my original proposal. This also means you have a
potential underflow in remove(), because now a port might get through
probe() with an enable_cnt of 0 -- then we still call
pci_disable_device() in remove().

>  
>  	/* Get and check PCI Express port services */
>  	capabilities = get_port_device_capability(dev);
>  	if (!capabilities)
> -		return 0;
> +		return;
>  
>  	pci_set_master(dev);
>  	/*
> @@ -359,7 +359,6 @@ static int pcie_port_device_register(struct pci_dev *dev)
>  	}
>  
>  	/* Allocate child services if any */
> -	status = -ENODEV;
>  	nr_service = 0;
>  	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
>  		int service = 1 << i;
> @@ -368,16 +367,16 @@ static int pcie_port_device_register(struct pci_dev *dev)
>  		if (!pcie_device_init(dev, service, irqs[i]))
>  			nr_service++;
>  	}
> +
>  	if (!nr_service)
>  		goto error_cleanup_irqs;
>  
> -	return 0;
> +	return;
>  
>  error_cleanup_irqs:
>  	pci_free_irq_vectors(dev);

I didn't look at all the implications, but now we might run
pci_free_irq_vectors() twice in a row -- here, and in remove(). I think
it might be safe, but it's not trivial to ensure that.

>  error_disable:
>  	pci_disable_device(dev);

By unwinding this here, we have another potential case of inducing
underflow in remove(), similar to the pci_enable_device() failure case I
described above.

Brian

> -	return status;
>  }
>  
>  typedef int (*pcie_callback_t)(struct pcie_device *);
> @@ -698,9 +697,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	if (type == PCI_EXP_TYPE_RC_EC)
>  		pcie_link_rcec(dev);
>  
> -	status = pcie_port_device_register(dev);
> -	if (status)
> -		return status;
> +	pcie_port_device_register(dev);
>  
>  	pci_save_state(dev);
>