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

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, [email protected] 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 <[email protected]>
> > 
> > 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/[email protected]/
> 
> 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

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