Date:   Wed, 23 Sep 2020 11:25:43 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Moshe Shemesh <moshe@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next RFC v5 01/15] devlink: Add reload action option
 to devlink reload command

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@...lanox.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;
> +}

Powered by blists - more mailing lists