Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode changes (legacy --> offloads)

2016-06-28 Thread Andy Gospodarek 
On Tue, Jun 28, 2016 at 05:25:11PM +0300, Or Gerlitz wrote:
> On 6/28/2016 4:42 PM, Andy Gospodarek wrote:
> >On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
> >>From: Or Gerlitz 
> >>
> >>Implement handlers for the devlink commands to get and set the SRIOV
> >>E-Switch mode.
> >>
> >>When turning to the offloads mode, we disable the e-switch and enable
> >>it again in the new mode, create the NIC offloads table and create VF reps.
> >>
> >>When turning to legacy mode, we remove the VF reps and the offloads
> >>table, and re-initiate the e-switch in it's legacy mode.
> >>
> >>The actual creation/removal of the VF reps is done in downstream patches.
> >>
> >>Signed-off-by: Or Gerlitz 
> >>Signed-off-by: Saeed Mahameed 
> >>---
> >>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.c  |  12 ++-
> >>  .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 
> >> -
> >>  2 files changed, 105 insertions(+), 9 deletions(-)
> >>
> >[...]
> >>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
> >>b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> >>index 3b3afbd..a39af6b 100644
> >>--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> >>+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> >[...]
> >>  int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
> >>  {
> >>-   return -EOPNOTSUPP;
> >>+   struct mlx5_core_dev *dev;
> >>+   u16 cur_mode;
> >>+
> >>+   dev = devlink_priv(devlink);
> >>+
> >>+   if (!MLX5_CAP_GEN(dev, vport_group_manager))
> >>+   return -EOPNOTSUPP;
> >>+
> >>+   cur_mode = dev->priv.eswitch->mode;
> >>+
> >>+   if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
> >>+   return -EOPNOTSUPP;
> >>+
> >>+   if (cur_mode == mode)
> >>+   return 0;
> >>+
> >>+   if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
> >>+   return esw_offloads_start(dev->priv.eswitch);
> >>+   else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
> >>+   return esw_offloads_stop(dev->priv.eswitch);
> >>+   else
> >>+   return -EINVAL;
> >>  }
> >>
> >>This is an _extremely_ minor nit, but I only bring it up since you are
> >>leading the way here and your model may be one that other people
> >>follow...
> >>
> >>Internally you have a enum to track the SRIOV modes:
> >>
> >>enum {
> >>SRIOV_NONE,
> >>SRIOV_LEGACY,
> >>SRIOV_OFFLOADS
> >>};
> >>
> >>But patch 8 adds a new enum for devlink to track this as well.
> >>
> >>enum devlink_eswitch_mode {
> >>DEVLINK_ESWITCH_MODE_NONE,
> >>DEVLINK_ESWITCH_MODE_LEGACY,
> >>DEVLINK_ESWITCH_MODE_OFFLOADS,
> >>};
> >>
> 
> 
> Andy,
> 
> In mlx5 we're having an eswitch driver instance also when not in sriov mode
> where on that case the mlx5 eswitch mode is called sriov_none, which is
> maybe not a very successful name, I'll look on that.
> 
> On the devlink/system level, the eswitch modes are relevant only for SRIOV,
> you can see in the mlx5 set function that we return error when in the none
> mode or asked to go there.
> 
> So... with your comment,  I realize now that I forgot to remove
> DEVLINK_ESWITCH_MODE_NONE value from the submission.
> 
> >Would it make sense at some point to use the devlink modes in the driver
> >so it's less to track?
> 
> This makes it a bit problematic for mlx5 to use the DEVLINK_ESWITCH_MODE_YYY
> values internally.

If you planned to remove DEVLINK_ESWITCH_MODE_NONE then I could see how
using these in mlx5 would be problematic.  Thinking about it for just a
minute, I can see the value dropping DEVLINK_ESWITCH_MODE_NONE.  If the
driver supports the ability to set the eswitch mode, then it should
report an actual mode other than none.

If you remove DEVLINK_ESWITCH_MODE_NONE, then obviously
mlx5_devlink_eswitch_mode_set/get will need to change a bit as well as
there will need to be a mapping between the two values since the enums
would no longer be the same.  Easy fix, though.  :-)

> >Again, this is an extremely _minor_ concern.  The rest of the set looks
> >great and I like the architectural decisions made here.  Awesome work
> >all around!

Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode changes (legacy --> offloads)

2016-06-28 Thread Or Gerlitz 

On 6/28/2016 4:42 PM, Andy Gospodarek wrote:

On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:

From: Or Gerlitz 

Implement handlers for the devlink commands to get and set the SRIOV
E-Switch mode.

When turning to the offloads mode, we disable the e-switch and enable
it again in the new mode, create the NIC offloads table and create VF reps.

When turning to legacy mode, we remove the VF reps and the offloads
table, and re-initiate the e-switch in it's legacy mode.

The actual creation/removal of the VF reps is done in downstream patches.

Signed-off-by: Or Gerlitz 
Signed-off-by: Saeed Mahameed 
---
  drivers/net/ethernet/mellanox/mlx5/core/eswitch.c  |  12 ++-
  .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 -
  2 files changed, 105 insertions(+), 9 deletions(-)


