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

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

[Marek Behún]

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(-)

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

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

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

Limiting Max Payload Size to 256 bytes solves this problem.

Signed-off-by: Marek Behún <kabel@kernel.org>
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=212695
Reported-by: Rötti <espressobinboardarmbiantempmailaddress@posteo.de>
Cc: Pali Rohár <pali@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/quirks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -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
-- 
2.31.1

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

[Pali Rohár]

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.
> 
> 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?

> Signed-off-by: Marek Behún <kabel@kernel.org>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=212695
> Reported-by: Rötti <espressobinboardarmbiantempmailaddress@posteo.de>
> Cc: Pali Rohár <pali@kernel.org>

Reviewed-by: Pali Rohár <pali@kernel.org>

> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/quirks.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -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
> -- 
> 2.31.1
> 

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

[Bjorn Helgaas]

On Thu, Jun 24, 2021 at 07:14:18PM +0200, 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.
> 
> 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 <kabel@kernel.org>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=212695
> Reported-by: Rötti <espressobinboardarmbiantempmailaddress@posteo.de>
> Cc: Pali Rohár <pali@kernel.org>
> Cc: stable@vger.kernel.org

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.

> ---
>  drivers/pci/quirks.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -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
> -- 
> 2.31.1
> 

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

[Pali Rohár]

On Monday 26 July 2021 12:24:03 Bjorn Helgaas wrote:
> On Thu, Jun 24, 2021 at 07:14:18PM +0200, 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.
> > 
> > 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 <kabel@kernel.org>
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=212695
> > Reported-by: Rötti <espressobinboardarmbiantempmailaddress@posteo.de>
> > Cc: Pali Rohár <pali@kernel.org>
> > Cc: stable@vger.kernel.org
> 
> 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/20210506153153.30454-43-pali@kernel.org/

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.

> so no
> information from that one.  I don't know if any of the PCIe
> controllers you tested support both AER and MPS=512.
> 
> > ---
> >  drivers/pci/quirks.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 4d9b9d8fbc43..a4ba3e3b3c5e 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -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
> > -- 
> > 2.31.1
> > 

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

[Bjorn Helgaas]

[+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
> 

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

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

-- 
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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bjorn Helgaas]

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

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

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.

-- 
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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]