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 <ogerl...@mellanox.com>
> >>
> >>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 <ogerl...@mellanox.com>
> >>Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
> >>---
> >> 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!
