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] [day] [month] [year] [list]

Message-ID:
 <PAXPR04MB851082CFB4011ABB615AB20A88272@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Mon, 18 Nov 2024 01:23:17 +0000
From: Wei Fang <wei.fang@....com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Claudiu Manoil <claudiu.manoil@....com>, Vladimir Oltean
	<vladimir.oltean@....com>, Clark Wang <xiaoning.wang@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>, Frank Li <frank.li@....com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <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

Thanks for the comments, I will improve this patch.

> --
> pw-bot: cr