Message-ID: <20200924133824.206b6308@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 24 Sep 2020 13:38:24 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Moshe Shemesh <moshe@...dia.com>
Cc:     Moshe Shemesh <moshe@...lanox.com>,
        "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 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@...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.  
> 
> 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.

Powered by blists - more mailing lists