Re: [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[Davide Caratti <dcaratti@redhat.com>]

hello Tariq,
On Sun, 2017-06-18 at 14:10 +0300, Tariq Toukan wrote:
> > @@ -624,12 +632,13 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
> >                hdr += sizeof(struct vlan_hdr);
> >        }
> >    
> > -     if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
> > -             get_fixed_ipv4_csum(hw_checksum, skb, hdr);
> > +     if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4)) &&
> > +         (unlikely(get_fixed_ipv4_csum(hw_checksum, skb, hdr))))
> 
> No! The lazy evaluation trick is wrong here.
> This way you'll end up going almost always to the else (ipv6) for the 
> wrong reason.

you are right! thanks for spotting this.

> > +             return -1;
> >    #if IS_ENABLED(CONFIG_IPV6)
> > -     else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
> > -             if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
> > -                     return -1;
> > +     else if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6)) &&
> > +              (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr))))
> > +             return -1;
> 
> Let's not change this, might cause future bugs, similarly to the one above.
> >    #endif
> >        return 0;
> >    }

maybe we can avoid adding braces, remove that 'else' keyword and the nested 'if',
thus saving one line, given that check_csum() returns the same set of values as
get_fixed_ipv{4,6}_checksum(), with the same meaning (-1 => go with CHECKSUM_NONE,
0 => go with CHECKSUM_COMPLETE).

---- >8 ----
@@ -625,11 +633,10 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
 	}
 
 	if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
-		get_fixed_ipv4_csum(hw_checksum, skb, hdr);
+		return get_fixed_ipv4_csum(hw_checksum, skb, hdr);
 #if IS_ENABLED(CONFIG_IPV6)
-	else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
-		if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
-			return -1;
+	if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
+		return get_fixed_ipv6_csum(hw_checksum, skb, hdr);
 #endif
 	return 0;
 }
---- 8< ----

I will test and repost a v2 with this modification, unless you have any
objections. Thank you in advance!
regards