[PATCH 0/2] TSO bug fixes

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

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.7.4

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

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
---
 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

[PATCH 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>
---
 drivers/net/ethernet/cadence/macb_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 2e418b8..994fe67 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -74,6 +74,7 @@ struct sifive_fu540_macb_mgmt {
 #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_ERRATA	(unsigned int)(0x3FC0)
 
 #define GEM_MTU_MIN_SIZE	ETH_MIN_MTU
 #define MACB_NETIF_LSO		NETIF_F_TSO
@@ -3640,7 +3641,7 @@ static int macb_init(struct platform_device *pdev)
 
 	/* setup appropriated routines according to adapter type */
 	if (macb_is_gem(bp)) {
-		bp->max_tx_length = GEM_MAX_TX_LEN;
+		bp->max_tx_length = min(GEM_MAX_TX_LEN, GEM_MAX_TX_LEN_ERRATA);
 		bp->macbgem_ops.mog_alloc_rx_buffers = gem_alloc_rx_buffers;
 		bp->macbgem_ops.mog_free_rx_buffers = gem_free_rx_buffers;
 		bp->macbgem_ops.mog_init_rings = gem_init_rings;
-- 
2.7.4

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

[Claudiu.Beznea]

On 31.01.2020 12:27, Harini Katakam wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 2e418b8..994fe67 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -74,6 +74,7 @@ struct sifive_fu540_macb_mgmt {
>  #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_ERRATA  (unsigned int)(0x3FC0)
> 
>  #define GEM_MTU_MIN_SIZE       ETH_MIN_MTU
>  #define MACB_NETIF_LSO         NETIF_F_TSO
> @@ -3640,7 +3641,7 @@ static int macb_init(struct platform_device *pdev)
> 
>         /* setup appropriated routines according to adapter type */
>         if (macb_is_gem(bp)) {
> -               bp->max_tx_length = GEM_MAX_TX_LEN;
> +               bp->max_tx_length = min(GEM_MAX_TX_LEN, GEM_MAX_TX_LEN_ERRATA);

Isn't this always resolved to GEM_MAX_TX_LEN_ERRATA?

>                 bp->macbgem_ops.mog_alloc_rx_buffers = gem_alloc_rx_buffers;
>                 bp->macbgem_ops.mog_free_rx_buffers = gem_free_rx_buffers;
>                 bp->macbgem_ops.mog_init_rings = gem_init_rings;
> --
> 2.7.4
> 
> 

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

[Harini Katakam]

Hi Claudiu,

On Fri, Jan 31, 2020 at 8:27 PM <Claudiu.Beznea@microchip.com> wrote:
>
>
>
> On 31.01.2020 12:27, Harini Katakam wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > 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>
> > ---
<snip>
> > -               bp->max_tx_length = GEM_MAX_TX_LEN;
> > +               bp->max_tx_length = min(GEM_MAX_TX_LEN, GEM_MAX_TX_LEN_ERRATA);
>
> Isn't this always resolved to GEM_MAX_TX_LEN_ERRATA?

Sorry, yes it does. I accidentally concluded that this was
version specific. Will just default to 0x3fc0 in v2.
Thanks for the review.

Regards,
Harini

Re: [PATCH 0/2] TSO bug fixes

[Jakub Kicinski]

On Fri, 31 Jan 2020 15:57:33 +0530, Harini Katakam wrote:
> orkaround 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.

Hi Harini! Please provide Fixes tags when you post v2, thanks!