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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241115190033.7452392e@kernel.org>
Date: Fri, 15 Nov 2024 19:00:33 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, vladimir.oltean@....com, xiaoning.wang@....com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, frank.li@....com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, imx@...ts.linux.dev
Subject: Re: [PATCH v4 net-next 2/5] net: enetc: add Tx checksum offload for
 i.MX95 ENETC

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
-- 
pw-bot: cr