PCI: Call MPS fixup quirks early

PATCH Applied Marek Behún 4 participants 9 messages Jun 24, 2021 – Jul 26, 2021

Subsystems (2)

Stable branch PCI

lore

Share & tools

lore

Marek Behún

author

2021-06-24 17:14 UTC #0 lore raw

PATCH 1/2 PCI: Call MPS fixup quirks early

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 <[email protected]>

Fixes 27d868b5e6cfa ("PCI: Set MPS to match upstream bridge")

Cc [email protected]

---
 drivers/pci/quirks.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/pci/quirks.c +6 -6

index 22b2bb1109c9..4d9b9d8fbc43 100644
@@ -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

Signature · 1 line

2.31.1

Marek Behún

author

2021-06-24 17:14 UTC #1 lore raw

PATCH 2/2 PCI: Add Max Payload Size quirk for ASMedia ASM1062 SATA controller

The ASMedia ASM1062 SATA controller advertises Max_Payload_Size_Supported of 512, but in fact it cannot handle TLPs with payload size of 512.

We discovered this issue on PCIe controllers capable of MPS = 512 (Aardvark and DesignWare), where the issue presents itself as an External Abort. Bjorn Helgaas says:
Probably ASM1062 reports a Malformed TLP error when it receives a data payload of 512 bytes, and Aardvark, DesignWare, etc convert this to an arm64 External Abort.

Limiting Max Payload Size to 256 bytes solves this problem.

Signed-off-by Marek Behún <[email protected]>

BugLink https://bugzilla.kernel.org/show_bug.cgi?id=212695

Reported-by Rötti <[email protected]>

Cc Pali Rohár <[email protected]>

Cc [email protected]

---
 drivers/pci/quirks.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/quirks.c +1 -0

