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

[Brian Norris <briannorris@chromium.org>]

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