Re: [PATCH 01/20] net-core: extending (hw_/wanted_/vlan_)features fields to a bitmap.

["Michał Mirosław" <mirqus@gmail.com>]

2011/4/6 Mahesh Bandewar <maheshb@google.com>:
> Converting current use of (hw_/wanted_/vlan_)features to
> legacy_(hw_/wanted_/vlan_)features to differntiate from the proposed usage.
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
>  include/linux/netdevice.h |  110 +++++++++++++++++++++++++++++++-------------
>  net/core/dev.c            |   51 +++++++++++----------
>  net/core/ethtool.c        |   97 ++++++++++++++++++++-------------------
>  net/core/net-sysfs.c      |    4 +-
>  net/core/sock.c           |    2 +-
>  5 files changed, 155 insertions(+), 109 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 09d2624..637bf2a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -980,6 +980,42 @@ struct net_device_ops {
>                                                    u32 features);
>  };
>
> +enum netdev_features {
> +       NETIF_F_SG_BIT,                 /* Scatter/gather IO. */
> +       NETIF_F_IP_CSUM_BIT,            /* Can checksum TCP/UDP over IPv4. */
> +       NETIF_F_NO_CSUM_BIT,            /* Does not require checksum. F.e. loopack. */
> +       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_FRAGLIST_BIT,           /* Scatter/gather IO. */
> +       NETIF_F_HW_VLAN_TX_BIT,         /* Transmit VLAN hw acceleration */
> +       NETIF_F_HW_VLAN_RX_BIT,         /* Receive VLAN hw acceleration */
> +       NETIF_F_HW_VLAN_FILTER_BIT,     /* Receive filtering on VLAN */
> +       NETIF_F_VLAN_CHALLENGED_BIT,    /* Device cannot handle VLAN packets */
> +       NETIF_F_GSO_BIT,                /* Enable software GSO. */
> +       NETIF_F_LLTX_BIT,               /* LockLess TX - deprecated. Please */
> +                                       /* do not use LLTX in new drivers */
> +       NETIF_F_NETNS_LOCAL_BIT,        /* Does not change network namespaces */
> +       NETIF_F_GRO_BIT,                /* Generic receive offload */
> +       NETIF_F_LRO_BIT,                /* large receive offload */
> +       /* the GSO_MASK reserves bits 16 through 23 */
> +       RESERVED1_BIT,
> +       RESERVED2_BIT,
> +       RESERVED3_BIT,
> +       RESERVED4_BIT,
> +       RESERVED5_BIT,
> +       RESERVED6_BIT,
> +       RESERVED7_BIT,
> +       RESERVED8_BIT,
> +       NETIF_F_FCOE_CRC_BIT,           /* FCoE CRC32 */
> +       NETIF_F_SCTP_CSUM_BIT,          /* SCTP checksum offload */
> +       NETIF_F_FCOE_MTU_BIT,           /* Supports max FCoE MTU, 2158 bytes*/
> +       NETIF_F_NTUPLE_BIT,             /* N-tuple filters supported */
> +       NETIF_F_RXHASH_BIT,             /* Receive hashing offload */
> +       NETIF_F_RXCSUM_BIT,             /* Receive checksumming offload */
> +       NETIF_F_NOCACHE_COPY_BIT,       /* Use no-cache copyfromuser */
> +};
> +

This should be a separate cleanup patch. And after that, for the
conversion you would add as a last entry:
NETIF_F_NUM_BITS and use it later (see below).

>  /*
>  *     The DEVICE structure.
>  *     Actually, this whole structure is a big mistake.  It mixes I/O
> @@ -1029,44 +1065,51 @@ struct net_device {
>        struct list_head        napi_list;
>        struct list_head        unreg_list;
>
> +#define DEV_FEATURE_WORDS      2
> +#define        DEV_FEATURE_BITS        (DEV_FEATURE_WORDS*sizeof(long)*BITS_PER_BYTE)
> +#define LEGACY_FEATURE_WORD    0
> +

#define DEV_FEATURE_WORDS BITS_TO_LONGS(NETIF_F_NUM_BITS)
#define DEV_FEATURE_BITS (DEV_FEATURE_WORDS*BITS_PER_LONG)

Though using bitmaps will make a mess for 32 versus 64 bit archs. It
would be better to stick to u32 as the base type instead of long.

[...]
> @@ -2376,13 +2419,13 @@ static inline void netif_tx_unlock_bh(struct net_device *dev)
>  }
>
>  #define HARD_TX_LOCK(dev, txq, cpu) {                  \
> -       if ((dev->features & NETIF_F_LLTX) == 0) {      \
> +       if ((dev->legacy_features & NETIF_F_LLTX) == 0) {       \
[...]

For those type of conversion there is really no point in using the
macro. Changing it to
dev->features[0] instead of dev->legacy_features needs the same effort
but avoids the
cleanup later. Flags in other feature words could have names line
NETIF_F2_xxx so that
it would be clear in which word they belong.

Best Regards,
Michał Mirosław