[PATCH v2 0/2] TSO bug fixes

[PATCH v2 0/2] TSO bug fixes

[Harini Katakam]

An IP errata was recently discovered when testing TSO enabled versions
with perf test tools where a false amba error is reported by the IP.
Some ways to reproduce would be to use iperf or applications with payload
descriptor sizes very close to 16K. Once the error is observed TXERR (or
bit 6 of ISR) will be constantly triggered leading to a series of tx path
error handling and clean up. Workaround the same by limiting this size to
0x3FC0 as recommended by Cadence. There was no performance impact on 1G
system that I tested with.

Note on patch 1: The alignment code may be unused but leaving it there
in case anyone is using UFO.

Added Fixes tag to patch 1.

Harini Katakam (2):
  net: macb: Remove unnecessary alignment check for TSO
  net: macb: Limit maximum GEM TX length in TSO

 drivers/net/ethernet/cadence/macb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] net: macb: Remove unnecessary alignment check for TSO

[Harini Katakam]

The IP TSO implementation does NOT require the length to be a
multiple of 8. That is only a requirement for UFO as per IP
documentation.

Fixes: 1629dd4f763c ("cadence: Add LSO support.")
Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
---
v2:
Added Fixes tag

 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 7a2fe63..2e418b8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1792,7 +1792,7 @@ static netdev_features_t macb_features_check(struct sk_buff *skb,
 	/* Validate LSO compatibility */
 
 	/* there is only one buffer */
-	if (!skb_is_nonlinear(skb))
+	if (!skb_is_nonlinear(skb) || (ip_hdr(skb)->protocol != IPPROTO_UDP))
 		return features;
 
 	/* length of header */
-- 
2.7.4

Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check for TSO

[David Miller]

From: Harini Katakam <harini.katakam@xilinx.com>
Date: Mon,  3 Feb 2020 18:48:01 +0530

> The IP TSO implementation does NOT require the length to be a
> multiple of 8. That is only a requirement for UFO as per IP
> documentation.
> 
> Fixes: 1629dd4f763c ("cadence: Add LSO support.")
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> ---
> v2:
> Added Fixes tag

Several problems with this.

The subject talks about alignemnt check, but you are not changing
the alignment check.  Instead you are modifying the linear buffer
check:

> @@ -1792,7 +1792,7 @@ static netdev_features_t macb_features_check(struct sk_buff *skb,
>  	/* Validate LSO compatibility */
>  
>  	/* there is only one buffer */
> -	if (!skb_is_nonlinear(skb))
> +	if (!skb_is_nonlinear(skb) || (ip_hdr(skb)->protocol != IPPROTO_UDP))
>  		return features;

So either your explanation is wrong or the code change is wrong.

Furthermore, if you add this condition then there is now dead code
below this.  The code that checks for example:

	/* length of header */
	hdrlen = skb_transport_offset(skb);
	if (ip_hdr(skb)->protocol == IPPROTO_TCP)
		hdrlen += tcp_hdrlen(skb);

will never trigger this IPPROTO_TCP condition after your change.

A lot of things about this patch do not add up.

Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check for TSO

[Harini Katakam]

Hi David,

> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Tuesday, February 4, 2020 3:07 PM
> > To: Harini Katakam <harinik@xilinx.com>
> > Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com;
> > kuba@kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Michal Simek <michals@xilinx.com>; harinikatakamlinux@gmail.com
> > Subject: Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check
> > for TSO
> >
> > From: Harini Katakam <harini.katakam@xilinx.com>
> > Date: Mon,  3 Feb 2020 18:48:01 +0530
> >
> > > The IP TSO implementation does NOT require the length to be a multiple
> > > of 8. That is only a requirement for UFO as per IP documentation.
> > >
> > > Fixes: 1629dd4f763c ("cadence: Add LSO support.")
> > > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> > > ---
> > > v2:
> > > Added Fixes tag
> >
> > Several problems with this.
> >
> > The subject talks about alignemnt check, but you are not changing the alignment
> > check.  Instead you are modifying the linear buffer
> > check:

Thanks for the review. Everything below that line becomes unused
when alignment check is removed. More details below.

> >
> > > @@ -1792,7 +1792,7 @@ static netdev_features_t
> > macb_features_check(struct sk_buff *skb,
> > >     /* Validate LSO compatibility */
> > >
> > >     /* there is only one buffer */
> > > -   if (!skb_is_nonlinear(skb))
> > > +   if (!skb_is_nonlinear(skb) || (ip_hdr(skb)->protocol !=
> > > +IPPROTO_UDP))
> > >             return features;
> >
> > So either your explanation is wrong or the code change is wrong.

Alignment check is not required for TSO and is ONLY required for UFO.
So, if NOT(UDP), just return.

macb_features_check()
{
If existing linear check (or) if !UDP
    no need to change features, just return

Alignment check implementation which is only necessary for UDP.
}

> >
> > Furthermore, if you add this condition then there is now dead code below this.
> > The code that checks for example:
> >
> >       /* length of header */
> >       hdrlen = skb_transport_offset(skb);
> >       if (ip_hdr(skb)->protocol == IPPROTO_TCP)
> >               hdrlen += tcp_hdrlen(skb);
> >
> > will never trigger this IPPROTO_TCP condition after your change.

Yes, this is dead code now. I'll remove it.

> >
> > A lot of things about this patch do not add up.

Please let me know if you have any further concerns.

Regards,
Harini

Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check for TSO

[David Miller]

From: Harini Katakam <harinik@xilinx.com>
Date: Tue, 4 Feb 2020 15:52:55 +0530

>> > will never trigger this IPPROTO_TCP condition after your change.
> 
> Yes, this is dead code now. I'll remove it.
> 
>> >
>> > A lot of things about this patch do not add up.
> 
> Please let me know if you have any further concerns.

Looks good with the above correction and a rework of your commit message
to explain things more clearly.

Thank you.

[PATCH v2 2/2] net: macb: Limit maximum GEM TX length in TSO

[Harini Katakam]

GEM_MAX_TX_LEN currently resolves to 0x3FF8 for any IP version supporting
TSO with full 14bits of length field in payload descriptor. But an IP
errata causes false amba_error (bit 6 of ISR) when length in payload
descriptors is specified above 16387. The error occurs because the DMA
falsely concludes that there is not enough space in SRAM for incoming
payload. These errors were observed continuously under stress of large
packets using iperf on a version where SRAM was 16K for each queue. This
errata will be documented shortly and affects all versions since TSO
functionality was added. Hence limit the max length to 0x3FC0 (rounded).

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
---
v2:
Use 0x3FC0 by default

 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 2e418b8..cca321c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -73,7 +73,7 @@ struct sifive_fu540_macb_mgmt {
 /* Max length of transmit frame must be a multiple of 8 bytes */
 #define MACB_TX_LEN_ALIGN	8
 #define MACB_MAX_TX_LEN		((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
-#define GEM_MAX_TX_LEN		((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
+#define GEM_MAX_TX_LEN		(unsigned int)(0x3FC0)
 
 #define GEM_MTU_MIN_SIZE	ETH_MIN_MTU
 #define MACB_NETIF_LSO		NETIF_F_TSO
-- 
2.7.4

Re: [PATCH v2 2/2] net: macb: Limit maximum GEM TX length in TSO

[David Miller]

From: Harini Katakam <harini.katakam@xilinx.com>
Date: Mon,  3 Feb 2020 18:48:02 +0530

> GEM_MAX_TX_LEN currently resolves to 0x3FF8 for any IP version supporting
> TSO with full 14bits of length field in payload descriptor. But an IP
> errata causes false amba_error (bit 6 of ISR) when length in payload
> descriptors is specified above 16387. The error occurs because the DMA
> falsely concludes that there is not enough space in SRAM for incoming
> payload. These errors were observed continuously under stress of large
> packets using iperf on a version where SRAM was 16K for each queue. This
> errata will be documented shortly and affects all versions since TSO
> functionality was added. Hence limit the max length to 0x3FC0 (rounded).
> 
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> ---
> v2:
> Use 0x3FC0 by default

You should add a comment above the definition which explains how this
value was derived.  It looks magic currently.

Re: [PATCH v2 2/2] net: macb: Limit maximum GEM TX length in TSO

[Harini Katakam]

Hi David,

<snip>
> > > v2:
> > > Use 0x3FC0 by default
> >
> > You should add a comment above the definition which explains how this value
> > was derived.  It looks magic currently.

I'll add a comment. The value was specified in the errata document that Cadence
provided to us.

Regards,
Harini