| Message ID | 20240405133731.1010128-2-aleksander.lobakin@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdev_features: start cleaning netdev_features_t up | expand |
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@intel.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? It would be good to add an explanation why it is not ABI to the cover letter. Andrew
From: Andrew Lunn <andrew@lunn.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@intel.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. > > It would be good to add an explanation why it is not ABI to the cover > letter. > > Andrew Thanks, Olek
On Fri, Apr 05, 2024 at 05:15:58PM +0200, Alexander Lobakin wrote: > From: Andrew Lunn <andrew@lunn.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@intel.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
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. */
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@intel.com> --- include/linux/netdev_features.h | 1 - 1 file changed, 1 deletion(-)