Date:   Sat, 16 Apr 2022 11:33:58 +0800
From:   "shenjian (K)" <shenjian15@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <andrew@...n.ch>, <ecree.xilinx@...il.com>,
        <hkallweit1@...il.com>, <alexandr.lobakin@...el.com>,
        <saeed@...nel.org>, <leon@...nel.org>, <netdev@...r.kernel.org>,
        <linuxarm@...neuler.org>, <lipeng321@...wei.com>
Subject: Re: [RFCv5 PATCH net-next 02/20] net: introduce operation helpers for
 netdev features



在 2022/3/25 9:09, Jakub Kicinski 写道:
> On Thu, 24 Mar 2022 23:49:14 +0800 Jian Shen wrote:
>> Introduce a set of bitmap operation helpers for netdev features,
>> then we can use them to replace the logical operation with them.
>> As the nic driversare not supposed to modify netdev_features
>> directly, it also introduces wrappers helpers to this.
>>
>> The implementation of these helpers are based on the old prototype
>> of netdev_features_t is still u64. I will rewrite them on the last
>> patch, when the prototype changes.
>>
>> Signed-off-by: Jian Shen <shenjian15@...wei.com>
>> ---
>>   include/linux/netdevice.h | 597 ++++++++++++++++++++++++++++++++++++++
> Please move these helpers to a new header file which won't be included
> by netdevice.h and include it at users appropriately.
I introduced a new header file "netdev_features_helper",  and moved 
thses helpers
to it.  Some helpers need to include struct  net_device which defined in 
netdevice.h,
but there are also some inline functions in netdevice.h need to use 
these netdev_features
helpers. It's conflicted.

So far I thought 3 ways to solved it, but all of them are not satisfactory.
1) Split netdevice.h, move the definition of struct net_device and its 
relative definitions to
a new header file A( haven't got a reasonable name).  Both the 
netdev_features_helper.h
and the netdevice include A.

2) Split netdevice.h, move the inline functions to a new header file B. 
The netdev_features_helper.h
inlucde netdevice.h， and B include netdev_features_helper.h and 
netdevice.h. All the source files
which using these ininline functions should include B.

3) Split netdevice.h, move the inline functions to to 
netdev_featurer_helper.h. The netdev_features_helper.h
inlucde netdevice.h, All the source files which using these ininline 
functions should include netde_features_helper.h.

I'd like to get more advice to this.

Thanks!
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 7307b9553bcf..0af4b26896d6 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2295,6 +2295,603 @@ struct net_device {
>>   };
>>   #define to_net_dev(d) container_of(d, struct net_device, dev)
>>   
>> +static inline void netdev_features_zero(netdev_features_t *dst)
>> +{
>> +	*dst = 0;
>> +}
>> +
>> +static inline void netdev_features_fill(netdev_features_t *dst)
>> +{
>> +	*dst = ~0ULL;
>> +}
>> +
>> +static inline bool netdev_features_empty(const netdev_features_t src)
> Don't pass by value something larger than 64 bit.
>
>> +{
>> +	return src == 0;
>> +}
>> +
>> +/* helpers for netdev features '==' operation */
>> +static inline bool netdev_features_equal(const netdev_features_t src1,
>> +					 const netdev_features_t src2)
>> +{
>> +	return src1 == src2;
>> +}
>> +/* helpers for netdev features '&=' operation */
>> +static inline void
>> +netdev_features_direct_and(netdev_features_t *dst,
>> +			   const netdev_features_t features)
>> +{
>> +	*dst = netdev_features_and(*dst, features);
>> +}
>> +
>> +static inline void
>> +netdev_active_features_direct_and(struct net_device *ndev,
> s/direct_and/mask/ ?
>
>> +				  const netdev_features_t features)
>> +{
>> +	ndev->active_features = netdev_active_features_and(ndev, features);
>> +}
>> +
>> +/* helpers for netdev features '|' operation */
>> +static inline netdev_features_t
>> +netdev_features_or(const netdev_features_t a, const netdev_features_t b)
>> +{
>> +	return a | b;
>> +}
>> +/* helpers for netdev features '|=' operation */
>> +static inline void
>> +netdev_features_direct_or(netdev_features_t *dst,
> s/direct_or/set/ ?
>
>> +			  const netdev_features_t features)
>> +{
>> +	*dst = netdev_features_or(*dst, features);
>> +}
>> +/* helpers for netdev features '^' operation */
>> +static inline netdev_features_t
>> +netdev_features_xor(const netdev_features_t a, const netdev_features_t b)
>> +{
>> +	return a ^ b;
>> +}
>> +/* helpers for netdev features '^=' operation */
>> +static inline void
>> +netdev_active_features_direct_xor(struct net_device *ndev,
> s/direct_xor/toggle/ ?
>
>> +/* helpers for netdev features '& ~' operation */
>> +static inline netdev_features_t
>> +netdev_features_andnot(const netdev_features_t a, const netdev_features_t b)
>> +{
>> +	return a & ~b;
>> +}
>> +static inline void
>> +netdev_features_direct_andnot(netdev_features_t *dst,
> s/andnot/clear/ ?
>
>> +			     const netdev_features_t features)
>> +{
>> +	*dst = netdev_features_andnot(*dst, features);
>> +}
>> +/* helpers for netdev features 'set bit' operation */
>> +static inline void netdev_features_set_bit(int nr, netdev_features_t *src)
> s/features_set_bit/feature_add/ ?
>
>> +{
>> +	*src |= __NETIF_F_BIT(nr);
>> +}
>> +/* helpers for netdev features 'set bit array' operation */
>> +static inline void netdev_features_set_array(const int *array, int array_size,
>> +					     netdev_features_t *dst)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < array_size; i++)
>> +		netdev_features_set_bit(array[i], dst);
>> +}
>> +
>> +#define netdev_active_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->active_features)
>> +
>> +#define netdev_hw_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->hw_features)
>> +
>> +#define netdev_wanted_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->wanted_features)
>> +
>> +#define netdev_vlan_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->vlan_features)
>> +
>> +#define netdev_hw_enc_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->hw_enc_features)
>> +
>> +#define netdev_mpls_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->mpls_features)
>> +
>> +#define netdev_gso_partial_features_set_array(ndev, array, array_size) \
>> +		netdev_features_set_array(array, array_size, &ndev->gso_partial_features)
>> +
>> +/* helpers for netdev features 'clear bit' operation */
>> +static inline void netdev_features_clear_bit(int nr, netdev_features_t *src)
> All the mentions of '_bit' are unnecessary IMHO.
> .
>

Powered by blists - more mailing lists