Re: [PATCH 1/2] PCI: Call MPS fixup quirks early

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Edward, Martin (SFC maintainers), Ben, Keith (just FYI)]

On Thu, Jun 24, 2021 at 07:14:17PM +0200, Marek Behún wrote:
> The pci_device_add() function calls header fixups only after
> pci_configure_device(), which configures MPS.

This makes good sense; the call graph looks like:

  pci_device_add
    pci_configure_device
      pci_configure_mps
        pcie_get_mps(dev)
        pcie_get_mps(bridge)
 +      pcie_set_mps(dev)             # added by 27d868b5e6cfa
    pci_fixup_device(pci_fixup_header)

> So in order to have MPS fixups working, they need to be called early.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: 27d868b5e6cfa ("PCI: Set MPS to match upstream bridge")

Before 27d868b5e6cfa, pci_configure_device() really didn't *do*
anything [1].  It read the MPS settings from the device and upstream
bridge and possibly printed a warning, but didn't change anything.

After 27d868b5e6cfa, pci_configure_device() did actually call
pcie_set_mps(), which updates the Device Control register (possibly
restricted by dev->pcie_mpss, which is set by this quirk).

The fixup_mpss_256() quirk was added in 2011 by a94d072b2023 ("PCI:
Add quirk for known incorrect MPSS").  Interesting that 27d868b5e6cfa
was merged in 2015 but apparently nobody noticed until now.  I guess
those Solarflare devices aren't widely used?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/probe.c?id=27d868b5e6cfa^#n1278

> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/quirks.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 22b2bb1109c9..4d9b9d8fbc43 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3233,12 +3233,12 @@ static void fixup_mpss_256(struct pci_dev *dev)
>  {
>  	dev->pcie_mpss = 1; /* 256 bytes */
>  }
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SOLARFLARE,
> -			 PCI_DEVICE_ID_SOLARFLARE_SFC4000A_0, fixup_mpss_256);
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SOLARFLARE,
> -			 PCI_DEVICE_ID_SOLARFLARE_SFC4000A_1, fixup_mpss_256);
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SOLARFLARE,
> -			 PCI_DEVICE_ID_SOLARFLARE_SFC4000B, fixup_mpss_256);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SOLARFLARE,
> +			PCI_DEVICE_ID_SOLARFLARE_SFC4000A_0, fixup_mpss_256);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SOLARFLARE,
> +			PCI_DEVICE_ID_SOLARFLARE_SFC4000A_1, fixup_mpss_256);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SOLARFLARE,
> +			PCI_DEVICE_ID_SOLARFLARE_SFC4000B, fixup_mpss_256);
>  
>  /*
>   * Intel 5000 and 5100 Memory controllers have an erratum with read completion
> -- 
> 2.31.1
>