net/mlx5: E-Switch, Add control for inline mode

bug report Dan Carpenter 2 participants 3 messages Nov 29, 2016 1 message not in the archive InfiniBand

Share & tools

Dan Carpenter

author

2016-11-29 10:52 UTC #0 lore raw

Hello Roi Dayan,

The patch bffaa916588e: "net/mlx5: E-Switch, Add control for inline mode" from Nov 22, 2016, leads to the following static checker warning:

drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c:950 mlx5_eswitch_inline_mode_get()
error: uninitialized symbol 'prev_mlx5_mode'.

drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
932 int mlx5_eswitch_inline_mode_get(struct mlx5_eswitch *esw, int nvfs, u8 *mode) 933 {

   934          struct mlx5_core_dev *dev = esw->dev;
   935          int vport;
   936          u8 prev_mlx5_mode, mlx5_mode = MLX5_INLINE_MODE_L2;
                   ^^^^^^^^^^^^^^
   937  
   938          if (!MLX5_CAP_GEN(dev, vport_group_manager))
   939                  return -EOPNOTSUPP;
   940  
   941          if (esw->mode == SRIOV_NONE)
   942                  return -EOPNOTSUPP;
   943  
   944          if (MLX5_CAP_ETH(dev, wqe_inline_mode) !=
   945              MLX5_CAP_INLINE_MODE_VPORT_CONTEXT)
   946                  return -EOPNOTSUPP;
   947  
   948          for (vport = 1; vport <= nvfs; vport++) {
   949                  mlx5_query_nic_vport_min_inline(dev, vport, &mlx5_mode);
   950                  if (vport > 1 && prev_mlx5_mode != mlx5_mode)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^

No idea what prev_mlx5_mode is supposed to be for.

   951                          return -EINVAL;
   952                  prev_mlx5_mode = mlx5_mode;
   953          }
   954  
   955          *mode = mlx5_mode;
   956          return 0;
   957  }

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Roi Dayan

2016-11-29 15:06 UTC #1 lore raw

Quoted 41 lines · Dan Carpenter

Hello Roi Dayan,

The patch bffaa916588e: "net/mlx5: E-Switch, Add control for inline mode" from Nov 22, 2016, leads to the following static checker warning:

drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c:950 mlx5_eswitch_inline_mode_get()
error: uninitialized symbol 'prev_mlx5_mode'.

drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c

    932  int mlx5_eswitch_inline_mode_get(struct mlx5_eswitch *esw, int nvfs, u8 *mode)
    933  {
    934          struct mlx5_core_dev *dev = esw->dev;
    935          int vport;
    936          u8 prev_mlx5_mode, mlx5_mode = MLX5_INLINE_MODE_L2;
                    ^^^^^^^^^^^^^^
    937
    938          if (!MLX5_CAP_GEN(dev, vport_group_manager))
    939                  return -EOPNOTSUPP;
    940
    941          if (esw->mode == SRIOV_NONE)
    942                  return -EOPNOTSUPP;
    943
    944          if (MLX5_CAP_ETH(dev, wqe_inline_mode) !=
    945              MLX5_CAP_INLINE_MODE_VPORT_CONTEXT)
    946                  return -EOPNOTSUPP;
    947
    948          for (vport = 1; vport <= nvfs; vport++) {
    949                  mlx5_query_nic_vport_min_inline(dev, vport, &mlx5_mode);
    950                  if (vport > 1 && prev_mlx5_mode != mlx5_mode)
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^

No idea what prev_mlx5_mode is supposed to be for.

    951                          return -EINVAL;
    952                  prev_mlx5_mode = mlx5_mode;
    953          }
    954
    955          *mode = mlx5_mode;
    956          return 0;
    957  }

Quoted 2 lines

regards,
dan carpenter

Hi Dan,

I really didn't see this. sparse and smatch didn't catch this. I use FC24 and gcc 6.2.1 for compilation and it doesn't give me any warning. Did another check with an old rhel machine with gcc 4.4.6 and I got a warning about it there.
Can you tell me which tool you used?

This is a false positive because of the first check if vport > 1, we actually check prev_mlx5_mode from the second for-loop iteration and on the first loop iteration we set a value (in line 952).
It is used to check if all vports are set with the same inline mode.

Thanks,
Roi

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): fa981369-2032-eb9e-6307-f3055297e7e6-VPRAkNaXOzVWk0Htik3J/[email protected]

Dan Carpenter

author

2016-11-29 21:25 UTC #3 lore raw

Quoted 6 lines · Roi Dayan

I really didn't see this. sparse and smatch didn't catch this. I use FC24 and gcc 6.2.1 for compilation and it doesn't give me any warning.
Did another check with an old rhel machine with gcc 4.4.6 and I got a warning about it there.
Can you tell me which tool you used?

Some new unreleased Smatch stuff I'm working on.

Quoted 5 lines

This is a false positive because of the first check if vport > 1, we actually check prev_mlx5_mode from the second for-loop iteration and on the first loop iteration we set a value (in line 952). It is used to check if all vports are set with the same inline mode.

Oh crap... Smatch takes a short cut handling loops (I really need to fix this but I wrote the orginal code a decade ago). I misread how the loops works as well. Sorry for the noise.