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

[Brian Norris <briannorris@chromium.org>]

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);