From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Gospodarek Subject: Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode changes (legacy --> offloads) Date: Tue, 28 Jun 2016 10:49:29 -0400 Message-ID: <20160628144928.GL18787@gospo.rdu.cumulusnetworks.com> References: <1467043649-28594-1-git-send-email-saeedm@mellanox.com> <1467043649-28594-11-git-send-email-saeedm@mellanox.com> <20160628134211.GK18787@gospo.rdu.cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Saeed Mahameed , "David S. Miller" , netdev@vger.kernel.org, Hadar Hen-Zion , Jiri Pirko , Jesse Brandeburg , John Fastabend To: Or Gerlitz Return-path: Received: from mail-qk0-f172.google.com ([209.85.220.172]:34031 "EHLO mail-qk0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752363AbcF1Ote (ORCPT ); Tue, 28 Jun 2016 10:49:34 -0400 Received: by mail-qk0-f172.google.com with SMTP id t127so33321990qkf.1 for ; Tue, 28 Jun 2016 07:49:33 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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!