index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
@@ -3239,6 +3239,7 @@ 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);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ASMEDIA, 0x0612, fixup_mpss_256);
 
 /*
  * Intel 5000 and 5100 Memory controllers have an erratum with read completion

Signature · 1 line

2.31.1

Pali Rohár

Reviewed-by on v1 2/2

2021-07-24 11:14 UTC #2 lore raw

Re: [PATCH 2/2] PCI: Add Max Payload Size quirk for ASMedia ASM1062 SATA controller

Quoted 4 lines · On Thursday 24 June 2021 19:14:18 Marek Behún wrote:

The ASMedia ASM1062 SATA controller advertises Max_Payload_Size_Supported of 512, but in fact it cannot handle TLPs with payload size of 512.

Quoted 8 lines

We discovered this issue on PCIe controllers capable of MPS = 512 (Aardvark and DesignWare), where the issue presents itself as an External Abort. Bjorn Helgaas says:
Probably ASM1062 reports a Malformed TLP error when it receives a data payload of 512 bytes, and Aardvark, DesignWare, etc convert this to an arm64 External Abort.
Limiting Max Payload Size to 256 bytes solves this problem.

Hello Bjorn! Is there anything else needed for merging this patch?

Quoted 4 lines

Signed-off-by Marek Behún <[email protected]>
BugLink https://bugzilla.kernel.org/show_bug.cgi?id=212695
Reported-by Rötti <[email protected]>
Cc Pali Rohár <[email protected]>

Reviewed-by Pali Rohár <[email protected]>

Quoted 20 lines

Cc [email protected]

---
 drivers/pci/quirks.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/quirks.c +1 -0

index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
@@ -3239,6 +3239,7 @@ 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);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ASMEDIA, 0x0612, fixup_mpss_256);
 
 /*
  * Intel 5000 and 5100 Memory controllers have an erratum with read completion

Signature · 1 line

2.31.1

Bjorn Helgaas

Applied

2021-07-26 17:24 UTC #3 lore raw

Re: [PATCH 2/2] PCI: Add Max Payload Size quirk for ASMedia ASM1062 SATA controller

In reply to Marek Behún

Quoted 11 lines · Marek Behún

The ASMedia ASM1062 SATA controller advertises Max_Payload_Size_Supported of 512, but in fact it cannot handle TLPs with payload size of 512.
We discovered this issue on PCIe controllers capable of MPS = 512 (Aardvark and DesignWare), where the issue presents itself as an External Abort. Bjorn Helgaas says:
Probably ASM1062 reports a Malformed TLP error when it receives a data payload of 512 bytes, and Aardvark, DesignWare, etc convert this to an arm64 External Abort.

Quoted 7 lines

Limiting Max Payload Size to 256 bytes solves this problem.
Signed-off-by Marek Behún <[email protected]>
BugLink https://bugzilla.kernel.org/show_bug.cgi?id=212695
Reported-by Rötti <[email protected]>
Cc Pali Rohár <[email protected]>
Cc [email protected]

Applied both to pci/enumeration for v5.15, thanks!

Were you able to confirm that a Malformed TLP error was logged? The lspci in the bugzilla is from a system with no AER support, so no information from that one. I don't know if any of the PCIe controllers you tested support both AER and MPS=512.

Quoted 19 lines

---
 drivers/pci/quirks.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/quirks.c +1 -0

index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
@@ -3239,6 +3239,7 @@ 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);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ASMEDIA, 0x0612, fixup_mpss_256);
 
 /*
  * Intel 5000 and 5100 Memory controllers have an erratum with read completion

Signature · 1 line

2.31.1

Pali Rohár

2021-07-26 21:13 UTC #4 lore raw

Re: [PATCH 2/2] PCI: Add Max Payload Size quirk for ASMedia ASM1062 SATA controller

Quoted 20 lines · On Monday 26 July 2021 12:24:03 Bjorn Helgaas wrote:

Quoted 11 lines · Marek Behún
The ASMedia ASM1062 SATA controller advertises Max_Payload_Size_Supported of 512, but in fact it cannot handle TLPs with payload size of 512.
We discovered this issue on PCIe controllers capable of MPS = 512 (Aardvark and DesignWare), where the issue presents itself as an External Abort. Bjorn Helgaas says:
Probably ASM1062 reports a Malformed TLP error when it receives a data payload of 512 bytes, and Aardvark, DesignWare, etc convert this to an arm64 External Abort.
Quoted 7 lines
Limiting Max Payload Size to 256 bytes solves this problem.
Signed-off-by Marek Behún <[email protected]>
BugLink https://bugzilla.kernel.org/show_bug.cgi?id=212695
Reported-by Rötti <[email protected]>
Cc Pali Rohár <[email protected]>
Cc [email protected]

Quoted 4 lines

Applied both to pci/enumeration for v5.15, thanks!
Were you able to confirm that a Malformed TLP error was logged? The lspci in the bugzilla is from a system with no AER support,

Hello Bjorn! That is because patch for AER support for pci-aardvark.c driver was not reviewed / commented / merged yet:
https://lore.kernel.org/linux-pci/[email protected]/

Anyway, this arm64 external abort is currently sent to arm64 EL3 level (implemented in trusted firmware) and not to kernel. And EL3 hook after receiving this abort resets CPU, so we / kernel do not have opportunity to look what is in AER registers.

So dumping AER registers in this stage from aardvark is quite harder.

Maybe it could be easier with DesignWare PCIe controller.

Quoted 23 lines

so no
information from that one. I don't know if any of the PCIe controllers you tested support both AER and MPS=512.

Quoted 19 lines

---
 drivers/pci/quirks.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/quirks.c +1 -0

index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
@@ -3239,6 +3239,7 @@ 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);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ASMEDIA, 0x0612, fixup_mpss_256);
 
 /*
  * Intel 5000 and 5100 Memory controllers have an erratum with read completion

Signature · 1 line

2.31.1

Bjorn Helgaas

2021-07-01 15:25 UTC #5 lore raw

In reply to Marek Behún

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

Quoted 2 lines · Marek Behún

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)

Quoted 4 lines

So in order to have MPS fixups working, they need to be called early.
Signed-off-by Marek Behún <[email protected]>
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

Quoted 31 lines

Cc [email protected]

---
 drivers/pci/quirks.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/pci/quirks.c +6 -6

index 22b2bb1109c9..4d9b9d8fbc43 100644
@@ -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

Signature · 1 line

2.31.1

Ben Hutchings

2021-07-02 15:55 UTC #6 lore raw

On Thu, 2021-07-01 at 10:25 -0500, Bjorn Helgaas wrote:
[...]

Quoted 8 lines

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.

Quoted 1 line

Signature · 5 lines

Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought. I realized that a large part of my life from then on was going
to be spent in finding mistakes in my own programs.
                                                 - Maurice Wilkes, 1949

Bjorn Helgaas

2021-07-02 16:24 UTC #7 lore raw

Quoted 10 lines · Ben Hutchings

On Thu, 2021-07-01 at 10:25 -0500, Bjorn Helgaas wrote:
[...]
Quoted 8 lines
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?

Quoted 6 lines

[...]
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

Ben Hutchings

2021-07-02 21:54 UTC #8 lore raw

Quoted 11 lines · Bjorn Helgaas

Quoted 10 lines · Ben Hutchings
On Thu, 2021-07-01 at 10:25 -0500, Bjorn Helgaas wrote:
[...]
Quoted 8 lines
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?

Quoted 6 lines

Quoted 6 lines
[...]
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.

Quoted 3 lines

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.

Signature · 5 lines

Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought. I realized that a large part of my life from then on was going
to be spent in finding mistakes in my own programs.
                                                 - Maurice Wilkes, 1949