[PATCH RFC net-next 1/7] netdev_features: remove unused __UNUSED_NETIF_F_1

Alexander Lobakin posted 7 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH RFC net-next 1/7] netdev_features: remove unused __UNUSED_NETIF_F_1
Posted by Alexander Lobakin 1 year, 10 months ago
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. */
-- 
2.44.0
Re: [PATCH RFC net-next 1/7] netdev_features: remove unused __UNUSED_NETIF_F_1
Posted by Andrew Lunn 1 year, 10 months ago
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
Re: [PATCH RFC net-next 1/7] netdev_features: remove unused __UNUSED_NETIF_F_1
Posted by Alexander Lobakin 1 year, 10 months ago
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
Re: [PATCH RFC net-next 1/7] netdev_features: remove unused __UNUSED_NETIF_F_1
Posted by Andrew Lunn 1 year, 10 months ago
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