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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160628134211.GK18787@gospo.rdu.cumulusnetworks.com>
Date:	Tue, 28 Jun 2016 09:42:13 -0400
From:	Andy Gospodarek <gospo@...ulusnetworks.com>
To:	Saeed Mahameed <saeedm@...lanox.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Hadar Hen-Zion <hadarh@...lanox.com>,
	Jiri Pirko <jiri@...lanox.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	John Fastabend <john.r.fastabend@...el.com>
Subject: Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode
 changes (legacy --> offloads)

On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
> From: Or Gerlitz <ogerlitz@...lanox.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 <ogerlitz@...lanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@...lanox.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;
>  }
>  
>  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!