On Mon, 28 Nov 2016 15:03:43 -0800, Jacob Keller wrote: > Often a driver wants to store the flow type and thus it must mask the > extra fields. This is a task that could grow more complex as more flags > are added in the future. Add a helper function that masks the flags for > marking additional fields. > > Modify drivers in drivers/net/ethernet that currently check for FLOW_EXT > and FLOW_MAC_EXT to use the helper. Currently this is only the mellanox > drivers. > > I chose not to modify other drivers as I'm actually unsure whether we > should always mask the flow type even for drivers which don't recognize > the newer flags. On the one hand, today's drivers (generally) > automatically fail when a new flag is used because they won't mask it > and their checks against flow_type will not match. On the other hand, it > means another place that you have to update when you begin implementing > a flag. > > An alternative is to have the driver store a set of flags that it knows > about, and then have ethtool core do the check for us to discard frames. > I haven't implemented this quite yet. > > Signed-off-by: Jacob Keller <[email protected]> > --- > drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 4 ++-- > drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c | 6 +++--- > include/uapi/linux/ethtool.h | 5 +++++ > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > index 487a58f9c192..d8f9839ce2a3 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > @@ -1270,7 +1270,7 @@ static int mlx4_en_validate_flow(struct net_device *dev, > return -EINVAL; > } > > - switch (cmd->fs.flow_type & ~(FLOW_EXT | FLOW_MAC_EXT)) { > + switch (ethtool_get_flow_spec_type(cmd->fs.flow_type)) { > case TCP_V4_FLOW: > case UDP_V4_FLOW: > if (cmd->fs.m_u.tcp_ip4_spec.tos) > @@ -1493,7 +1493,7 @@ static int mlx4_en_ethtool_to_net_trans_rule(struct > net_device *dev, > if (err) > return err; > > - switch (cmd->fs.flow_type & ~(FLOW_EXT | FLOW_MAC_EXT)) { > + switch (ethtool_get_flow_spec_type(cmd->fs.flow_type)) { > case ETHER_FLOW: > spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL); > if (!spec_l2) > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c > index 3691451c728c..066e6c5cf38b 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c > @@ -63,7 +63,7 @@ static struct mlx5e_ethtool_table *get_flow_table(struct > mlx5e_priv *priv, > int table_size; > int prio; > > - switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT)) { > + switch (ethtool_get_flow_spec_type(fs->flow_type)) { > case TCP_V4_FLOW: > case UDP_V4_FLOW: > max_tuples = ETHTOOL_NUM_L3_L4_FTS; > @@ -147,7 +147,7 @@ static int set_flow_attrs(u32 *match_c, u32 *match_v, > outer_headers); > void *outer_headers_v = MLX5_ADDR_OF(fte_match_param, match_v, > outer_headers); > - u32 flow_type = fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT); > + u32 flow_type = ethtool_get_flow_spec_type(fs->flow_type); > struct ethtool_tcpip4_spec *l4_mask; > struct ethtool_tcpip4_spec *l4_val; > struct ethtool_usrip4_spec *l3_mask; > @@ -393,7 +393,7 @@ static int validate_flow(struct mlx5e_priv *priv, > fs->ring_cookie != RX_CLS_FLOW_DISC) > return -EINVAL; > > - switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT)) { > + switch (ethtool_get_flow_spec_type(fs->flow_type)) { > case ETHER_FLOW: > eth_mask = &fs->m_u.ether_spec; > if (!is_zero_ether_addr(eth_mask->h_dest)) > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h > index f0db7788f887..e92ad725c9d0 100644 > --- a/include/uapi/linux/ethtool.h > +++ b/include/uapi/linux/ethtool.h > @@ -1583,6 +1583,11 @@ static inline int ethtool_validate_duplex(__u8 duplex) > #define FLOW_EXT 0x80000000 > #define FLOW_MAC_EXT 0x40000000 > > +static inline __u32 ethtool_get_flow_spec_type(__u32 flow_type) > +{ > + return flow_type & (FLOW_EXT | FLOW_MAC_EXT);
I don't have anything of substance to say but I think you are missing a negation (~) here compared to the code you are replacing ;)
