netdev - Re: [RFCv5 PATCH net-next 02/20] net: introduce operation helpers for netdev features

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 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]
Date:   Thu, 24 Mar 2022 18:09:31 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jian Shen <shenjian15@...wei.com>
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

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.

> 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.