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

On Tue, Jun 28, 2016 at 05:25:11PM +0300, Or Gerlitz wrote:

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

quoted

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

quoted

From: Or Gerlitz <redacted>

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 <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c  |  12 ++-
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++++++-
 2 files changed, 105 insertions(+), 9 deletions(-)

[...]

quoted

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

[...]

quoted

 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.

quoted

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.  :-)

quoted

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!