Re: [Intel-wired-lan] [PATCH iwl-next v2 1/4] ice: pass the return value of skb_checksum_help()

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

Simon says:
There is also an AI-generated review of this patch available on sashiko.dev
However I believe that can be treated in the context of possible follow-up.

---
ice: pass the return value of skb_checksum_help()

This patch propagates the return value of skb_checksum_help() back to
the caller and consolidates the software checksum fallback path under a
single checksum_sw_fb label in ice_tx_csum().

> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 4ca1a0602307..c04c5856dad6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -1654,7 +1654,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  			ret = ipv6_skip_exthdr(skb, exthdr - skb->data,
>  					       &l4_proto, &frag_off);
>  			if (ret < 0)
> -				return -1;
> +				goto checksum_sw_fb;
>  		}
>  
>  		/* define outer transport */
> @@ -1673,11 +1673,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  			l4.hdr = skb_inner_network_header(skb);
>  			break;
>  		default:
> -			if (first->tx_flags & ICE_TX_FLAGS_TSO)
> -				return -1;
> -
> -			skb_checksum_help(skb);
> -			return 0;
> +			goto checksum_sw_fb;
>  		}
>  
>  		/* compute outer L3 header size */

[ ... ]

> @@ -1736,7 +1732,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  			ipv6_skip_exthdr(skb, exthdr - skb->data, &l4_proto,
>  					 &frag_off);
>  	} else {
> -		return -1;
> +		goto checksum_sw_fb;
>  	}

[Low]

In the encapsulated branch, when the inner IP version is neither 4 nor 6
the new behaviour is to jump to checksum_sw_fb instead of unconditionally
returning -1. Before this patch the non-TSO path also bailed via
return 0 after skb_checksum_help(), and the caller would not emit a
context descriptor for that packet. After this patch the encapsulation
block has already populated off->cd_tunnel_params and set
off->cd_qw1 |= ICE_TX_DESC_DTYPE_CTX:

	/* record tunnel offload values */
	off->cd_tunnel_params |= tunnel;

	off->cd_qw1 |= (u64)ICE_TX_DESC_DTYPE_CTX;

If skb_checksum_help() then returns 0, ice_tx_csum() returns 0 and
ice_xmit_frame_ring() will still build a Tx context descriptor with the
tunnel parameters even though no L3/L4 offload bits ever get programmed
into the data descriptor.

Is that descriptor pair intentional for these malformed encapsulated
packets, or should the encapsulated default case clear the tunnel state
(or just return -1) before falling through to the software fallback?

> @@ -1789,15 +1785,17 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  		break;
>  
>  	default:
> -		if (first->tx_flags & ICE_TX_FLAGS_TSO)
> -			return -1;
> -		skb_checksum_help(skb);
> -		return 0;
> +		goto checksum_sw_fb;
>  	}
>  
>  	off->td_cmd |= cmd;
>  	off->td_offset |= offset;
>  	return 1;
> +
> +checksum_sw_fb:
> +	if (first->tx_flags & ICE_TX_FLAGS_TSO)
> +		return -1;
> +	return skb_checksum_help(skb);
>  }