| Message ID | 1467043649-28594-11-git-send-email-saeedm@mellanox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote: > From: Or Gerlitz <ogerlitz@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 <ogerlitz@mellanox.com> > Signed-off-by: Saeed Mahameed <saeedm@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; > } > > 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!
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 <ogerlitz@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 <ogerlitz@mellanox.com> >> Signed-off-by: Saeed Mahameed <saeedm@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. > 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!
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 <ogerlitz@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 <ogerlitz@mellanox.com> > >>Signed-off-by: Saeed Mahameed <saeedm@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!
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c index 1fc4cfd..12f509c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c @@ -81,8 +81,8 @@ enum { MC_ADDR_CHANGE | \ PROMISC_CHANGE) -int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports); -void esw_destroy_offloads_fdb_table(struct mlx5_eswitch *esw); +int esw_offloads_init(struct mlx5_eswitch *esw, int nvports); +void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports); static int arm_vport_context_events_cmd(struct mlx5_core_dev *dev, u16 vport, u32 events_mask) @@ -1561,7 +1561,7 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode) if (mode == SRIOV_LEGACY) err = esw_create_legacy_fdb_table(esw, nvfs + 1); else - err = esw_create_offloads_fdb_table(esw, nvfs + 1); + err = esw_offloads_init(esw, nvfs + 1); if (err) goto abort; @@ -1581,6 +1581,7 @@ abort: void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw) { struct esw_mc_addr *mc_promisc; + int nvports; int i; if (!esw || !MLX5_CAP_GEN(esw->dev, vport_group_manager) || @@ -1591,6 +1592,7 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw) esw->enabled_vports, esw->mode); mc_promisc = esw->mc_promisc; + nvports = esw->enabled_vports; for (i = 0; i < esw->total_vports; i++) esw_disable_vport(esw, i); @@ -1600,8 +1602,8 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw) if (esw->mode == SRIOV_LEGACY) esw_destroy_legacy_fdb_table(esw); - else - esw_destroy_offloads_fdb_table(esw); + else if (esw->mode == SRIOV_OFFLOADS) + esw_offloads_cleanup(esw, nvports); esw->mode = SRIOV_NONE; /* VPORT 0 (PF) must be enabled back with non-sriov configuration */ 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 @@ -114,7 +114,7 @@ out: #define MAX_PF_SQ 256 -int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) +static int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports) { int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in); struct mlx5_core_dev *dev = esw->dev; @@ -202,7 +202,7 @@ ns_err: return err; } -void esw_destroy_offloads_fdb_table(struct mlx5_eswitch *esw) +static void esw_destroy_offloads_fdb_table(struct mlx5_eswitch *esw) { if (!esw->fdb_table.fdb) return; @@ -331,12 +331,106 @@ out: return flow_rule; } +static int esw_offloads_start(struct mlx5_eswitch *esw) +{ + int err, num_vfs = esw->dev->priv.sriov.num_vfs; + + if (esw->mode != SRIOV_LEGACY) { + esw_warn(esw->dev, "Can't set offloads mode, SRIOV legacy not enabled\n"); + return -EINVAL; + } + + mlx5_eswitch_disable_sriov(esw); + err = mlx5_eswitch_enable_sriov(esw, num_vfs, SRIOV_OFFLOADS); + if (err) + esw_warn(esw->dev, "Failed set eswitch to offloads, err %d\n", err); + return err; +} + +int esw_offloads_init(struct mlx5_eswitch *esw, int nvports) +{ + int err; + + err = esw_create_offloads_fdb_table(esw, nvports); + if (err) + return err; + + err = esw_create_offloads_table(esw); + if (err) + goto create_ft_err; + + err = esw_create_vport_rx_group(esw); + if (err) + goto create_fg_err; + + return 0; + +create_fg_err: + esw_destroy_offloads_table(esw); + +create_ft_err: + esw_destroy_offloads_fdb_table(esw); + return err; +} + +static int esw_offloads_stop(struct mlx5_eswitch *esw) +{ + int err, num_vfs = esw->dev->priv.sriov.num_vfs; + + mlx5_eswitch_disable_sriov(esw); + err = mlx5_eswitch_enable_sriov(esw, num_vfs, SRIOV_LEGACY); + if (err) + esw_warn(esw->dev, "Failed set eswitch legacy mode. err %d\n", err); + + return err; +} + +void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports) +{ + esw_destroy_vport_rx_group(esw); + esw_destroy_offloads_table(esw); + esw_destroy_offloads_fdb_table(esw); +} + 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; }