diff mbox series

[mlx5-next,07/10] net/mlx5: E-Switch, Change vhca id valid bool field to bit flag

Message ID 20181210030442.7543-8-saeedm@mellanox.com (mailing list archive)
State Superseded
Headers show
Series mlx5 core updates and cleanups | expand

Commit Message

Saeed Mahameed Dec. 10, 2018, 3:04 a.m. UTC 
From: Eli Britstein <elibr@mellanox.com>

Change the driver flow destination struct to use bit flags with the vhca
id valid being the 1st one. Done to avoid bool fields in structs, as
warned by static checkers, with no functionality change.

Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/eswitch_offloads.c    | 8 +++++---
 drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c          | 3 ++-
 include/linux/mlx5/fs.h                                   | 6 +++++-
 3 files changed, 12 insertions(+), 5 deletions(-)

Comments

Jason Gunthorpe Dec. 10, 2018, 4:12 p.m. UTC | #1 
On Sun, Dec 09, 2018 at 07:04:39PM -0800, Saeed Mahameed wrote:
> From: Eli Britstein <elibr@mellanox.com>
> 
> Change the driver flow destination struct to use bit flags with the vhca
> id valid being the 1st one. Done to avoid bool fields in structs, as
> warned by static checkers, with no functionality change.

This is a thing now? I thought the warning was not to use bool with
bitfields for some reason?

Jason
Saeed Mahameed Dec. 10, 2018, 7:12 p.m. UTC | #2 
On Mon, Dec 10, 2018 at 8:12 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Sun, Dec 09, 2018 at 07:04:39PM -0800, Saeed Mahameed wrote:
> > From: Eli Britstein <elibr@mellanox.com>
> >
> > Change the driver flow destination struct to use bit flags with the vhca
> > id valid being the 1st one. Done to avoid bool fields in structs, as
> > warned by static checkers, with no functionality change.
>
> This is a thing now? I thought the warning was not to use bool with
> bitfields for some reason?
>

Yea, the commit message is kinda silly,
But the patch makes sense since having a flag can be more extendable
in the future, e.g in the next patch:
MLX5_FLOW_DEST_VPORT_REFORMAT_ID

We can fix the commit message.

> Jason
Or Gerlitz Dec. 10, 2018, 7:51 p.m. UTC | #3 
On Mon, Dec 10, 2018 at 6:31 PM Jason Gunthorpe <jgg@mellanox.com> wrote:
> On Sun, Dec 09, 2018 at 07:04:39PM -0800, Saeed Mahameed wrote:
> > From: Eli Britstein <elibr@mellanox.com>
> >
> > Change the driver flow destination struct to use bit flags with the vhca
> > id valid being the 1st one. Done to avoid bool fields in structs, as
> > warned by static checkers, with no functionality change.
>
> This is a thing now? I thought the warning was not to use bool with
> bitfields for some reason?

we have downstream, patch that puts more content into the flags field, so
the change is justified also as pre-step to that commit
Or Gerlitz Dec. 10, 2018, 9:05 p.m. UTC | #4 
On Mon, Dec 10, 2018 at 10:09 PM Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
>
> On Mon, Dec 10, 2018 at 8:12 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> >
> > On Sun, Dec 09, 2018 at 07:04:39PM -0800, Saeed Mahameed wrote:
> > > From: Eli Britstein <elibr@mellanox.com>
> > >
> > > Change the driver flow destination struct to use bit flags with the vhca
> > > id valid being the 1st one. Done to avoid bool fields in structs, as
> > > warned by static checkers, with no functionality change.
> >
> > This is a thing now? I thought the warning was not to use bool with
> > bitfields for some reason?
> >
>
> Yea, the commit message is kinda silly,

hey, sometimes you just want to make your boss happy:

CHECK: Avoid using bool structure members because of possible
alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#99: FILE: include/linux/mlx5/fs.h:99:
+                       bool            vhca_id_valid;