[...]

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 3b3afbd..a39af6b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c

[...]

  int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
  {
-   return -EOPNOTSUPP;
+   struct mlx5_core_dev *dev;
+   u16 cur_mode;
+
+   dev = devlink_priv(devlink);
+
+   if (!MLX5_CAP_GEN(dev, vport_group_manager))
+   return -EOPNOTSUPP;
+
+   cur_mode = dev->priv.eswitch->mode;
+
+   if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
+   return -EOPNOTSUPP;
+
+   if (cur_mode == mode)
+   return 0;
+
+   if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
+   return esw_offloads_start(dev->priv.eswitch);
+   else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
+   return esw_offloads_stop(dev->priv.eswitch);
+   else
+   return -EINVAL;
  }
  


This is an _extremely_ minor nit, but I only bring it up since you are
leading the way here and your model may be one that other people
follow...

Internally you have a enum to track the SRIOV modes:

enum {
SRIOV_NONE,
SRIOV_LEGACY,
SRIOV_OFFLOADS
};

But patch 8 adds a new enum for devlink to track this as well.

enum devlink_eswitch_mode {
DEVLINK_ESWITCH_MODE_NONE,
DEVLINK_ESWITCH_MODE_LEGACY,
DEVLINK_ESWITCH_MODE_OFFLOADS,
};




Andy,

In mlx5 we're having an eswitch driver instance also when not in sriov 
mode where on that case the mlx5 eswitch mode is called sriov_none, 
which is maybe not a very successful name, I'll look on that.


On the devlink/system level, the eswitch modes are relevant only for 
SRIOV, you can see in the mlx5 set function that we return error when in 
the none mode or asked to go there.


So... with your comment,  I realize now that I forgot to remove 
DEVLINK_ESWITCH_MODE_NONE value from the submission.



Would it make sense at some point to use the devlink modes in the driver
so it's less to track?


This makes it a bit problematic for mlx5 to use the 
DEVLINK_ESWITCH_MODE_YYY values internally.



Again, this is an extremely _minor_ concern.  The rest of the set looks
great and I like the architectural decisions made here.  Awesome work
all around!

Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode changes (legacy --> offloads)

2016-06-28 Thread Andy Gospodarek 
On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
> From: Or Gerlitz 
> 
> Implement handlers for the devlink commands to get and set the SRIOV
> E-Switch mode.
> 
> When turning to the offloads mode, we disable the e-switch and enable
> it again in the new mode, create the NIC offloads table and create VF reps.
> 
> When turning to legacy mode, we remove the VF reps and the offloads
> table, and re-initiate the e-switch in it's legacy mode.
> 
> The actual creation/removal of the VF reps is done in downstream patches.
> 
> Signed-off-by: Or Gerlitz 
> Signed-off-by: Saeed Mahameed 
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.c  |  12 ++-
>  .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 
> -
>  2 files changed, 105 insertions(+), 9 deletions(-)
> 
[...]
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 3b3afbd..a39af6b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
[...]
>  int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
>  {
> - return -EOPNOTSUPP;
> + struct mlx5_core_dev *dev;
> + u16 cur_mode;
> +
> + dev = devlink_priv(devlink);
> +
> + if (!MLX5_CAP_GEN(dev, vport_group_manager))
> + return -EOPNOTSUPP;
> +
> + cur_mode = dev->priv.eswitch->mode;
> +
> + if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
> + return -EOPNOTSUPP;
> +
> + if (cur_mode == mode)
> + return 0;
> +
> + if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
> + return esw_offloads_start(dev->priv.eswitch);
> + else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
> + return esw_offloads_stop(dev->priv.eswitch);
> + else
> + return -EINVAL;
>  }
>  
>  int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
>  {
> - return -EOPNOTSUPP;
> + struct mlx5_core_dev *dev;
> +
> + dev = devlink_priv(devlink);
> +
> + if (!MLX5_CAP_GEN(dev, vport_group_manager))
> + return -EOPNOTSUPP;
> +
> + if (dev->priv.eswitch->mode == SRIOV_NONE)
> + return -EOPNOTSUPP;
> +
> + *mode = dev->priv.eswitch->mode;
> +
> + return 0;
>  }

This is an _extremely_ minor nit, but I only bring it up since you are
leading the way here and your model may be one that other people
follow...

Internally you have a enum to track the SRIOV modes:

enum {
   SRIOV_NONE,
   SRIOV_LEGACY,
   SRIOV_OFFLOADS
};

But patch 8 adds a new enum for devlink to track this as well.

enum devlink_eswitch_mode {
   DEVLINK_ESWITCH_MODE_NONE,
   DEVLINK_ESWITCH_MODE_LEGACY,
   DEVLINK_ESWITCH_MODE_OFFLOADS,
};

Would it make sense at some point to use the devlink modes in the driver
so it's less to track?

Again, this is an extremely _minor_ concern.  The rest of the set looks
great and I like the architectural decisions made here.  Awesome work
all around!