RE: [PATCH v4 net-next 2/5] net: enetc: add Tx checksum offload for i.MX95 ENETC

[Wei Fang]

> On Fri, 15 Nov 2024 10:47:41 +0800 Wei Fang wrote:
> > +static inline bool enetc_skb_is_ipv6(struct sk_buff *skb) {
> > +	return vlan_get_protocol(skb) == htons(ETH_P_IPV6); }
> > +
> > +static inline bool enetc_skb_is_tcp(struct sk_buff *skb) {
> > +	return skb->csum_offset == offsetof(struct tcphdr, check); }
> 
> Please don't use "inline" for trivial functions, compiler will inline them anyway,
> and it hides unused code. In addition to being pointless.
> 
> >  static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct
> > sk_buff *skb)  {
> >  	bool do_vlan, do_onestep_tstamp = false, do_twostep_tstamp = false;
> > @@ -160,6 +181,27 @@ static int enetc_map_tx_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb)
> >  	dma_addr_t dma;
> >  	u8 flags = 0;
> >
> > +	enetc_clear_tx_bd(&temp_bd);
> > +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +		/* Can not support TSD and checksum offload at the same time */
> > +		if (priv->active_offloads & ENETC_F_TXCSUM &&
> > +		    enetc_tx_csum_offload_check(skb) && !tx_ring->tsd_enable) {
> > +			temp_bd.l3_start = skb_network_offset(skb);
> > +			temp_bd.ipcs = enetc_skb_is_ipv6(skb) ? 0 : 1;
> 
> Linux calculates the IPv4 csum, always, no need.
> 
> > +			temp_bd.l3_hdr_size = skb_network_header_len(skb) / 4;
> > +			temp_bd.l3t = enetc_skb_is_ipv6(skb) ? 1 : 0;
> 
> no need for ternary op, simply :
> 
> 		temp_bd.l3t = enetc_skb_is_ipv6(skb);
> 
> > +			temp_bd.l4t = enetc_skb_is_tcp(skb) ? ENETC_TXBD_L4T_TCP :
> > +							      ENETC_TXBD_L4T_UDP;
> > +			flags |= ENETC_TXBD_FLAGS_CSUM_LSO |
> ENETC_TXBD_FLAGS_L4CS;
> > +		} else {
> > +			if (skb_checksum_help(skb)) {
> > +				dev_err(tx_ring->dev, "skb_checksum_help() error\n");
> > +
> > +				return 0;
> 
> don't print errors on the datapath, it may flood the logs

Thanks for the comments, I will improve this patch.

> --
> pw-bot: cr