| Message ID | 20210624171418.27194-1-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/2] PCI: Call MPS fixup quirks early | expand |
[+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 >
On Thu, 2021-07-01 at 10:25 -0500, Bjorn Helgaas wrote: [...] > 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? [...] The key thing is that this quirk was working around an issue with legacy interrupts, while the sfc and sfc-falcon drivers have always preferred to use MSIs if available. (But I also don't think many SFC4000-based NICs were sold, and they were EOL'd about 10 years ago.) Ben. >
On Fri, Jul 02, 2021 at 05:39:43PM +0200, Ben Hutchings wrote: > On Thu, 2021-07-01 at 10:25 -0500, Bjorn Helgaas wrote: > [...] > > 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? > [...] > > The key thing is that this quirk was working around an issue with > legacy interrupts, while the sfc and sfc-falcon drivers have always > preferred to use MSIs if available. (But I also don't think many > SFC4000-based NICs were sold, and they were EOL'd about 10 years ago.) Just out of curiosity, do you happen to remember the legacy interrupt connection? MPS has to do with the maximum TLP size, and it's not obvious to me why using INTx vs MSI would matter there. I guessing the scenario is that SFC4000 uses either either INTx or MSI to signal some kind of I/O completion, the ISR puts more I/Os in the queue, the SFC4000 does a DMA read, and chokes on a Completion TLP that's too big? But somehow if it uses MSI, it can handle bigger TLPS? Not a big deal; I think it's obvious that we need Marek's patch to fix the ordering issue. Bjorn
On Fri, 2021-07-02 at 11:24 -0500, Bjorn Helgaas wrote: > On Fri, Jul 02, 2021 at 05:39:43PM +0200, Ben Hutchings wrote: > > On Thu, 2021-07-01 at 10:25 -0500, Bjorn Helgaas wrote: > > [...] > > > 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? > > [...] > > > > The key thing is that this quirk was working around an issue with > > legacy interrupts, while the sfc and sfc-falcon drivers have always > > preferred to use MSIs if available. (But I also don't think many > > SFC4000-based NICs were sold, and they were EOL'd about 10 years ago.) Also, most of the read-only PCIe config registers on the SFC4000 are initialised from flash, and the commit message implies MPSS was changed on later boards. > Just out of curiosity, do you happen to remember the legacy interrupt > connection? MPS has to do with the maximum TLP size, and it's not > obvious to me why using INTx vs MSI would matter there. [...] No I don't. I had completely forgotten about this, so I'm just combining my commit message for the quirk with my general knowledge of that chip. (The bug I actually remember involving legacy interrupts, affecting both SFC4000 and SFC9020, required a horrible workaround in the driver.) Ben.
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
The pci_device_add() function calls header fixups only after pci_configure_device(), which configures MPS. 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") Cc: stable@vger.kernel.org --- drivers/pci/quirks.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)