Re: [net 1/2] net/mlx5e: When RXFCS is set, add FCS data into checksum calculation

[Eric Dumazet <eric.dumazet@gmail.com>]

On 05/24/2018 02:53 PM, Saeed Mahameed wrote:
> From: Eran Ben Elisha <eranbe@mellanox.com>
> 
> When RXFCS feature is enabled, the HW do not strip the FCS data,
> however it is not present in the checksum calculated by the HW.
> 
> Fix that by manually calculating the FCS checksum and adding it to the SKB
> checksum field.
> 
> Add helper function to find the FCS data for all SKB forms (linear,
> one fragment or more).
> 
> Fixes: 102722fc6832 ("net/mlx5e: Add support for RXFCS feature flag")
> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 176645762e49..1ff0b0e93804 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -615,6 +615,45 @@ static inline bool is_last_ethertype_ip(struct sk_buff *skb, int *network_depth)
>  	return (ethertype == htons(ETH_P_IP) || ethertype == htons(ETH_P_IPV6));
>  }
>  
> +static __be32 mlx5e_get_fcs(struct sk_buff *skb)
> +{
> +	int last_frag_sz, bytes_in_prev, nr_frags;
> +	u8 *fcs_p1, *fcs_p2;
> +	skb_frag_t *last_frag;
> +	__be32 fcs_bytes;
> +
> +	if (!skb_is_nonlinear(skb))
> +		return *(__be32 *)(skb->data + skb->len - ETH_FCS_LEN);
> +
> +	nr_frags = skb_shinfo(skb)->nr_frags;
> +	last_frag = &skb_shinfo(skb)->frags[nr_frags - 1];
> +	last_frag_sz = skb_frag_size(last_frag);
> +
> +	/* If all FCS data is in last frag */
> +	if (last_frag_sz >= ETH_FCS_LEN)
> +		return *(__be32 *)(skb_frag_address(last_frag) +
> +				   last_frag_sz - ETH_FCS_LEN);
> +
> +	fcs_p2 = (u8 *)skb_frag_address(last_frag);
> +	bytes_in_prev = ETH_FCS_LEN - last_frag_sz;
> +
> +	/* Find where the other part of the FCS is - Linear or another frag */
> +	if (nr_frags == 1) {
> +		fcs_p1 = skb_tail_pointer(skb);
> +	} else {
> +		skb_frag_t *prev_frag = &skb_shinfo(skb)->frags[nr_frags - 2];
> +
> +		fcs_p1 = skb_frag_address(prev_frag) +
> +			    skb_frag_size(prev_frag);
> +	}
> +	fcs_p1 -= bytes_in_prev;
> +
> +	memcpy(&fcs_bytes, fcs_p1, bytes_in_prev);
> +	memcpy(((u8 *)&fcs_bytes) + bytes_in_prev, fcs_p2, last_frag_sz);
> +
> +	return fcs_bytes;
> +}
>

Oh well, this is so ugly, why isn't skb_header_pointer() used ?

Untested patch :

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 94224c22ecc310a87b6715051e335446f29bec03..11129e3a50d6f3b9a49861a99023541720bbcbe4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -713,43 +713,12 @@ static inline void mlx5e_enable_ecn(struct mlx5e_rq *rq, struct sk_buff *skb)
        rq->stats->ecn_mark += !!rc;
 }
 
-static __be32 mlx5e_get_fcs(struct sk_buff *skb)
+static __be32 mlx5e_get_fcs(const struct sk_buff *skb)
 {
-       int last_frag_sz, bytes_in_prev, nr_frags;
-       u8 *fcs_p1, *fcs_p2;
-       skb_frag_t *last_frag;
        __be32 fcs_bytes;
 
-       if (!skb_is_nonlinear(skb))
-               return *(__be32 *)(skb->data + skb->len - ETH_FCS_LEN);
-
-       nr_frags = skb_shinfo(skb)->nr_frags;
-       last_frag = &skb_shinfo(skb)->frags[nr_frags - 1];
-       last_frag_sz = skb_frag_size(last_frag);
-
-       /* If all FCS data is in last frag */
-       if (last_frag_sz >= ETH_FCS_LEN)
-               return *(__be32 *)(skb_frag_address(last_frag) +
-                                  last_frag_sz - ETH_FCS_LEN);
-
-       fcs_p2 = (u8 *)skb_frag_address(last_frag);
-       bytes_in_prev = ETH_FCS_LEN - last_frag_sz;
-
-       /* Find where the other part of the FCS is - Linear or another frag */
-       if (nr_frags == 1) {
-               fcs_p1 = skb_tail_pointer(skb);
-       } else {
-               skb_frag_t *prev_frag = &skb_shinfo(skb)->frags[nr_frags - 2];
-
-               fcs_p1 = skb_frag_address(prev_frag) +
-                           skb_frag_size(prev_frag);
-       }
-       fcs_p1 -= bytes_in_prev;
-
-       memcpy(&fcs_bytes, fcs_p1, bytes_in_prev);
-       memcpy(((u8 *)&fcs_bytes) + bytes_in_prev, fcs_p2, last_frag_sz);
-
-       return fcs_bytes;
+       return *(__be32 *)skb_header_pointer(skb, skb->len - ETH_FCS_LEN,
+                                            ETH_FCS_LEN, &fcs_bytes);
 }