netdev - Re: [PATCH RFC net-next 1/7] netdev_features: remove unused __UNUSED_NETIF_F

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: <10a14ce9-1cbb-4906-8363-99c8fc3c7fb6@lunn.ch>
Date: Fri, 5 Apr 2024 17:41:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	David Ahern <dsahern@...nel.org>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 1/7] netdev_features: remove unused
 __UNUSED_NETIF_F_1

On Fri, Apr 05, 2024 at 05:15:58PM +0200, Alexander Lobakin wrote:
> From: Andrew Lunn <andrew@...n.ch>
> Date: Fri, 5 Apr 2024 16:12:50 +0200
> 
> > On Fri, Apr 05, 2024 at 03:37:25PM +0200, Alexander Lobakin wrote:
> >> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27
> >> ("net: remove NETIF_F_NO_CSUM feature bit") and became
> >> __UNUSED_NETIF_F_1. It's not used anywhere in the code.
> >> Remove this bit waste.
> >>
> >> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> >> ---
> >>  include/linux/netdev_features.h | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> >> index 7c2d77d75a88..44c428d62db4 100644
> >> --- a/include/linux/netdev_features.h
> >> +++ b/include/linux/netdev_features.h
> >> @@ -14,7 +14,6 @@ typedef u64 netdev_features_t;
> >>  enum {
> >>  	NETIF_F_SG_BIT,			/* Scatter/gather IO. */
> >>  	NETIF_F_IP_CSUM_BIT,		/* Can checksum TCP/UDP over IPv4. */
> >> -	__UNUSED_NETIF_F_1,
> >>  	NETIF_F_HW_CSUM_BIT,		/* Can checksum all the packets. */
> >>  	NETIF_F_IPV6_CSUM_BIT,		/* Can checksum TCP/UDP over IPV6 */
> >>  	NETIF_F_HIGHDMA_BIT,		/* Can DMA to high memory. */
> > 
> > Are you sure this enum is not ABI?
> 
> Why should this be ABI? It's not a part of UAPI and Ethtool receives
> these bits together with string names.

As a reviewer, i think about ABI. When looking at a change like this,
it is the first thing i think of. Our code is not always clean, there
could well be things outside of include/uapi which influence the
ABI. For some reason ("net: remove NETIF_F_NO_CSUM feature bit")
renamed rather than removed it? Why?

I assume you have looked into the code in this respect, you have
tested both and ioctl and netlink code, and concluded it does not
cause an ABI change. It could of been removed in 3.2-rc2. But i don't
see anything in the cover letter or commit message which indicates you
have done that. That is partially what the cover letter and the commit
message is about. Explaining things you have done, but cannot be seen
in the code change.

	Andrew