Re: reset_slot() callback not respecting MPS config

[Lukas Wunner <lukas@wunner.de>]

On Fri, May 23, 2025 at 12:09:06PM +0530, Manivannan Sadhasivam wrote:
> On Fri, May 23, 2025 at 07:33:16AM +0200, Lukas Wunner wrote:
> > On Thu, May 22, 2025 at 06:19:56PM +0200, Niklas Cassel wrote:
> > > As you know the reset_slot() callback patches were merged recently.
> > > 
> > > Wilfred and I (mostly Wilfred), have been debugging DMA issues after the
> > > reset_slot() callback has been invoked. The issue is reproduced when MPS
> > > configuration is set to performance, but might be applicable for other
> > > MPS configurations as well. The problem appears to be that reset_slot()
> > > feature does not respect/restore the MPS configuration.
> > 
> > The Device Control register (and thus the MPS setting) is saved via:
> > 
> >   pci_save_state()
> >     pci_save_pcie_state()
> > 
> > So either you're missing a call to pci_restore_state() after reset,
> > or you're missing a call to pci_save_state() after changing MPS,
> > or MPS is somehow overwritten after pci_restore_state().
> 
> I think the issue is that the PCI bridge is getting reset while trying
> to reset the PCI device. And in the reset path, we only save the config
> space of the *device*, not the bridge.
> 
> As seen from the lspci output shared by Niklas, the content of the PCI
> bridge seem to be diverged. Since the reset_slot() callback resets the
> whole Root Complex (if there is a single Root port), then the config
> space of the Root port/bridge needs to be saved and restored as well.
> 
> I believe pcibios_reset_secondary_bus() is not supposed to change the
> config space of the root port. As per the definition of the "Secondary
> Bus Reset" field in the Bridge Control Register, r3.0, sec 7.5.3.6:
> 
> "Port configuration registers must not be changed, except as required
> to update Port status."
> 
> So pci_reset_secondary_bus() is not changing the config space,
> but reset_slot() does. Are we plugging reset_slot() at the wrong place?

On ACPI-based platforms (x86 etc), I'm not aware that it's possible
to reset the Root Complex.  If it is, I don't think we've exposed
that feature and hence we don't really have a better place to hook
into.

There's the pci_reset_fn_methods[] array and conceivably, an entry
could be added there to reset the Root Port on capable platforms.
However that array is meant to reset a single PCI function,
whereas the ->reset_slot() also resets the entire hierarchy below
the Root Port (IIUC).  So that's not really what the array is
meant to be used for.

You wanted to use ->reset_slot() for aer_root_reset().  It performs
a Secondary Bus Reset via:

  pci_bus_error_reset()
    pci_bus_reset()
      pci_bridge_secondary_bus_reset()

or:

  pci_bus_error_reset()
    pci_slot_reset()
      pci_reset_hotplug_slot()
        hotplug->ops->reset_slot()
	  pciehp_reset_slot()      # or other hotplug driver
	    pci_bridge_secondary_bus_reset()

...and that's the reason I suggested to plumb ->reset_slot()
into pcibios_reset_secondary_bus().  I don't think we have
a better place.

If all host bridge drivers reset the Root Complex as part of
->reset_slot(), then it should be fine to just call
pci_save_state(dev) before and pci_restore_state(dev) after
invoking host->reset_slot() in pcibios_reset_secondary_bus().

If however this behavior is specific only to certain host
bridge drivers, then you want to call pci_save_state() and
pci_restore_state() directly in their ->reset_slot()
implementations.

I note that if you have a deeper hierarchy with PCIe switches
below the host bridge, you'll reset the Root Complex even if
the error was reported further down in the hierarchy by some
Switch Downstream Port.  I think in that case you may not
want to reset the Root Complex, but only perform a Secondary
Bus Reset at that Downstream Port.  In other words,
I'm wondering if pcibios_reset_secondary_bus() should invoke
host->reset_slot() only if dev is a Root Port / is sitting
on the root bus.

I'm also wondering if ->reset_slot() should be renamed to
something like ->reset_root_complex() or ->reset_root_port()
or somesuch to more aptly describe what it does.
I guess the name ->reset_slot() came about because these
Root Complexes typically consist of a single Root Port with
a single slot.

Thanks,

Lukas