Messages in this thread | | | Subject | Re: [PATCH net-next RFC v5 01/15] devlink: Add reload action option to devlink reload command | From | Moshe Shemesh <> | Date | Thu, 24 Sep 2020 22:01:42 +0300 |
| |
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; >> +}
| |