Re: [PATCH] PCI: Align MPS to upstream bridge for SAFE and PERFORMANCE mode

[Tyler Hicks <code@tyhicks.com>]

On 2022-10-20 15:24:37, Bjorn Helgaas wrote:
> On Wed, Oct 19, 2022 at 01:25:59PM -0500, Tyler Hicks wrote:
> > On 2022-06-10 23:01:31, Zhiqiang Hou wrote:
> > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > 
> > > The commit 27d868b5e6cf ("PCI: Set MPS to match upstream bridge")
> > > made the device's MPS matches upstream bridge for PCIE_BUS_DEFAULT
> > > mode, so that it's more likely that a hot-added device will work in
> > > a system with an optimized MPS configuration.
> > > 
> > > Obviously, the Linux itself optimizes the MPS settings in the
> > > PCIE_BUS_SAFE and PCIE_BUS_PERFORMANCE mode, so let's do this also
> > > for these modes.
> > > 
> > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > 
> > I wanted to give a little more information about the issue we're seeing.
> > We're having trouble retaining the optimized Max Payload Size (MPS)
> > value of a PCIe endpoint after hotplug/rescan. In this case,
> > `pcie_ports=native` and `pci=pcie_bus_safe` are set on the cmdline and
> > we expect the Linux kernel to retain the MPS value. Commit 27d868b5e6cf
> > preserved the MPS value when using the default PCIe bus mode
> > (PCIE_BUS_DEFAULT) and we're hopeful that this can be extended to other
> > modes, as well.
> 
> Thanks, Tyler.  I need help understanding what's going on here.  An
> example of the topology and what happens and what *should* happen
> might help.

Hey Bjorn and Keith! Thanks for both of your responses and your
patience. They spurred good investigations on my side and I'm learning a
lot as I go.

> Some MPS configuration is done per-device in pci_configure_mps(), and
> some considers a hierarchy in pcie_bus_configure_settings().  In the
> current tree, in the PCIE_BUS_SAFE case:
> 
>   - pci_configure_mps() does nothing (except for RCiEPs).
> 
>   - pcie_bus_configure_settings(bus) looks at the hierarchy rooted at
>     the bridge leading to "bus".  If the hierarchy contains a hotplug
>     Switch Downstream Port, it sets MPS and MRRS to 128 for
>     everything.
> 
>     If it does not contain such a bridge, it finds the smallest
>     MPS_Supported ("smpss") of any device in the hierarchy and sets
>     MPS and MRRS to that for everything.
> 
> If you boot with a hotplug Root Port leading to an empty slot, I think
> the RP MPS will end up being whatever BIOS put there.

I've been talking to the firmware folks on why SAFE mode was selected,
based on Keith's question from Wednesday. From what I've been told,
U-Boot doesn't seed the RP MPS with a value so the kernel sees a value
of 128 for p_mps in pci_configure_mps() when using the DEFAULT mode.
Apparently UEFI does seed the RP MPS but we don't get that with U-Boot.
Therefore, SAFE mode was selected to get an initial, tuned RP MPS value
set to 256.

> A subsequent hot-add will do nothing in pci_configure_mps(), and
> pcie_bus_configure_settings() looks like it would set the RP and EP
> MPS to the minimum of the RP and EP MPS_Supported.
> 
> Is that what you see?  What would you like to see instead?

No, not quite. After hot-add, we see the EP MPS set to 128 with SAFE
mode even though the RP MPS is 256.

So, the question is if SAFE/PERFORMANCE modes are expected to tune the
EP when it is added? We would have thought that EP's would be tuned
based on the algorithm selected as they're hot-added.

Tyler

> 
> Bjorn