| Message ID | 1600445211-31078-1-git-send-email-moshe@mellanox.com |
|---|---|
| Headers | show |
| Series | Add devlink reload action and limit level options | expand |
I see no more comments, we reached consensus on API. I will finalize my work and re-send as a feature to net-next. Thanks, Moshe. On 9/18/2020 7:06 PM, Moshe Shemesh wrote: > Introduce new options on devlink reload API to enable the user to select > the reload action required and contrains limits on these actions that he > may want to ensure. Complete support for reload actions in mlx5. > The following reload actions are supported: > driver_reinit: driver entities re-initialization, applying devlink-param > and devlink-resource values. > fw_activate: firmware activate. > > The uAPI is backward compatible, if the reload action option is omitted > from the reload command, the driver reinit action will be used. > Note that when required to do firmware activation some drivers may need > to reload the driver. On the other hand some drivers may need to reset > the firmware to reinitialize the driver entities. Therefore, the devlink > reload command returns the actions which were actually performed. > > By default reload actions are not limited and driver implementation may > include reset or downtime as needed to perform the actions. > However, if limit_level is selected, the driver should perform only if > it can do it while keeping the limit level constrains. > Reload action limit level added: > no_reset: No reset allowed, no down time allowed, no link flap and no > configuration is lost. > > Each driver which supports devlink reload command should expose the > reload actions and limit levels supported. > > Add reload action stats to hold the history per reload action per limit > level. For example, the number of times fw_activate has been done on > this device since the driver module was added or if the firmware > activation was done with or without reset. > > Patch 1-2 add the new API reload action and reload action limit level > option to devlink reload. > Patch 3 adds reload actions stats. > Patch 4 exposes the reload actions stats on devlink dev get. > Patches 5-10 add support on mlx5 for devlink reload action fw_activate > and handle the firmware reset events. > Patches 11-12 add devlink enable remote dev reset parameter and use it > in mlx5. > Patches 13-14 mlx5 add devlink reload action limit level no_reset > support for fw_activate reload action. > Patch 15 adds documentation file devlink-reload.rst > > > Moshe Shemesh (15): > devlink: Add reload action option to devlink reload command > devlink: Add reload action limit level > devlink: Add reload action stats > devlink: Add reload actions stats to dev get > net/mlx5: Add functions to set/query MFRL register > net/mlx5: Set cap for pci sync for fw update event > net/mlx5: Handle sync reset request event > net/mlx5: Handle sync reset now event > net/mlx5: Handle sync reset abort event > net/mlx5: Add support for devlink reload action fw activate > devlink: Add enable_remote_dev_reset generic parameter > net/mlx5: Add devlink param enable_remote_dev_reset support > net/mlx5: Add support for fw live patch event > net/mlx5: Add support for devlink reload action limit level no reset > devlink: Add Documentation/networking/devlink/devlink-reload.rst > > .../networking/devlink/devlink-params.rst | 6 + > .../networking/devlink/devlink-reload.rst | 79 +++ > Documentation/networking/devlink/index.rst | 1 + > drivers/net/ethernet/mellanox/mlx4/main.c | 16 +- > .../net/ethernet/mellanox/mlx5/core/Makefile | 2 +- > .../net/ethernet/mellanox/mlx5/core/devlink.c | 122 ++++- > .../mellanox/mlx5/core/diag/fw_tracer.c | 31 ++ > .../mellanox/mlx5/core/diag/fw_tracer.h | 1 + > .../ethernet/mellanox/mlx5/core/fw_reset.c | 454 ++++++++++++++++++ > .../ethernet/mellanox/mlx5/core/fw_reset.h | 19 + > .../net/ethernet/mellanox/mlx5/core/health.c | 35 +- > .../net/ethernet/mellanox/mlx5/core/main.c | 13 + > .../ethernet/mellanox/mlx5/core/mlx5_core.h | 2 + > drivers/net/ethernet/mellanox/mlxsw/core.c | 27 +- > drivers/net/netdevsim/dev.c | 17 +- > include/linux/mlx5/device.h | 1 + > include/linux/mlx5/driver.h | 4 + > include/net/devlink.h | 21 +- > include/uapi/linux/devlink.h | 41 ++ > net/core/devlink.c | 339 ++++++++++++- > 20 files changed, 1179 insertions(+), 52 deletions(-) > create mode 100644 Documentation/networking/devlink/devlink-reload.rst > create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c > create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/fw_reset.h >
On Fri, 18 Sep 2020 19:06:38 +0300 Moshe Shemesh wrote: > Add reload action limit level to demand restrictions on actions. > Reload action limit levels supported: > none (default): No constrains on actions. Driver implementation may > include reset or downtime as needed to perform the > actions. > no_reset: No reset allowed, no down time allowed, no link flap and no > configuration is lost. > > Some combinations of action and limit level are invalid. For example, > driver can not reinitialize its entities without any downtime. > > The no_reset limit level will have usecase in this patchset to > implement restricted fw_activate on mlx5. > > Signed-off-by: Moshe Shemesh <moshe@mellanox.com> > --- > v4 -> v5: > - Remove check DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX > - Added list of invalid action-limit_level combinations and add check to > supported actions and levels and check user request > v3 -> v4: > - New patch > --- > drivers/net/ethernet/mellanox/mlx4/main.c | 3 + > .../net/ethernet/mellanox/mlx5/core/devlink.c | 3 + > drivers/net/ethernet/mellanox/mlxsw/core.c | 3 + > drivers/net/netdevsim/dev.c | 6 +- > include/net/devlink.h | 6 +- > include/uapi/linux/devlink.h | 17 +++++ > net/core/devlink.c | 76 +++++++++++++++++-- > 7 files changed, 107 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > index 1a482120cc0a..f0ef295af477 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/main.c > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > @@ -3947,6 +3947,7 @@ static int mlx4_restart_one_up(struct pci_dev *pdev, bool reload, > > static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, > enum devlink_reload_action action, > + enum devlink_reload_action_limit_level limit_level, > struct netlink_ext_ack *extack) > { > struct mlx4_priv *priv = devlink_priv(devlink); > @@ -3964,6 +3965,7 @@ static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, > } > > static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_action action, > + enum devlink_reload_action_limit_level limit_level, > struct netlink_ext_ack *extack, unsigned long *actions_performed) > { > struct mlx4_priv *priv = devlink_priv(devlink); > @@ -3985,6 +3987,7 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_a > static const struct devlink_ops mlx4_devlink_ops = { > .port_type_set = mlx4_devlink_port_type_set, > .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT), > + .supported_reload_action_limit_levels = BIT(DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE), Please cut down the name lenghts, this is just lazy. 'supported_reload_limits' or 'cap_reload_limits' is perfectly sufficient. 'reload_limits' would be even better. Cause what else would it be if not a capability. Besides I don't think drivers should have to fill negative attributes (that they don't support something). Everyone is always going to support NONE, since it's "unspecified" / "pick your favorite", right? > .reload_down = mlx4_devlink_reload_down, > .reload_up = mlx4_devlink_reload_up, > }; > diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > index fdba7ab58a79..0c5d942dcbd5 100644 > --- a/include/uapi/linux/devlink.h > +++ b/include/uapi/linux/devlink.h > @@ -289,6 +289,22 @@ enum devlink_reload_action { > DEVLINK_RELOAD_ACTION_MAX = __DEVLINK_RELOAD_ACTION_MAX - 1 > }; > > +/** > + * enum devlink_reload_action_limit_level - Reload action limit level. > + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE: No constrains on action. Action may include > + * reset or downtime as needed. > + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET: No reset allowed, no down time allowed, > + * no link flap and no configuration is lost. > + */ > +enum devlink_reload_action_limit_level { You reserved UNSPEC for actions but not for limit level? > + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE, > + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET, > + > + /* Add new reload actions limit level above */ > + __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX, > + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX = __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX - 1 > +}; > + > enum devlink_attr { > /* don't change the order or add anything between, this is ABI! */ > DEVLINK_ATTR_UNSPEC, > @@ -480,6 +496,7 @@ enum devlink_attr { > > DEVLINK_ATTR_RELOAD_ACTION, /* u8 */ > DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED, /* nested */ > + DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, /* u8 */ > > /* add new attributes above here, update the policy in devlink.c */ > > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 318ef29f81f2..fee6fcc7dead 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -462,12 +462,45 @@ static int devlink_nl_put_handle(struct sk_buff *msg, struct devlink *devlink) > return 0; > } > > +struct devlink_reload_combination { > + enum devlink_reload_action action; > + enum devlink_reload_action_limit_level limit_level; > +}; > + > +static const struct devlink_reload_combination devlink_reload_invalid_combinations[] = { > + { > + /* can't reinitialize driver with no down time */ > + .action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT, > + .limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET, > + }, > +}; > + > +static bool > +devlink_reload_combination_is_invalid(enum devlink_reload_action action, > + enum devlink_reload_action_limit_level limit_level) > +{ > + int i; > + > + for (i = 0 ; i < ARRAY_SIZE(devlink_reload_invalid_combinations) ; i++) Whitespace. Did you checkpatch? > + if (devlink_reload_invalid_combinations[i].action == action && > + devlink_reload_invalid_combinations[i].limit_level == limit_level) > + return true; > + return false; > +} > + > static bool > devlink_reload_action_is_supported(struct devlink *devlink, enum devlink_reload_action action) > { > return test_bit(action, &devlink->ops->supported_reload_actions); > } > > +static bool > +devlink_reload_action_limit_level_is_supported(struct devlink *devlink, > + enum devlink_reload_action_limit_level limit_level) > +{ > + return test_bit(limit_level, &devlink->ops->supported_reload_action_limit_levels); > +} This single-use helper just grows LoC and muddies the code IMHO. > static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink, > enum devlink_command cmd, u32 portid, > u32 seq, int flags) > @@ -2975,22 +3008,23 @@ bool devlink_is_reload_failed(const struct devlink *devlink) > EXPORT_SYMBOL_GPL(devlink_is_reload_failed); > > static int devlink_reload(struct devlink *devlink, struct net *dest_net, > - enum devlink_reload_action action, struct netlink_ext_ack *extack, > - unsigned long *actions_performed) > + enum devlink_reload_action action, > + enum devlink_reload_action_limit_level limit_level, > + struct netlink_ext_ack *extack, unsigned long *actions_performed) > { > int err; > > if (!devlink->reload_enabled) > return -EOPNOTSUPP; > > - err = devlink->ops->reload_down(devlink, !!dest_net, action, extack); > + err = devlink->ops->reload_down(devlink, !!dest_net, action, limit_level, extack); > if (err) > return err; > > if (dest_net && !net_eq(dest_net, devlink_net(devlink))) > devlink_reload_netns_change(devlink, dest_net); > > - err = devlink->ops->reload_up(devlink, action, extack, actions_performed); > + err = devlink->ops->reload_up(devlink, action, limit_level, extack, actions_performed); > devlink_reload_failed_set(devlink, !!err); > if (err) > return err; > @@ -3040,6 +3074,7 @@ devlink_nl_reload_actions_performed_fill(struct sk_buff *msg, > > static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) > { > + enum devlink_reload_action_limit_level limit_level; > struct devlink *devlink = info->user_ptr[0]; > enum devlink_reload_action action; > unsigned long actions_performed; > @@ -3077,7 +3112,21 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) > return -EOPNOTSUPP; > } > > - err = devlink_reload(devlink, dest_net, action, info->extack, &actions_performed); > + if (info->attrs[DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL]) > + limit_level = nla_get_u8(info->attrs[DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL]); > + else > + limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE; > + > + if (!devlink_reload_action_limit_level_is_supported(devlink, limit_level)) { > + NL_SET_ERR_MSG_MOD(info->extack, "Requested limit level is not supported by the driver"); > + return -EOPNOTSUPP; > + } > + if (devlink_reload_combination_is_invalid(action, limit_level)) { > + NL_SET_ERR_MSG_MOD(info->extack, "Requested limit level is invalid for this action"); > + return -EINVAL; > + } > + err = devlink_reload(devlink, dest_net, action, limit_level, info->extack, > + &actions_performed); > > if (dest_net) > put_net(dest_net); > @@ -7154,6 +7203,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { > [DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 }, > [DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED }, > [DEVLINK_ATTR_RELOAD_ACTION] = { .type = NLA_U8 }, > + [DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL] = { .type = NLA_U8 }, range > }; > > static const struct genl_ops devlink_nl_ops[] = { > @@ -7489,6 +7539,9 @@ static struct genl_family devlink_nl_family __ro_after_init = { > > static bool devlink_reload_actions_valid(const struct devlink_ops *ops) > { > + const struct devlink_reload_combination *comb; > + int i; > + > if (!devlink_reload_supported(ops)) { > if (WARN_ON(ops->supported_reload_actions)) > return false; > @@ -7498,6 +7551,18 @@ static bool devlink_reload_actions_valid(const struct devlink_ops *ops) > if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || > ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) > return false; > + > + if (WARN_ON(!ops->supported_reload_action_limit_levels || > + ops->supported_reload_action_limit_levels >= > + BIT(__DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX))) > + return false; > + > + for (i = 0; i < ARRAY_SIZE(devlink_reload_invalid_combinations); i++) { White space, again. > + comb = &devlink_reload_invalid_combinations[i]; > + if (ops->supported_reload_actions == BIT(comb->action) && > + ops->supported_reload_action_limit_levels == BIT(comb->limit_level)) > + return false; > + } > return true; > } > > @@ -9793,6 +9858,7 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) > continue; > err = devlink_reload(devlink, &init_net, > DEVLINK_RELOAD_ACTION_DRIVER_REINIT, > + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE, > NULL, &actions_performed); > if (err && err != -EOPNOTSUPP) > pr_warn("Failed to reload devlink instance into init_net\n");
On 9/23/2020 9:36 PM, Jakub Kicinski wrote: > On Fri, 18 Sep 2020 19:06:38 +0300 Moshe Shemesh wrote: >> Add reload action limit level to demand restrictions on actions. >> Reload action limit levels supported: >> none (default): No constrains on actions. Driver implementation may >> include reset or downtime as needed to perform the >> actions. >> no_reset: No reset allowed, no down time allowed, no link flap and no >> configuration is lost. >> >> Some combinations of action and limit level are invalid. For example, >> driver can not reinitialize its entities without any downtime. >> >> The no_reset limit level will have usecase in this patchset to >> implement restricted fw_activate on mlx5. >> >> Signed-off-by: Moshe Shemesh <moshe@mellanox.com> >> --- >> v4 -> v5: >> - Remove check DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX >> - Added list of invalid action-limit_level combinations and add check to >> supported actions and levels and check user request >> v3 -> v4: >> - New patch >> --- >> drivers/net/ethernet/mellanox/mlx4/main.c | 3 + >> .../net/ethernet/mellanox/mlx5/core/devlink.c | 3 + >> drivers/net/ethernet/mellanox/mlxsw/core.c | 3 + >> drivers/net/netdevsim/dev.c | 6 +- >> include/net/devlink.h | 6 +- >> include/uapi/linux/devlink.h | 17 +++++ >> net/core/devlink.c | 76 +++++++++++++++++-- >> 7 files changed, 107 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c >> index 1a482120cc0a..f0ef295af477 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/main.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c >> @@ -3947,6 +3947,7 @@ static int mlx4_restart_one_up(struct pci_dev *pdev, bool reload, >> >> static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, >> enum devlink_reload_action action, >> + enum devlink_reload_action_limit_level limit_level, >> struct netlink_ext_ack *extack) >> { >> struct mlx4_priv *priv = devlink_priv(devlink); >> @@ -3964,6 +3965,7 @@ static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, >> } >> >> static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_action action, >> + enum devlink_reload_action_limit_level limit_level, >> struct netlink_ext_ack *extack, unsigned long *actions_performed) >> { >> struct mlx4_priv *priv = devlink_priv(devlink); >> @@ -3985,6 +3987,7 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_a >> static const struct devlink_ops mlx4_devlink_ops = { >> .port_type_set = mlx4_devlink_port_type_set, >> .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT), >> + .supported_reload_action_limit_levels = BIT(DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE), > Please cut down the name lenghts, this is just lazy. > > 'supported_reload_limits' or 'cap_reload_limits' is perfectly > sufficient. > > 'reload_limits' would be even better. Cause what else would it be if > not a capability. Sounds good. So instead of supported_reload_actions_limit_levels will have reload_limits. Instead of supported_reload_actions will have reload_actions, OK ? May also use reload_limit_level instead of reload_action_limit_level everywhere if its clear enough. > Besides I don't think drivers should have to fill negative attributes > (that they don't support something). Everyone is always going to > support NONE, since it's "unspecified" / "pick your favorite", right? Good point, will remove it. >> .reload_down = mlx4_devlink_reload_down, >> .reload_up = mlx4_devlink_reload_up, >> }; >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> index fdba7ab58a79..0c5d942dcbd5 100644 >> --- a/include/uapi/linux/devlink.h >> +++ b/include/uapi/linux/devlink.h >> @@ -289,6 +289,22 @@ enum devlink_reload_action { >> DEVLINK_RELOAD_ACTION_MAX = __DEVLINK_RELOAD_ACTION_MAX - 1 >> }; >> >> +/** >> + * enum devlink_reload_action_limit_level - Reload action limit level. >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE: No constrains on action. Action may include >> + * reset or downtime as needed. >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET: No reset allowed, no down time allowed, >> + * no link flap and no configuration is lost. >> + */ >> +enum devlink_reload_action_limit_level { > You reserved UNSPEC for actions but not for limit level? Yes, I used LIMIT_LEVEL_NONE = 0 as no limit needed, so I skipped UNSPEC. Maybe should add UNSPEC and use UNSPEC as no limit needed. But UNSPEC is kind of invalid. >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE, >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET, >> + >> + /* Add new reload actions limit level above */ >> + __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX, >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX = __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX - 1 >> +}; >> + >> enum devlink_attr { >> /* don't change the order or add anything between, this is ABI! */ >> DEVLINK_ATTR_UNSPEC, >> @@ -480,6 +496,7 @@ enum devlink_attr { >> >> DEVLINK_ATTR_RELOAD_ACTION, /* u8 */ >> DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED, /* nested */ >> + DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, /* u8 */ >> >> /* add new attributes above here, update the policy in devlink.c */ >> >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index 318ef29f81f2..fee6fcc7dead 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -462,12 +462,45 @@ static int devlink_nl_put_handle(struct sk_buff *msg, struct devlink *devlink) >> return 0; >> } >> >> +struct devlink_reload_combination { >> + enum devlink_reload_action action; >> + enum devlink_reload_action_limit_level limit_level; >> +}; >> + >> +static const struct devlink_reload_combination devlink_reload_invalid_combinations[] = { >> + { >> + /* can't reinitialize driver with no down time */ >> + .action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT, >> + .limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET, >> + }, >> +}; >> + >> +static bool >> +devlink_reload_combination_is_invalid(enum devlink_reload_action action, >> + enum devlink_reload_action_limit_level limit_level) >> +{ >> + int i; >> + >> + for (i = 0 ; i < ARRAY_SIZE(devlink_reload_invalid_combinations) ; i++) > Whitespace. Did you checkpatch? Yes, checked it again now, it still pass. I think checkpatch doesn't see double space. But anyway, I missed it, I will fix. >> + if (devlink_reload_invalid_combinations[i].action == action && >> + devlink_reload_invalid_combinations[i].limit_level == limit_level) >> + return true; >> + return false; >> +} >> + >> static bool >> devlink_reload_action_is_supported(struct devlink *devlink, enum devlink_reload_action action) >> { >> return test_bit(action, &devlink->ops->supported_reload_actions); >> } >> >> +static bool >> +devlink_reload_action_limit_level_is_supported(struct devlink *devlink, >> + enum devlink_reload_action_limit_level limit_level) >> +{ >> + return test_bit(limit_level, &devlink->ops->supported_reload_action_limit_levels); >> +} > This single-use helper just grows LoC and muddies the code IMHO. It is used again in patch 4. > >> static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink, >> enum devlink_command cmd, u32 portid, >> u32 seq, int flags) >> @@ -2975,22 +3008,23 @@ bool devlink_is_reload_failed(const struct devlink *devlink) >> EXPORT_SYMBOL_GPL(devlink_is_reload_failed); >> >> static int devlink_reload(struct devlink *devlink, struct net *dest_net, >> - enum devlink_reload_action action, struct netlink_ext_ack *extack, >> - unsigned long *actions_performed) >> + enum devlink_reload_action action, >> + enum devlink_reload_action_limit_level limit_level, >> + struct netlink_ext_ack *extack, unsigned long *actions_performed) >> { >> int err; >> >> if (!devlink->reload_enabled) >> return -EOPNOTSUPP; >> >> - err = devlink->ops->reload_down(devlink, !!dest_net, action, extack); >> + err = devlink->ops->reload_down(devlink, !!dest_net, action, limit_level, extack); >> if (err) >> return err; >> >> if (dest_net && !net_eq(dest_net, devlink_net(devlink))) >> devlink_reload_netns_change(devlink, dest_net); >> >> - err = devlink->ops->reload_up(devlink, action, extack, actions_performed); >> + err = devlink->ops->reload_up(devlink, action, limit_level, extack, actions_performed); >> devlink_reload_failed_set(devlink, !!err); >> if (err) >> return err; >> @@ -3040,6 +3074,7 @@ devlink_nl_reload_actions_performed_fill(struct sk_buff *msg, >> >> static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) >> { >> + enum devlink_reload_action_limit_level limit_level; >> struct devlink *devlink = info->user_ptr[0]; >> enum devlink_reload_action action; >> unsigned long actions_performed; >> @@ -3077,7 +3112,21 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) >> return -EOPNOTSUPP; >> } >> >> - err = devlink_reload(devlink, dest_net, action, info->extack, &actions_performed); >> + if (info->attrs[DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL]) >> + limit_level = nla_get_u8(info->attrs[DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL]); >> + else >> + limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE; >> + >> + if (!devlink_reload_action_limit_level_is_supported(devlink, limit_level)) { >> + NL_SET_ERR_MSG_MOD(info->extack, "Requested limit level is not supported by the driver"); >> + return -EOPNOTSUPP; >> + } >> + if (devlink_reload_combination_is_invalid(action, limit_level)) { >> + NL_SET_ERR_MSG_MOD(info->extack, "Requested limit level is invalid for this action"); >> + return -EINVAL; >> + } >> + err = devlink_reload(devlink, dest_net, action, limit_level, info->extack, >> + &actions_performed); >> >> if (dest_net) >> put_net(dest_net); >> @@ -7154,6 +7203,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { >> [DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 }, >> [DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED }, >> [DEVLINK_ATTR_RELOAD_ACTION] = { .type = NLA_U8 }, >> + [DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL] = { .type = NLA_U8 }, > range > >> }; >> >> static const struct genl_ops devlink_nl_ops[] = { >> @@ -7489,6 +7539,9 @@ static struct genl_family devlink_nl_family __ro_after_init = { >> >> static bool devlink_reload_actions_valid(const struct devlink_ops *ops) >> { >> + const struct devlink_reload_combination *comb; >> + int i; >> + >> if (!devlink_reload_supported(ops)) { >> if (WARN_ON(ops->supported_reload_actions)) >> return false; >> @@ -7498,6 +7551,18 @@ static bool devlink_reload_actions_valid(const struct devlink_ops *ops) >> if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || >> ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) >> return false; >> + >> + if (WARN_ON(!ops->supported_reload_action_limit_levels || >> + ops->supported_reload_action_limit_levels >= >> + BIT(__DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX))) >> + return false; >> + >> + for (i = 0; i < ARRAY_SIZE(devlink_reload_invalid_combinations); i++) { > White space, again. Same double space, I will fix. > >> + comb = &devlink_reload_invalid_combinations[i]; >> + if (ops->supported_reload_actions == BIT(comb->action) && >> + ops->supported_reload_action_limit_levels == BIT(comb->limit_level)) >> + return false; >> + } >> return true; >> } >> >> @@ -9793,6 +9858,7 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) >> continue; >> err = devlink_reload(devlink, &init_net, >> DEVLINK_RELOAD_ACTION_DRIVER_REINIT, >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE, >> NULL, &actions_performed); >> if (err && err != -EOPNOTSUPP) >> pr_warn("Failed to reload devlink instance into init_net\n");
On Thu, 24 Sep 2020 22:29:55 +0300 Moshe Shemesh wrote: > >> @@ -3964,6 +3965,7 @@ static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, > >> } > >> > >> static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_action action, > >> + enum devlink_reload_action_limit_level limit_level, > >> struct netlink_ext_ack *extack, unsigned long *actions_performed) > >> { > >> struct mlx4_priv *priv = devlink_priv(devlink); > >> @@ -3985,6 +3987,7 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_a > >> static const struct devlink_ops mlx4_devlink_ops = { > >> .port_type_set = mlx4_devlink_port_type_set, > >> .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT), > >> + .supported_reload_action_limit_levels = BIT(DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE), > > Please cut down the name lenghts, this is just lazy. > > > > 'supported_reload_limits' or 'cap_reload_limits' is perfectly > > sufficient. > > > > 'reload_limits' would be even better. Cause what else would it be if > > not a capability. > > Sounds good. > > So instead of supported_reload_actions_limit_levels will have reload_limits. > > Instead of supported_reload_actions will have reload_actions, OK ? Sounds good. > May also use reload_limit_level instead of reload_action_limit_level > everywhere if its clear enough. I think reload_limits is clear. I'd also cut down the length of the defines / enum names. > > Besides I don't think drivers should have to fill negative attributes > > (that they don't support something). Everyone is always going to > > support NONE, since it's "unspecified" / "pick your favorite", right? > > Good point, will remove it. > > >> .reload_down = mlx4_devlink_reload_down, > >> .reload_up = mlx4_devlink_reload_up, > >> }; > >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > >> index fdba7ab58a79..0c5d942dcbd5 100644 > >> --- a/include/uapi/linux/devlink.h > >> +++ b/include/uapi/linux/devlink.h > >> @@ -289,6 +289,22 @@ enum devlink_reload_action { > >> DEVLINK_RELOAD_ACTION_MAX = __DEVLINK_RELOAD_ACTION_MAX - 1 > >> }; > >> > >> +/** > >> + * enum devlink_reload_action_limit_level - Reload action limit level. > >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE: No constrains on action. Action may include > >> + * reset or downtime as needed. > >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET: No reset allowed, no down time allowed, > >> + * no link flap and no configuration is lost. > >> + */ > >> +enum devlink_reload_action_limit_level { > > You reserved UNSPEC for actions but not for limit level? > > > Yes, I used LIMIT_LEVEL_NONE = 0 as no limit needed, so I skipped UNSPEC. > > Maybe should add UNSPEC and use UNSPEC as no limit needed. But UNSPEC is > kind of invalid. Yeah, if we have UNSPEC then it should be invalid. I'm mostly asking for consistency, either have UNSPEC for both actions and limits or for neither. > >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE, > >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET, > >> + > >> + /* Add new reload actions limit level above */ > >> + __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX, > >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX = __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX - 1 > >> +}; > >> + > >> enum devlink_attr { > >> /* don't change the order or add anything between, this is ABI! */ > >> DEVLINK_ATTR_UNSPEC, > >> @@ -480,6 +496,7 @@ enum devlink_attr { > >> > >> DEVLINK_ATTR_RELOAD_ACTION, /* u8 */ > >> DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED, /* nested */ > >> + DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, /* u8 */ > >> > >> /* add new attributes above here, update the policy in devlink.c */ > >> > >> diff --git a/net/core/devlink.c b/net/core/devlink.c > >> index 318ef29f81f2..fee6fcc7dead 100644 > >> --- a/net/core/devlink.c > >> +++ b/net/core/devlink.c > >> @@ -462,12 +462,45 @@ static int devlink_nl_put_handle(struct sk_buff *msg, struct devlink *devlink) > >> return 0; > >> } > >> > >> +struct devlink_reload_combination { > >> + enum devlink_reload_action action; > >> + enum devlink_reload_action_limit_level limit_level; > >> +}; > >> + > >> +static const struct devlink_reload_combination devlink_reload_invalid_combinations[] = { > >> + { > >> + /* can't reinitialize driver with no down time */ > >> + .action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT, > >> + .limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET, > >> + }, > >> +}; > >> + > >> +static bool > >> +devlink_reload_combination_is_invalid(enum devlink_reload_action action, > >> + enum devlink_reload_action_limit_level limit_level) > >> +{ > >> + int i; > >> + > >> + for (i = 0 ; i < ARRAY_SIZE(devlink_reload_invalid_combinations) ; i++) > > Whitespace. Did you checkpatch? > > > Yes, checked it again now, it still pass. I think checkpatch doesn't see > double space. And the spaces before semicolons? It's sad if checkpatch misses such basic stuff :( > But anyway, I missed it, I will fix.
Introduce new options on devlink reload API to enable the user to select the reload action required and contrains limits on these actions that he may want to ensure. Complete support for reload actions in mlx5. The following reload actions are supported: driver_reinit: driver entities re-initialization, applying devlink-param and devlink-resource values. fw_activate: firmware activate. The uAPI is backward compatible, if the reload action option is omitted from the reload command, the driver reinit action will be used. Note that when required to do firmware activation some drivers may need to reload the driver. On the other hand some drivers may need to reset the firmware to reinitialize the driver entities. Therefore, the devlink reload command returns the actions which were actually performed. By default reload actions are not limited and driver implementation may include reset or downtime as needed to perform the actions. However, if limit_level is selected, the driver should perform only if it can do it while keeping the limit level constrains. Reload action limit level added: no_reset: No reset allowed, no down time allowed, no link flap and no configuration is lost. Each driver which supports devlink reload command should expose the reload actions and limit levels supported. Add reload action stats to hold the history per reload action per limit level. For example, the number of times fw_activate has been done on this device since the driver module was added or if the firmware activation was done with or without reset. Patch 1-2 add the new API reload action and reload action limit level option to devlink reload. Patch 3 adds reload actions stats. Patch 4 exposes the reload actions stats on devlink dev get. Patches 5-10 add support on mlx5 for devlink reload action fw_activate and handle the firmware reset events. Patches 11-12 add devlink enable remote dev reset parameter and use it in mlx5. Patches 13-14 mlx5 add devlink reload action limit level no_reset support for fw_activate reload action. Patch 15 adds documentation file devlink-reload.rst Moshe Shemesh (15): devlink: Add reload action option to devlink reload command devlink: Add reload action limit level devlink: Add reload action stats devlink: Add reload actions stats to dev get net/mlx5: Add functions to set/query MFRL register net/mlx5: Set cap for pci sync for fw update event net/mlx5: Handle sync reset request event net/mlx5: Handle sync reset now event net/mlx5: Handle sync reset abort event net/mlx5: Add support for devlink reload action fw activate devlink: Add enable_remote_dev_reset generic parameter net/mlx5: Add devlink param enable_remote_dev_reset support net/mlx5: Add support for fw live patch event net/mlx5: Add support for devlink reload action limit level no reset devlink: Add Documentation/networking/devlink/devlink-reload.rst .../networking/devlink/devlink-params.rst | 6 + .../networking/devlink/devlink-reload.rst | 79 +++ Documentation/networking/devlink/index.rst | 1 + drivers/net/ethernet/mellanox/mlx4/main.c | 16 +- .../net/ethernet/mellanox/mlx5/core/Makefile | 2 +- .../net/ethernet/mellanox/mlx5/core/devlink.c | 122 ++++- .../mellanox/mlx5/core/diag/fw_tracer.c | 31 ++ .../mellanox/mlx5/core/diag/fw_tracer.h | 1 + .../ethernet/mellanox/mlx5/core/fw_reset.c | 454 ++++++++++++++++++ .../ethernet/mellanox/mlx5/core/fw_reset.h | 19 + .../net/ethernet/mellanox/mlx5/core/health.c | 35 +- .../net/ethernet/mellanox/mlx5/core/main.c | 13 + .../ethernet/mellanox/mlx5/core/mlx5_core.h | 2 + drivers/net/ethernet/mellanox/mlxsw/core.c | 27 +- drivers/net/netdevsim/dev.c | 17 +- include/linux/mlx5/device.h | 1 + include/linux/mlx5/driver.h | 4 + include/net/devlink.h | 21 +- include/uapi/linux/devlink.h | 41 ++ net/core/devlink.c | 339 ++++++++++++- 20 files changed, 1179 insertions(+), 52 deletions(-) create mode 100644 Documentation/networking/devlink/devlink-reload.rst create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/fw_reset.h