| Message ID | 1600445211-31078-2-git-send-email-moshe@mellanox.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add devlink reload action and limit level options | expand |
On Fri, 18 Sep 2020 19:06:37 +0300 Moshe Shemesh wrote: > Add devlink reload action to allow the user to request a specific reload > action. The action parameter is optional, if not specified then devlink > driver re-init action is used (backward compatible). > 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. > Reload actions supported are: > driver_reinit: driver entities re-initialization, applying devlink-param > and devlink-resource values. > fw_activate: firmware activate. > > command examples: > $devlink dev reload pci/0000:82:00.0 action driver_reinit > reload_actions_performed: > driver_reinit > > $devlink dev reload pci/0000:82:00.0 action fw_activate > reload_actions_performed: > driver_reinit fw_activate > > Signed-off-by: Moshe Shemesh <moshe@mellanox.com> > @@ -3971,15 +3972,19 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, > int err; > > err = mlx4_restart_one_up(persist->pdev, true, devlink); > - if (err) > + if (err) { > mlx4_err(persist->dev, "mlx4_restart_one_up failed, ret=%d\n", > err); > + return err; > + } > + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); FWIW I think drivers should be able to assign this even if they return an error. On error there is no certainty what actions were actually performed (e.g. when timeout happened but the device did the reset a little later) so this argument should not be interpreted in presence of errors, anyway. Also consider providing a second enum for the BIT(xyz)s. > -static bool devlink_reload_supported(const struct devlink *devlink) > +static bool devlink_reload_supported(const struct devlink_ops *ops) > { > - return devlink->ops->reload_down && devlink->ops->reload_up; > + return ops->reload_down && ops->reload_up; > } Please make the change to devlink_reload_supported() a separate patch. > - > + What is this white space funk? 🤔 > static void devlink_reload_failed_set(struct devlink *devlink, > bool reload_failed) > { > @@ -2969,32 +2975,79 @@ 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, > - struct netlink_ext_ack *extack) > + enum devlink_reload_action action, 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, extack); > + err = devlink->ops->reload_down(devlink, !!dest_net, action, 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, extack); > + err = devlink->ops->reload_up(devlink, action, extack, actions_performed); > devlink_reload_failed_set(devlink, !!err); > - return err; > + if (err) > + return err; > + > + WARN_ON(!test_bit(action, actions_performed)); > + return 0; > +} > + > +static int > +devlink_nl_reload_actions_performed_fill(struct sk_buff *msg, > + struct devlink *devlink, > + unsigned long actions_performed, > + enum devlink_command cmd, u32 portid, > + u32 seq, int flags) > +{ > + struct nlattr *actions_performed_attr; > + void *hdr; > + int i; > + > + hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd); > + if (!hdr) > + return -EMSGSIZE; > + > + if (devlink_nl_put_handle(msg, devlink)) > + goto genlmsg_cancel; > + > + actions_performed_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED); > + if (!actions_performed_attr) > + goto genlmsg_cancel; > + > + for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) { > + if (!test_bit(i, &actions_performed)) > + continue; > + if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, i)) > + goto actions_performed_nest_cancel; Why not just return a mask? You need a special attribute for the nest, anyway.. User space would probably actually prefer to have a single attr than an iteration over a nest... > + } > + nla_nest_end(msg, actions_performed_attr); > + genlmsg_end(msg, hdr); > + return 0; > + > +actions_performed_nest_cancel: > + nla_nest_cancel(msg, actions_performed_attr); > +genlmsg_cancel: > + genlmsg_cancel(msg, hdr); > + return -EMSGSIZE; > } > > static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) > { > struct devlink *devlink = info->user_ptr[0]; > + enum devlink_reload_action action; > + unsigned long actions_performed; > struct net *dest_net = NULL; > + struct sk_buff *msg; > int err; > > - if (!devlink_reload_supported(devlink)) > + if (!devlink_reload_supported(devlink->ops)) > return -EOPNOTSUPP; > > err = devlink_resources_validate(devlink, NULL, info); > @@ -3011,12 +3064,43 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) > return PTR_ERR(dest_net); > } > > - err = devlink_reload(devlink, dest_net, info->extack); > + if (info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) > + action = nla_get_u8(info->attrs[DEVLINK_ATTR_RELOAD_ACTION]); > + else > + action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT; > + > + if (action == DEVLINK_RELOAD_ACTION_UNSPEC) { > + NL_SET_ERR_MSG_MOD(info->extack, "Invalid reload action"); > + return -EINVAL; > + } else if (!devlink_reload_action_is_supported(devlink, action)) { > + NL_SET_ERR_MSG_MOD(info->extack, "Requested reload action is not supported by the driver"); > + return -EOPNOTSUPP; > + } > + > + err = devlink_reload(devlink, dest_net, action, info->extack, &actions_performed); Perhaps we can pass the requested action to the driver via actions_performed already, and then all the drivers which only do what they're asked to don't have to touch it? > if (dest_net) > put_net(dest_net); > > - return err; > + if (err) > + return err; > + /* For backward compatibility generate reply only if attributes used by user */ > + if (!info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) > + return 0; > + > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!msg) > + return -ENOMEM; > + > + err = devlink_nl_reload_actions_performed_fill(msg, devlink, actions_performed, > + DEVLINK_CMD_RELOAD, info->snd_portid, > + info->snd_seq, 0); > + if (err) { > + nlmsg_free(msg); > + return err; > + } > + > + return genlmsg_reply(msg, info); Are you using devlink_nl_reload_actions_performed_fill() somewhere else? I'd move the nlmsg_new() / genlmsg_reply() into the helper. > } > > static int devlink_nl_flash_update_fill(struct sk_buff *msg, > @@ -7069,6 +7153,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { > [DEVLINK_ATTR_TRAP_POLICER_RATE] = { .type = NLA_U64 }, > [DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 }, > [DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED }, > + [DEVLINK_ATTR_RELOAD_ACTION] = { .type = NLA_U8 }, Why not just range validation here? > }; > > static const struct genl_ops devlink_nl_ops[] = { > @@ -7402,6 +7487,20 @@ static struct genl_family devlink_nl_family __ro_after_init = { > .n_mcgrps = ARRAY_SIZE(devlink_nl_mcgrps), > }; > > +static bool devlink_reload_actions_valid(const struct devlink_ops *ops) > +{ > + if (!devlink_reload_supported(ops)) { > + if (WARN_ON(ops->supported_reload_actions)) > + return false; > + return true; > + } > + > + if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || > + ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) This won't protect you from ACTION_UNSPEC being set.. WARN_ON(ops->supported_reload_actions & ~GENMASK(...)) > + return false; > + return true; > +}
On 9/23/2020 9:25 PM, Jakub Kicinski wrote: > > On Fri, 18 Sep 2020 19:06:37 +0300 Moshe Shemesh wrote: >> Add devlink reload action to allow the user to request a specific reload >> action. The action parameter is optional, if not specified then devlink >> driver re-init action is used (backward compatible). >> 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. >> Reload actions supported are: >> driver_reinit: driver entities re-initialization, applying devlink-param >> and devlink-resource values. >> fw_activate: firmware activate. >> >> command examples: >> $devlink dev reload pci/0000:82:00.0 action driver_reinit >> reload_actions_performed: >> driver_reinit >> >> $devlink dev reload pci/0000:82:00.0 action fw_activate >> reload_actions_performed: >> driver_reinit fw_activate >> >> Signed-off-by: Moshe Shemesh <moshe@mellanox.com> >> @@ -3971,15 +3972,19 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, >> int err; >> >> err = mlx4_restart_one_up(persist->pdev, true, devlink); >> - if (err) >> + if (err) { >> mlx4_err(persist->dev, "mlx4_restart_one_up failed, ret=%d\n", >> err); >> + return err; >> + } >> + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); > FWIW I think drivers should be able to assign this even if they return > an error. On error there is no certainty what actions were actually > performed (e.g. when timeout happened but the device did the reset a > little later) so this argument should not be interpreted in presence of > errors, anyway. Not sure I got it. Do you mean driver can assign it anyway and devlink should ignore in case of failure ? As I implemented here devlink already ignores actions_performed in case driver returns with error. > Also consider providing a second enum for the BIT(xyz)s. OK. >> -static bool devlink_reload_supported(const struct devlink *devlink) >> +static bool devlink_reload_supported(const struct devlink_ops *ops) >> { >> - return devlink->ops->reload_down && devlink->ops->reload_up; >> + return ops->reload_down && ops->reload_up; >> } > Please make the change to devlink_reload_supported() a separate patch. Ack. >> - >> + > What is this white space funk? 🤔 Missed that. >> static void devlink_reload_failed_set(struct devlink *devlink, >> bool reload_failed) >> { >> @@ -2969,32 +2975,79 @@ 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, >> - struct netlink_ext_ack *extack) >> + enum devlink_reload_action action, 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, extack); >> + err = devlink->ops->reload_down(devlink, !!dest_net, action, 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, extack); >> + err = devlink->ops->reload_up(devlink, action, extack, actions_performed); >> devlink_reload_failed_set(devlink, !!err); >> - return err; >> + if (err) >> + return err; >> + >> + WARN_ON(!test_bit(action, actions_performed)); >> + return 0; >> +} >> + >> +static int >> +devlink_nl_reload_actions_performed_fill(struct sk_buff *msg, >> + struct devlink *devlink, >> + unsigned long actions_performed, >> + enum devlink_command cmd, u32 portid, >> + u32 seq, int flags) >> +{ >> + struct nlattr *actions_performed_attr; >> + void *hdr; >> + int i; >> + >> + hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd); >> + if (!hdr) >> + return -EMSGSIZE; >> + >> + if (devlink_nl_put_handle(msg, devlink)) >> + goto genlmsg_cancel; >> + >> + actions_performed_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED); >> + if (!actions_performed_attr) >> + goto genlmsg_cancel; >> + >> + for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) { >> + if (!test_bit(i, &actions_performed)) >> + continue; >> + if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, i)) >> + goto actions_performed_nest_cancel; > Why not just return a mask? You need a special attribute for the nest, > anyway.. > > User space would probably actually prefer to have a single attr than an > iteration over a nest... OK. >> + } >> + nla_nest_end(msg, actions_performed_attr); >> + genlmsg_end(msg, hdr); >> + return 0; >> + >> +actions_performed_nest_cancel: >> + nla_nest_cancel(msg, actions_performed_attr); >> +genlmsg_cancel: >> + genlmsg_cancel(msg, hdr); >> + return -EMSGSIZE; >> } >> >> static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) >> { >> struct devlink *devlink = info->user_ptr[0]; >> + enum devlink_reload_action action; >> + unsigned long actions_performed; >> struct net *dest_net = NULL; >> + struct sk_buff *msg; >> int err; >> >> - if (!devlink_reload_supported(devlink)) >> + if (!devlink_reload_supported(devlink->ops)) >> return -EOPNOTSUPP; >> >> err = devlink_resources_validate(devlink, NULL, info); >> @@ -3011,12 +3064,43 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) >> return PTR_ERR(dest_net); >> } >> >> - err = devlink_reload(devlink, dest_net, info->extack); >> + if (info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) >> + action = nla_get_u8(info->attrs[DEVLINK_ATTR_RELOAD_ACTION]); >> + else >> + action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT; >> + >> + if (action == DEVLINK_RELOAD_ACTION_UNSPEC) { >> + NL_SET_ERR_MSG_MOD(info->extack, "Invalid reload action"); >> + return -EINVAL; >> + } else if (!devlink_reload_action_is_supported(devlink, action)) { >> + NL_SET_ERR_MSG_MOD(info->extack, "Requested reload action is not supported by the driver"); >> + return -EOPNOTSUPP; >> + } >> + >> + err = devlink_reload(devlink, dest_net, action, info->extack, &actions_performed); > Perhaps we can pass the requested action to the driver via > actions_performed already, and then all the drivers which > only do what they're asked to don't have to touch it? Not sure about it. Note that in the next patch I add here limit_level and that has only input param, so I think it would be confusing. >> if (dest_net) >> put_net(dest_net); >> >> - return err; >> + if (err) >> + return err; >> + /* For backward compatibility generate reply only if attributes used by user */ >> + if (!info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) >> + return 0; >> + >> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); >> + if (!msg) >> + return -ENOMEM; >> + >> + err = devlink_nl_reload_actions_performed_fill(msg, devlink, actions_performed, >> + DEVLINK_CMD_RELOAD, info->snd_portid, >> + info->snd_seq, 0); >> + if (err) { >> + nlmsg_free(msg); >> + return err; >> + } >> + >> + return genlmsg_reply(msg, info); > Are you using devlink_nl_reload_actions_performed_fill() somewhere else? No > I'd move the nlmsg_new() / genlmsg_reply() into the helper. Can do it, but there are many _fill() functions in devlink.c code to fill the data, none of them include nlmsg_new() and genlmsg_reply() that's always in the calling function, even if the calling function adds only that. So I guess I will leave it for consistency. >> } >> >> static int devlink_nl_flash_update_fill(struct sk_buff *msg, >> @@ -7069,6 +7153,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { >> [DEVLINK_ATTR_TRAP_POLICER_RATE] = { .type = NLA_U64 }, >> [DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 }, >> [DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED }, >> + [DEVLINK_ATTR_RELOAD_ACTION] = { .type = NLA_U8 }, > Why not just range validation here? All devlink attributes that pass here go through devlink_nl_poicy this way, including other enums. I think changing that should be in a different patch for all, not in this patchset. >> }; >> >> static const struct genl_ops devlink_nl_ops[] = { >> @@ -7402,6 +7487,20 @@ static struct genl_family devlink_nl_family __ro_after_init = { >> .n_mcgrps = ARRAY_SIZE(devlink_nl_mcgrps), >> }; >> >> +static bool devlink_reload_actions_valid(const struct devlink_ops *ops) >> +{ >> + if (!devlink_reload_supported(ops)) { >> + if (WARN_ON(ops->supported_reload_actions)) >> + return false; >> + return true; >> + } >> + >> + if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || >> + ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) > This won't protect you from ACTION_UNSPEC being set.. > > WARN_ON(ops->supported_reload_actions & ~GENMASK(...)) Right, I will fix. >> + return false; >> + return true; >> +}
On Thu, 24 Sep 2020 22:01:42 +0300 Moshe Shemesh wrote: > On 9/23/2020 9:25 PM, Jakub Kicinski wrote: > >> Signed-off-by: Moshe Shemesh <moshe@mellanox.com> > >> @@ -3971,15 +3972,19 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, > >> int err; > >> > >> err = mlx4_restart_one_up(persist->pdev, true, devlink); > >> - if (err) > >> + if (err) { > >> mlx4_err(persist->dev, "mlx4_restart_one_up failed, ret=%d\n", > >> err); > >> + return err; > >> + } > >> + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); > > FWIW I think drivers should be able to assign this even if they return > > an error. On error there is no certainty what actions were actually > > performed (e.g. when timeout happened but the device did the reset a > > little later) so this argument should not be interpreted in presence of > > errors, anyway. > > Not sure I got it. Do you mean driver can assign it anyway and devlink > should ignore in case of failure ? Yup. > As I implemented here devlink already ignores actions_performed in case > driver returns with error. Right, but you're changing all bunch of drivers like this: static void reload() { - return do_it(); + int err; + + err = do_it(); + if (err) + return err; + + *actions_performed = SOMETHING; + return 0; } When you can instead: static void reload() { + *actions_performed = SOMETHING; return do_it(); } > >> @@ -3011,12 +3064,43 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) > >> return PTR_ERR(dest_net); > >> } > >> > >> - err = devlink_reload(devlink, dest_net, info->extack); > >> + if (info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) > >> + action = nla_get_u8(info->attrs[DEVLINK_ATTR_RELOAD_ACTION]); > >> + else > >> + action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT; > >> + > >> + if (action == DEVLINK_RELOAD_ACTION_UNSPEC) { > >> + NL_SET_ERR_MSG_MOD(info->extack, "Invalid reload action"); > >> + return -EINVAL; > >> + } else if (!devlink_reload_action_is_supported(devlink, action)) { > >> + NL_SET_ERR_MSG_MOD(info->extack, "Requested reload action is not supported by the driver"); > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + err = devlink_reload(devlink, dest_net, action, info->extack, &actions_performed); > > Perhaps we can pass the requested action to the driver via > > actions_performed already, and then all the drivers which > > only do what they're asked to don't have to touch it? > > Not sure about it. Note that in the next patch I add here limit_level > and that has only input param, so I think it would be confusing. I don't think it'd be, but don't feel strongly either. > >> if (dest_net) > >> put_net(dest_net); > >> > >> - return err; > >> + if (err) > >> + return err; > >> + /* For backward compatibility generate reply only if attributes used by user */ > >> + if (!info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) > >> + return 0; > >> + > >> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > >> + if (!msg) > >> + return -ENOMEM; > >> + > >> + err = devlink_nl_reload_actions_performed_fill(msg, devlink, actions_performed, > >> + DEVLINK_CMD_RELOAD, info->snd_portid, > >> + info->snd_seq, 0); > >> + if (err) { > >> + nlmsg_free(msg); > >> + return err; > >> + } > >> + > >> + return genlmsg_reply(msg, info); > > Are you using devlink_nl_reload_actions_performed_fill() somewhere else? > No > > I'd move the nlmsg_new() / genlmsg_reply() into the helper. > > Can do it, but there are many _fill() functions in devlink.c code to > fill the data, none of them include nlmsg_new() and genlmsg_reply() > that's always in the calling function, even if the calling function adds > only that. So I guess I will leave it for consistency. Don't call the helper _fill() and you'll be good. > >> } > >> > >> static int devlink_nl_flash_update_fill(struct sk_buff *msg, > >> @@ -7069,6 +7153,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { > >> [DEVLINK_ATTR_TRAP_POLICER_RATE] = { .type = NLA_U64 }, > >> [DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 }, > >> [DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED }, > >> + [DEVLINK_ATTR_RELOAD_ACTION] = { .type = NLA_U8 }, > > Why not just range validation here? > > All devlink attributes that pass here go through devlink_nl_poicy this > way, including other enums. > > I think changing that should be in a different patch for all, not in > this patchset. I don't think this is on purpose. Please use range validation in new code from the start. We support dumping policies to user space, it's useful to know the range of parameters from the policy.
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c index 70cf24ba71e4..1a482120cc0a 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c +++ b/drivers/net/ethernet/mellanox/mlx4/main.c @@ -3946,6 +3946,7 @@ static int mlx4_restart_one_up(struct pci_dev *pdev, bool reload, struct devlink *devlink); static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, + enum devlink_reload_action action, struct netlink_ext_ack *extack) { struct mlx4_priv *priv = devlink_priv(devlink); @@ -3962,8 +3963,8 @@ static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change, return 0; } -static int mlx4_devlink_reload_up(struct devlink *devlink, - struct netlink_ext_ack *extack) +static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_action action, + struct netlink_ext_ack *extack, unsigned long *actions_performed) { struct mlx4_priv *priv = devlink_priv(devlink); struct mlx4_dev *dev = &priv->dev; @@ -3971,15 +3972,19 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, int err; err = mlx4_restart_one_up(persist->pdev, true, devlink); - if (err) + if (err) { mlx4_err(persist->dev, "mlx4_restart_one_up failed, ret=%d\n", err); + return err; + } + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); - return err; + return 0; } 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), .reload_down = mlx4_devlink_reload_down, .reload_up = mlx4_devlink_reload_up, }; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c index c709e9a385f6..ffc0525cea64 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c @@ -89,6 +89,7 @@ mlx5_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req, } static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change, + enum devlink_reload_action action, struct netlink_ext_ack *extack) { struct mlx5_core_dev *dev = devlink_priv(devlink); @@ -97,12 +98,18 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change, return 0; } -static int mlx5_devlink_reload_up(struct devlink *devlink, - struct netlink_ext_ack *extack) +static int mlx5_devlink_reload_up(struct devlink *devlink, enum devlink_reload_action action, + struct netlink_ext_ack *extack, unsigned long *actions_performed) { struct mlx5_core_dev *dev = devlink_priv(devlink); + int err; - return mlx5_load_one(dev, false); + err = mlx5_load_one(dev, false); + if (err) + return err; + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); + + return 0; } static const struct devlink_ops mlx5_devlink_ops = { @@ -118,6 +125,7 @@ static const struct devlink_ops mlx5_devlink_ops = { #endif .flash_update = mlx5_devlink_flash_update, .info_get = mlx5_devlink_info_get, + .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT), .reload_down = mlx5_devlink_reload_down, .reload_up = mlx5_devlink_reload_up, }; diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c index 1bb21fe295b9..19f4486d5faf 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/core.c +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c @@ -1409,7 +1409,7 @@ mlxsw_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req, static int mlxsw_devlink_core_bus_device_reload_down(struct devlink *devlink, - bool netns_change, + bool netns_change, enum devlink_reload_action action, struct netlink_ext_ack *extack) { struct mlxsw_core *mlxsw_core = devlink_priv(devlink); @@ -1422,15 +1422,23 @@ mlxsw_devlink_core_bus_device_reload_down(struct devlink *devlink, } static int -mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink, - struct netlink_ext_ack *extack) +mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink, enum devlink_reload_action action, + struct netlink_ext_ack *extack, + unsigned long *actions_performed) { struct mlxsw_core *mlxsw_core = devlink_priv(devlink); + int err; - return mlxsw_core_bus_device_register(mlxsw_core->bus_info, - mlxsw_core->bus, - mlxsw_core->bus_priv, true, - devlink, extack); + err = mlxsw_core_bus_device_register(mlxsw_core->bus_info, + mlxsw_core->bus, + mlxsw_core->bus_priv, true, + devlink, extack); + if (err) + return err; + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) | + BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE); + + return 0; } static int mlxsw_devlink_flash_update(struct devlink *devlink, @@ -1560,6 +1568,8 @@ mlxsw_devlink_trap_policer_counter_get(struct devlink *devlink, } static const struct devlink_ops mlxsw_devlink_ops = { + .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) | + BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE), .reload_down = mlxsw_devlink_core_bus_device_reload_down, .reload_up = mlxsw_devlink_core_bus_device_reload_up, .port_type_set = mlxsw_devlink_port_type_set, diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 32f339fedb21..0eb522f6a718 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -697,7 +697,7 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev, static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev); static int nsim_dev_reload_down(struct devlink *devlink, bool netns_change, - struct netlink_ext_ack *extack) + enum devlink_reload_action action, struct netlink_ext_ack *extack) { struct nsim_dev *nsim_dev = devlink_priv(devlink); @@ -713,10 +713,11 @@ static int nsim_dev_reload_down(struct devlink *devlink, bool netns_change, return 0; } -static int nsim_dev_reload_up(struct devlink *devlink, - struct netlink_ext_ack *extack) +static int nsim_dev_reload_up(struct devlink *devlink, enum devlink_reload_action action, + struct netlink_ext_ack *extack, unsigned long *actions_performed) { struct nsim_dev *nsim_dev = devlink_priv(devlink); + int err; if (nsim_dev->fail_reload) { /* For testing purposes, user set debugfs fail_reload @@ -726,7 +727,12 @@ static int nsim_dev_reload_up(struct devlink *devlink, return -EINVAL; } - return nsim_dev_reload_create(nsim_dev, extack); + err = nsim_dev_reload_create(nsim_dev, extack); + if (err) + return err; + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); + + return 0; } static int nsim_dev_info_get(struct devlink *devlink, @@ -875,6 +881,7 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink *devlink, } static const struct devlink_ops nsim_dev_devlink_ops = { + .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT), .reload_down = nsim_dev_reload_down, .reload_up = nsim_dev_reload_up, .info_get = nsim_dev_info_get, diff --git a/include/net/devlink.h b/include/net/devlink.h index 48b1c1ef1ebd..37abc3e08e9e 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1014,10 +1014,11 @@ enum devlink_trap_group_generic_id { } struct devlink_ops { + unsigned long supported_reload_actions; int (*reload_down)(struct devlink *devlink, bool netns_change, - struct netlink_ext_ack *extack); - int (*reload_up)(struct devlink *devlink, - struct netlink_ext_ack *extack); + enum devlink_reload_action action, struct netlink_ext_ack *extack); + int (*reload_up)(struct devlink *devlink, enum devlink_reload_action action, + struct netlink_ext_ack *extack, unsigned long *actions_performed); int (*port_type_set)(struct devlink_port *devlink_port, enum devlink_port_type port_type); int (*port_split)(struct devlink *devlink, unsigned int port_index, diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index 631f5bdf1707..fdba7ab58a79 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -274,6 +274,21 @@ enum { DEVLINK_ATTR_TRAP_METADATA_TYPE_FA_COOKIE, }; +/** + * enum devlink_reload_action - Reload action. + * @DEVLINK_RELOAD_ACTION_DRIVER_REINIT: Driver entities re-instantiation. + * @DEVLINK_RELOAD_ACTION_FW_ACTIVATE: FW activate. + */ +enum devlink_reload_action { + DEVLINK_RELOAD_ACTION_UNSPEC, + DEVLINK_RELOAD_ACTION_DRIVER_REINIT, + DEVLINK_RELOAD_ACTION_FW_ACTIVATE, + + /* Add new reload actions above */ + __DEVLINK_RELOAD_ACTION_MAX, + DEVLINK_RELOAD_ACTION_MAX = __DEVLINK_RELOAD_ACTION_MAX - 1 +}; + enum devlink_attr { /* don't change the order or add anything between, this is ABI! */ DEVLINK_ATTR_UNSPEC, @@ -462,6 +477,10 @@ enum devlink_attr { DEVLINK_ATTR_PORT_EXTERNAL, /* u8 */ DEVLINK_ATTR_PORT_CONTROLLER_NUMBER, /* u32 */ + + DEVLINK_ATTR_RELOAD_ACTION, /* u8 */ + DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED, /* nested */ + /* add new attributes above here, update the policy in devlink.c */ __DEVLINK_ATTR_MAX, diff --git a/net/core/devlink.c b/net/core/devlink.c index e5b71f3c2d4d..318ef29f81f2 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -462,6 +462,12 @@ static int devlink_nl_put_handle(struct sk_buff *msg, struct devlink *devlink) return 0; } +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 int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink, enum devlink_command cmd, u32 portid, u32 seq, int flags) @@ -2948,11 +2954,11 @@ static void devlink_reload_netns_change(struct devlink *devlink, DEVLINK_CMD_PARAM_NEW); } -static bool devlink_reload_supported(const struct devlink *devlink) +static bool devlink_reload_supported(const struct devlink_ops *ops) { - return devlink->ops->reload_down && devlink->ops->reload_up; + return ops->reload_down && ops->reload_up; } - + static void devlink_reload_failed_set(struct devlink *devlink, bool reload_failed) { @@ -2969,32 +2975,79 @@ 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, - struct netlink_ext_ack *extack) + enum devlink_reload_action action, 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, extack); + err = devlink->ops->reload_down(devlink, !!dest_net, action, 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, extack); + err = devlink->ops->reload_up(devlink, action, extack, actions_performed); devlink_reload_failed_set(devlink, !!err); - return err; + if (err) + return err; + + WARN_ON(!test_bit(action, actions_performed)); + return 0; +} + +static int +devlink_nl_reload_actions_performed_fill(struct sk_buff *msg, + struct devlink *devlink, + unsigned long actions_performed, + enum devlink_command cmd, u32 portid, + u32 seq, int flags) +{ + struct nlattr *actions_performed_attr; + void *hdr; + int i; + + hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd); + if (!hdr) + return -EMSGSIZE; + + if (devlink_nl_put_handle(msg, devlink)) + goto genlmsg_cancel; + + actions_performed_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED); + if (!actions_performed_attr) + goto genlmsg_cancel; + + for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) { + if (!test_bit(i, &actions_performed)) + continue; + if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, i)) + goto actions_performed_nest_cancel; + } + nla_nest_end(msg, actions_performed_attr); + genlmsg_end(msg, hdr); + return 0; + +actions_performed_nest_cancel: + nla_nest_cancel(msg, actions_performed_attr); +genlmsg_cancel: + genlmsg_cancel(msg, hdr); + return -EMSGSIZE; } static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) { struct devlink *devlink = info->user_ptr[0]; + enum devlink_reload_action action; + unsigned long actions_performed; struct net *dest_net = NULL; + struct sk_buff *msg; int err; - if (!devlink_reload_supported(devlink)) + if (!devlink_reload_supported(devlink->ops)) return -EOPNOTSUPP; err = devlink_resources_validate(devlink, NULL, info); @@ -3011,12 +3064,43 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) return PTR_ERR(dest_net); } - err = devlink_reload(devlink, dest_net, info->extack); + if (info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) + action = nla_get_u8(info->attrs[DEVLINK_ATTR_RELOAD_ACTION]); + else + action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT; + + if (action == DEVLINK_RELOAD_ACTION_UNSPEC) { + NL_SET_ERR_MSG_MOD(info->extack, "Invalid reload action"); + return -EINVAL; + } else if (!devlink_reload_action_is_supported(devlink, action)) { + NL_SET_ERR_MSG_MOD(info->extack, "Requested reload action is not supported by the driver"); + return -EOPNOTSUPP; + } + + err = devlink_reload(devlink, dest_net, action, info->extack, &actions_performed); if (dest_net) put_net(dest_net); - return err; + if (err) + return err; + /* For backward compatibility generate reply only if attributes used by user */ + if (!info->attrs[DEVLINK_ATTR_RELOAD_ACTION]) + return 0; + + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) + return -ENOMEM; + + err = devlink_nl_reload_actions_performed_fill(msg, devlink, actions_performed, + DEVLINK_CMD_RELOAD, info->snd_portid, + info->snd_seq, 0); + if (err) { + nlmsg_free(msg); + return err; + } + + return genlmsg_reply(msg, info); } static int devlink_nl_flash_update_fill(struct sk_buff *msg, @@ -7069,6 +7153,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { [DEVLINK_ATTR_TRAP_POLICER_RATE] = { .type = NLA_U64 }, [DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 }, [DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED }, + [DEVLINK_ATTR_RELOAD_ACTION] = { .type = NLA_U8 }, }; static const struct genl_ops devlink_nl_ops[] = { @@ -7402,6 +7487,20 @@ static struct genl_family devlink_nl_family __ro_after_init = { .n_mcgrps = ARRAY_SIZE(devlink_nl_mcgrps), }; +static bool devlink_reload_actions_valid(const struct devlink_ops *ops) +{ + if (!devlink_reload_supported(ops)) { + if (WARN_ON(ops->supported_reload_actions)) + return false; + return true; + } + + if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || + ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) + return false; + return true; +} + /** * devlink_alloc - Allocate new devlink instance resources * @@ -7418,10 +7517,14 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) if (WARN_ON(!ops)) return NULL; + if (!devlink_reload_actions_valid(ops)) + return NULL; + devlink = kzalloc(sizeof(*devlink) + priv_size, GFP_KERNEL); if (!devlink) return NULL; devlink->ops = ops; + xa_init_flags(&devlink->snapshot_ids, XA_FLAGS_ALLOC); __devlink_net_set(devlink, &init_net); INIT_LIST_HEAD(&devlink->port_list); @@ -7466,7 +7569,7 @@ EXPORT_SYMBOL_GPL(devlink_register); void devlink_unregister(struct devlink *devlink) { mutex_lock(&devlink_mutex); - WARN_ON(devlink_reload_supported(devlink) && + WARN_ON(devlink_reload_supported(devlink->ops) && devlink->reload_enabled); devlink_notify(devlink, DEVLINK_CMD_DEL); list_del(&devlink->list); @@ -8503,7 +8606,7 @@ __devlink_param_driverinit_value_set(struct devlink *devlink, int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id, union devlink_param_value *init_val) { - if (!devlink_reload_supported(devlink)) + if (!devlink_reload_supported(devlink->ops)) return -EOPNOTSUPP; return __devlink_param_driverinit_value_get(&devlink->param_list, @@ -8550,7 +8653,7 @@ int devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port, { struct devlink *devlink = devlink_port->devlink; - if (!devlink_reload_supported(devlink)) + if (!devlink_reload_supported(devlink->ops)) return -EOPNOTSUPP; return __devlink_param_driverinit_value_get(&devlink_port->param_list, @@ -9676,6 +9779,7 @@ int devlink_compat_switch_id_get(struct net_device *dev, static void __net_exit devlink_pernet_pre_exit(struct net *net) { + unsigned long actions_performed; struct devlink *devlink; int err; @@ -9685,9 +9789,11 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) mutex_lock(&devlink_mutex); list_for_each_entry(devlink, &devlink_list, list) { if (net_eq(devlink_net(devlink), net)) { - if (WARN_ON(!devlink_reload_supported(devlink))) + if (WARN_ON(!devlink_reload_supported(devlink->ops))) continue; - err = devlink_reload(devlink, &init_net, NULL); + err = devlink_reload(devlink, &init_net, + DEVLINK_RELOAD_ACTION_DRIVER_REINIT, + NULL, &actions_performed); if (err && err != -EOPNOTSUPP) pr_warn("Failed to reload devlink instance into init_net\n"); }
Add devlink reload action to allow the user to request a specific reload action. The action parameter is optional, if not specified then devlink driver re-init action is used (backward compatible). 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. Reload actions supported are: driver_reinit: driver entities re-initialization, applying devlink-param and devlink-resource values. fw_activate: firmware activate. command examples: $devlink dev reload pci/0000:82:00.0 action driver_reinit reload_actions_performed: driver_reinit $devlink dev reload pci/0000:82:00.0 action fw_activate reload_actions_performed: driver_reinit fw_activate Signed-off-by: Moshe Shemesh <moshe@mellanox.com> --- v4 -> v5: - Always pass actions_performed to unload_up() instead of checking in each driver - Verify returned actions_performed includes the requested action - Changed devlink_reload_actions_verify(devlink) to get ops - Changed devlink_reload_actions_verify() to return bool and rename to devlink_reload_actions_valid() - Only generate the reply if request uses new attributes v3 -> v4: - Removed fw_activate_no_reset as an action (next patch adds limit levels instead). - Renamed actions_done to actions_performed v2 -> v3: - Replace fw_live_patch action by fw_activate_no_reset - Devlink reload returns the actions done over netlink reply v1 -> v2: - Instead of reload levels driver,fw_reset,fw_live_patch have reload actions driver_reinit,fw_activate,fw_live_patch - Remove driver default level, the action driver_reinit is the default action for all drivers --- drivers/net/ethernet/mellanox/mlx4/main.c | 13 +- .../net/ethernet/mellanox/mlx5/core/devlink.c | 14 +- drivers/net/ethernet/mellanox/mlxsw/core.c | 24 +++- drivers/net/netdevsim/dev.c | 15 +- include/net/devlink.h | 7 +- include/uapi/linux/devlink.h | 19 +++ net/core/devlink.c | 136 ++++++++++++++++-- 7 files changed, 192 insertions(+), 36 deletions(-)