Re: [PATCH net-next v4] cadence: Add LSO support.

[Florian Fainelli]

On 11/08/2016 05:41 AM, Rafal Ozieblo wrote:
> New Cadence GEM hardware support Large Segment Offload (LSO):
> TCP segmentation offload (TSO) as well as UDP fragmentation
> offload (UFO). Support for those features was added to the driver.
> 
> Signed-off-by: Rafal Ozieblo 
> ---

> -#define MACB_MAX_TX_LEN  ((unsigned int)((1 << 
> MACB_TX_FRMLEN_SIZE) - 1))
> -#define GEM_MAX_TX_LEN   ((unsigned int)((1 << 
> GEM_TX_FRMLEN_SIZE) - 1))
> +/* Max length of transmit frame must be a multiple of 8 bytes */
> +#define MACB_TX_LEN_ALIGN8
> +#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_MTU_MIN_SIZE ETH_MIN_MTU
> +#define MACB_NETIF_LSO   (NETIF_F_TSO | NETIF_F_UFO)

Not a huge fan of this definition, since it is always used in conjuction
with netdev_features_t, having it expanded all the time is kind of nicer
for the reader, but this is just personal preference here.

>  
>  #define MACB_WOL_HAS_MAGIC_PACKET(0x1 << 0)
>  #define MACB_WOL_ENABLED (0x1 << 1)
> @@ -1223,7 +1228,8 @@ static void macb_poll_controller(struct net_device *dev)
>  
>  static unsigned int macb_tx_map(struct macb *bp,
>   struct macb_queue *queue,
> - struct sk_buff *skb)
> + struct sk_buff *skb,
> + unsigned int hdrlen)
>  {
>   dma_addr_t mapping;
>   unsigned int len, entry, i, tx_head = queue->tx_head;
> @@ -1231,14 +1237,27 @@ static unsigned int macb_tx_map(struct macb *bp,
>   struct macb_dma_desc *desc;
>   unsigned int offset, size, count = 0;
>   unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
> - unsigned int eof = 1;
> - u32 ctrl;
> + unsigned int eof = 1, mss_mfs = 0;
> + u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
> +
> + /* LSO */
> + if (skb_shinfo(skb)->gso_size != 0) {
> + if (IPPROTO_UDP == (ip_hdr(skb)->protocol))

Most checks are usually done the other way with the left and right
member swapped.

> + /* UDP - UFO */
> + lso_ctrl = MACB_LSO_UFO_ENABLE;
> + else
> + /* TCP - TSO */
> + lso_ctrl = MACB_LSO_TSO_ENABLE;
> + }

>  
>   /* Then, map paged data from fragments */
> @@ -1311,6 +1332,20 @@ static unsigned int macb_tx_map(struct macb *bp,
>   desc = >tx_ring[entry];
>   desc->ctrl = ctrl;
>  
> + if (lso_ctrl) {
> + if (lso_ctrl == MACB_LSO_UFO_ENABLE)
> + /* include header and FCS in value given to h/w */
> + mss_mfs = skb_shinfo(skb)->gso_size +
> + skb_transport_offset(skb) + 4;

ETH_FCS_LEN instead of 4?


> +static netdev_features_t macb_features_check(struct sk_buff *skb,
> +  struct net_device *dev,
> +  netdev_features_t features)
> +{
> + unsigned int nr_frags, f;
> + unsigned int hdrlen;
> +
> + /* Validate LSO compatibility */
> +
> + /* there is only one buffer */
> + if (!skb_is_nonlinear(skb))
> + return features;
> +
> + /* length of header */
> + hdrlen = skb_transport_offset(skb);
> + if (IPPROTO_TCP == (ip_hdr(skb)->protocol))
> + hdrlen += tcp_hdrlen(skb);

Same here, please reverse the left and right members, no need for
parenthesis aground ip_hdr(skb)->protocol.

> +
> + /* For LSO:
> +  * When software supplies two or more payload buffers all payload 
> buffers
> +  * apart from the last must be a multiple of 8 bytes in size.
> +  */
> + if (!IS_ALIGNED(skb_headlen(skb) - hdrlen, MACB_TX_LEN_ALIGN))
> + return features & ~MACB_NETIF_LSO;
> +
> + nr_frags = skb_shinfo(skb)->nr_frags;
> + /* No need to check last fragment */
> + nr_frags--;
> + for (f = 0; f < nr_frags; f++) {
> + const skb_frag_t *frag = _shinfo(skb)->frags[f];
> +
> + if (!IS_ALIGNED(skb_frag_size(frag), MACB_TX_LEN_ALIGN))
> + return features & ~MACB_NETIF_LSO;
> + }
> + return features;
> +}
> +
>  static inline int macb_clear_csum(struct sk_buff *skb)
>  {
>   /* no change for packets without checksum offloading */
> @@ -1374,7 +1456,27 @@ static int macb_start_xmit(struct sk_buff *skb, struct 
> net_device *dev)
>   struct macb *bp = netdev_priv(dev);
>   struct macb_queue *queue = >queues[queue_index];
>   unsigned long flags;
> - unsigned int count, nr_frags, frag_size, f;
> + unsigned int desc_cnt, nr_frags, frag_size, f;
> + unsigned int is_lso = 0, is_udp, hdrlen;
> +
> + is_lso 

[PATCH net-next v4] cadence: Add LSO support.

[Rafal Ozieblo]

New Cadence GEM hardware support Large Segment Offload (LSO):
TCP segmentation offload (TSO) as well as UDP fragmentation
offload (UFO). Support for those features was added to the driver.

Signed-off-by: Rafal Ozieblo 
---
Changed in v2:
macb_lso_check_compatibility() changed to macb_features_check()
(with little modifications) and bind to .ndo_features_check.
(after Eric Dumazet suggestion)
---
Changed in v3:
Respin to net-next.
---
Changed in v4:
(struct iphdr*)skb_network_header(skb) changed to ip_hdr(skb)
---
 drivers/net/ethernet/cadence/macb.c | 142 +---
 drivers/net/ethernet/cadence/macb.h |  14 
 2 files changed, 144 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index e1847ce..2a46aae 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -32,7 +32,9 @@
 #include 
 #include 
 #include 
-
+#include 
+#include 
+#include 
 #include "macb.h"
 
 #define MACB_RX_BUFFER_SIZE128
@@ -60,10 +62,13 @@
| MACB_BIT(TXERR))
 #define MACB_TX_INT_FLAGS  (MACB_TX_ERR_FLAGS | MACB_BIT(TCOMP))
 
-#define MACB_MAX_TX_LEN((unsigned int)((1 << 
MACB_TX_FRMLEN_SIZE) - 1))
-#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1))
+/* 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_MTU_MIN_SIZE   ETH_MIN_MTU
+#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
 
 #define MACB_WOL_HAS_MAGIC_PACKET  (0x1 << 0)
 #define MACB_WOL_ENABLED   (0x1 << 1)
@@ -1223,7 +1228,8 @@ static void macb_poll_controller(struct net_device *dev)
 
 static unsigned int macb_tx_map(struct macb *bp,
struct macb_queue *queue,
-   struct sk_buff *skb)
+   struct sk_buff *skb,
+   unsigned int hdrlen)
 {
dma_addr_t mapping;
unsigned int len, entry, i, tx_head = queue->tx_head;
@@ -1231,14 +1237,27 @@ static unsigned int macb_tx_map(struct macb *bp,
struct macb_dma_desc *desc;
unsigned int offset, size, count = 0;
unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
-   unsigned int eof = 1;
-   u32 ctrl;
+   unsigned int eof = 1, mss_mfs = 0;
+   u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
+
+   /* LSO */
+   if (skb_shinfo(skb)->gso_size != 0) {
+   if (IPPROTO_UDP == (ip_hdr(skb)->protocol))
+   /* UDP - UFO */
+   lso_ctrl = MACB_LSO_UFO_ENABLE;
+   else
+   /* TCP - TSO */
+   lso_ctrl = MACB_LSO_TSO_ENABLE;
+   }
 
/* First, map non-paged data */
len = skb_headlen(skb);
+
+   /* first buffer length */
+   size = hdrlen;
+
offset = 0;
while (len) {
-   size = min(len, bp->max_tx_length);
entry = macb_tx_ring_wrap(bp, tx_head);
tx_skb = >tx_skb[entry];
 
@@ -1258,6 +1277,8 @@ static unsigned int macb_tx_map(struct macb *bp,
offset += size;
count++;
tx_head++;
+
+   size = min(len, bp->max_tx_length);
}
 
/* Then, map paged data from fragments */
@@ -1311,6 +1332,20 @@ static unsigned int macb_tx_map(struct macb *bp,
desc = >tx_ring[entry];
desc->ctrl = ctrl;
 
+   if (lso_ctrl) {
+   if (lso_ctrl == MACB_LSO_UFO_ENABLE)
+   /* include header and FCS in value given to h/w */
+   mss_mfs = skb_shinfo(skb)->gso_size +
+   skb_transport_offset(skb) + 4;
+   else /* TSO */ {
+   mss_mfs = skb_shinfo(skb)->gso_size;
+   /* TCP Sequence Number Source Select
+* can be set only for TSO
+*/
+   seq_ctrl = 0;
+   }
+   }
+
do {
i--;
entry = macb_tx_ring_wrap(bp, i);
@@ -1325,6 +1360,16 @@ static unsigned int macb_tx_map(struct macb *bp,
if (unlikely(entry == (bp->tx_ring_size - 1)))
ctrl |= MACB_BIT(TX_WRAP);
 
+   /* First descriptor is header descriptor */
+   if (i == queue->tx_head) {
+   ctrl |= MACB_BF(TX_LSO, lso_ctrl);
+   ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
+   } else
+   /* Only set MSS/MFS on payload