diff mbox series

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

Message ID 20210624171418.27194-2-kabel@kernel.org
State New
Headers show
Series [1/2] PCI: Call MPS fixup quirks early | expand

Commit Message

Marek Behún June 24, 2021, 5:14 p.m. UTC
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(+)

Comments

Pali Rohár July 24, 2021, 11:14 a.m. UTC | #1
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
>
Bjorn Helgaas July 26, 2021, 5:24 p.m. UTC | #2
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
>
Pali Rohár July 26, 2021, 9:13 p.m. UTC | #3
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
> >
diff mbox series

Patch

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