missed anything?
Saeed Mahameed Dec. 10, 2018, 9:18 p.m. UTC | #5 
On Mon, Dec 10, 2018 at 1:05 PM Or Gerlitz <gerlitz.or@gmail.com> wrote:
>
> On Mon, Dec 10, 2018 at 10:09 PM Saeed Mahameed
> <saeedm@dev.mellanox.co.il> wrote:
> >
> > On Mon, Dec 10, 2018 at 8:12 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > >
> > > On Sun, Dec 09, 2018 at 07:04:39PM -0800, Saeed Mahameed wrote:
> > > > From: Eli Britstein <elibr@mellanox.com>
> > > >
> > > > Change the driver flow destination struct to use bit flags with the vhca
> > > > id valid being the 1st one. Done to avoid bool fields in structs, as
> > > > warned by static checkers, with no functionality change.
> > >
> > > This is a thing now? I thought the warning was not to use bool with
> > > bitfields for some reason?
> > >
> >
> > Yea, the commit message is kinda silly,
>
> hey, sometimes you just want to make your boss happy:
>
> CHECK: Avoid using bool structure members because of possible
> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
> #99: FILE: include/linux/mlx5/fs.h:99:
> +                       bool            vhca_id_valid;
>
> missed anything?

Looks good ! but this wasn't really the purpose of the patch so the
commit message didn't have to make a big deal of it.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 9eac137790f5..4d7b65df32ef 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -125,8 +125,9 @@  mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 				dest[i].vport.num = attr->out_rep[j]->vport;
 				dest[i].vport.vhca_id =
 					MLX5_CAP_GEN(attr->out_mdev[j], vhca_id);
-				dest[i].vport.vhca_id_valid =
-					!!MLX5_CAP_ESW(esw->dev, merged_eswitch);
+				if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
+					dest[i].vport.flags |=
+						MLX5_FLOW_DEST_VPORT_VHCA_ID;
 				i++;
 			}
 		}
@@ -220,7 +221,8 @@  mlx5_eswitch_add_fwd_rule(struct mlx5_eswitch *esw,
 		dest[i].vport.num = attr->out_rep[i]->vport;
 		dest[i].vport.vhca_id =
 			MLX5_CAP_GEN(attr->out_mdev[i], vhca_id);
-		dest[i].vport.vhca_id_valid = !!MLX5_CAP_ESW(esw->dev, merged_eswitch);
+		if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
+			dest[i].vport.flags |= MLX5_FLOW_DEST_VPORT_VHCA_ID;
 	}
 	dest[i].type = MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE;
 	dest[i].ft = fwd_fdb,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index 08a891f9aade..dda63dedaa49 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -387,7 +387,8 @@  static int mlx5_cmd_set_fte(struct mlx5_core_dev *dev,
 				id = dst->dest_attr.vport.num;
 				MLX5_SET(dest_format_struct, in_dests,
 					 destination_eswitch_owner_vhca_id_valid,
-					 dst->dest_attr.vport.vhca_id_valid);
+					 !!(dst->dest_attr.vport.flags &
+					    MLX5_FLOW_DEST_VPORT_VHCA_ID));
 				MLX5_SET(dest_format_struct, in_dests,
 					 destination_eswitch_owner_vhca_id,
 					 dst->dest_attr.vport.vhca_id);
diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
index 5660f07d3be0..25ffd8018b72 100644
--- a/include/linux/mlx5/fs.h
+++ b/include/linux/mlx5/fs.h
@@ -86,6 +86,10 @@  struct mlx5_flow_spec {
 	u32  match_value[MLX5_ST_SZ_DW(fte_match_param)];
 };
 
+enum {
+	MLX5_FLOW_DEST_VPORT_VHCA_ID      = BIT(0),
+};
+
 struct mlx5_flow_destination {
 	enum mlx5_flow_destination_type	type;
 	union {
@@ -96,7 +100,7 @@  struct mlx5_flow_destination {
 		struct {
 			u16		num;
 			u16		vhca_id;
-			bool		vhca_id_valid;
+			u8		flags;
 		} vport;
 	};
 };