| Message ID | 1600063682-17313-1-git-send-email-moshe@mellanox.com |
|---|---|
| Headers | show |
| Series | Add devlink reload action and | expand |
On Mon, Sep 14, 2020 at 11:39 AM Moshe Shemesh <moshe@mellanox.com> 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> > --- > 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 | 14 ++- > .../net/ethernet/mellanox/mlx5/core/devlink.c | 15 ++- > drivers/net/ethernet/mellanox/mlxsw/core.c | 25 ++-- > drivers/net/netdevsim/dev.c | 16 ++- > include/net/devlink.h | 7 +- > include/uapi/linux/devlink.h | 19 +++ > net/core/devlink.c | 111 +++++++++++++++++- > 7 files changed, 180 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > index 70cf24ba71e4..aadf1676a0ed 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,20 @@ 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; > + } > + if (actions_performed) > + *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..9cd6b6c884e3 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,19 @@ 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; > + if (actions_performed) > + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); > + > + return 0; > } > > static const struct devlink_ops mlx5_devlink_ops = { > @@ -118,6 +126,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 ec45a03140d7..c0a32f685b85 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/core.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c > @@ -1113,7 +1113,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); > @@ -1126,15 +1126,24 @@ 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) Sorry for repeating again, for fw_activate action on our device, all the driver entities undergo reset asynchronously once user initiates "devlink dev reload action fw_activate" and reload_up does not have much to do except reporting actions that will be/being performed. Once reset is complete, the health reporter will be notified using devlink_health_reporter_recovery_done(). Returning from reload_up does not guarantee successful activation of firmware. Status of reset will be notified to the health reporter via devlink_health_reporter_state_update(). I am just repeating this, so I want to know if I am on the same page. Thanks. > { > 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; > + if (actions_performed) > + *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, > @@ -1268,6 +1277,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), What if drivers do not support DRIVER_REINIT operation directly but when users issue FW_ACTIVATE, driver performs both FW_ACTIVATE and DRIVER_REINIT? That particular driver has to advertise only DRIVER_REINIT action, right? > .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..f0919fa0cd8b 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,13 @@ 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; > + if (actions_performed) > + *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT); > + > + return 0; > } > > static int nsim_dev_info_get(struct devlink *devlink, > @@ -875,6 +882,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 eaec0a8cc5ef..b09db891db04 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -1011,10 +1011,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 40d35145c879..a6f64db0bdf3 100644 > --- a/include/uapi/linux/devlink.h > +++ b/include/uapi/linux/devlink.h > @@ -272,6 +272,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, > @@ -460,6 +475,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 19037f114307..f4be1e1bf864 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) > @@ -2969,29 +2975,72 @@ 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; > } > > +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)) > @@ -3011,12 +3060,41 @@ 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 || action > DEVLINK_RELOAD_ACTION_MAX) { > + 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"); > + 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; > + > + WARN_ON(!actions_performed); > + 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, > @@ -7047,6 +7125,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[] = { > @@ -7372,6 +7451,20 @@ static struct genl_family devlink_nl_family __ro_after_init = { > .n_mcgrps = ARRAY_SIZE(devlink_nl_mcgrps), > }; > > +static int devlink_reload_actions_verify(struct devlink *devlink) > +{ > + const struct devlink_ops *ops; > + > + if (!devlink_reload_supported(devlink)) > + return 0; > + > + ops = devlink->ops; > + if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || > + ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) > + return -EINVAL; > + return 0; > +} > + > /** > * devlink_alloc - Allocate new devlink instance resources > * > @@ -7392,6 +7485,11 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) > if (!devlink) > return NULL; > devlink->ops = ops; > + if (devlink_reload_actions_verify(devlink)) { > + kfree(devlink); > + return NULL; > + } > + > xa_init_flags(&devlink->snapshot_ids, XA_FLAGS_ALLOC); > __devlink_net_set(devlink, &init_net); > INIT_LIST_HEAD(&devlink->port_list); > @@ -9657,7 +9755,8 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) > if (net_eq(devlink_net(devlink), net)) { > if (WARN_ON(!devlink_reload_supported(devlink))) > continue; > - err = devlink_reload(devlink, &init_net, NULL); > + err = devlink_reload(devlink, &init_net, > + DEVLINK_RELOAD_ACTION_DRIVER_REINIT, NULL, NULL); > if (err && err != -EOPNOTSUPP) > pr_warn("Failed to reload devlink instance into init_net\n"); > } > -- > 2.17.1 >
Mon, Sep 14, 2020 at 11:54:55AM CEST, vasundhara-v.volam@broadcom.com wrote: >On Mon, Sep 14, 2020 at 3:02 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Mon, Sep 14, 2020 at 09:08:58AM CEST, vasundhara-v.volam@broadcom.com wrote: >> >On Mon, Sep 14, 2020 at 11:39 AM Moshe Shemesh <moshe@mellanox.com> wrote: >> >> [...] >> >> >> >> @@ -1126,15 +1126,24 @@ 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) >> >Sorry for repeating again, for fw_activate action on our device, all >> >the driver entities undergo reset asynchronously once user initiates >> >"devlink dev reload action fw_activate" and reload_up does not have >> >much to do except reporting actions that will be/being performed. >> > >> >Once reset is complete, the health reporter will be notified using >> >> Hmm, how is the fw reset related to health reporter recovery? Recovery >> happens after some error event. I don't believe it is wise to mix it. >Our device has a fw_reset health reporter, which is updated on reset >events and firmware activation is one among them. All non-fatal >firmware reset events are reported on fw_reset health reporter. Hmm, interesting. In that case, assuming this is fine, should we have some standard in this. I mean, if the driver supports reset, should it also define the "fw_reset" reporter to report such events? Jakub, what is your take here? > >> >> Instead, why don't you block in reload_up() until the reset is complete? > >Though user initiate "devlink dev reload" event on a single interface, >all driver entities undergo reset and all entities recover >independently. I don't think we can block the reload_up() on the >interface(that user initiated the command), until whole reset is >complete. Why not? mlxsw reset takes up to like 10 seconds for example. >> >> >> >devlink_health_reporter_recovery_done(). Returning from reload_up does >> >not guarantee successful activation of firmware. Status of reset will >> >be notified to the health reporter via >> >devlink_health_reporter_state_update(). >> > >> >I am just repeating this, so I want to know if I am on the same page. >> > >> >Thanks. >> >> [...]
Mon, Sep 14, 2020 at 08:08:02AM CEST, moshe@mellanox.com wrote: >Add devlink reload rst documentation file. >Update index file to include it. > >Signed-off-by: Moshe Shemesh <moshe@mellanox.com> >--- >v3 -> v4: >- Remove reload action fw_activate_no_reset >- Add reload actions limit levels and document the no_reset limit level > constrains >v2 -> v3: >- Devlink reload returns the actions done >- Replace fw_live_patch action by fw_activate_no_reset >- Explain fw_activate meaning >v1 -> v2: >- Instead of reload levels driver,fw_reset,fw_live_patch have reload > actions driver_reinit,fw_activate,fw_live_patch >--- > .../networking/devlink/devlink-reload.rst | 80 +++++++++++++++++++ > Documentation/networking/devlink/index.rst | 1 + > 2 files changed, 81 insertions(+) > create mode 100644 Documentation/networking/devlink/devlink-reload.rst > >diff --git a/Documentation/networking/devlink/devlink-reload.rst b/Documentation/networking/devlink/devlink-reload.rst >new file mode 100644 >index 000000000000..6ac9dddd2208 >--- /dev/null >+++ b/Documentation/networking/devlink/devlink-reload.rst >@@ -0,0 +1,80 @@ >+.. SPDX-License-Identifier: GPL-2.0 >+ >+============== >+Devlink Reload >+============== >+ >+``devlink-reload`` provides mechanism to either reload driver entities, >+applying ``devlink-params`` and ``devlink-resources`` new values or firmware >+activation depends on reload action selected. >+ >+Reload actions >+============== >+ >+User may select a reload action. >+By default ``driver_reinit`` action is selected. >+ >+.. list-table:: Possible reload actions >+ :widths: 5 90 >+ >+ * - Name >+ - Description >+ * - ``driver-reinit`` >+ - Devlink driver entities re-initialization, including applying >+ new values to devlink entities which are used during driver >+ load such as ``devlink-params`` in configuration mode >+ ``driverinit`` or ``devlink-resources`` >+ * - ``fw_activate`` >+ - Firmware activate. Activates new firmware if such image is stored and >+ pending activation. This action involves firmware reset, if no new image >+ pending this action will reload current firmware image. >+ >+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 s/reload/reinit" ? >+the firmware to reinitialize the driver entities. Therefore, the devlink >+reload command returns the actions which were actually performed. I would perhaps say something more generic like: Note that even though user asks for a specific action, the driver implementation might require to perform another action alongside with it. For example, some driver do not support driver reinitialization being performed without fw activation. Therefore, the devlink reload command return the list of actions which were actrually performed. >+ >+Reload action limit levels >+========================== >+ >+By default reload actions are not limited and Driver implementation may Why capital "D"? >+include reset or downtime as needed to perform the actions. >+ >+However, some drivers support action limit levels, which limits the action >+implementation to specific constrains. >+ >+.. list-table:: Possible reload action limit levels >+ :widths: 5 90 >+ >+ * - Name >+ - Description >+ * - ``no_reset`` >+ - No reset allowed, no down time allowed, no link flap and no >+ configuration is lost. >+ >+Change namespace >+================ >+ >+All devlink instances are created in init_net and stay there for a >+lifetime. Allow user to be able to move devlink instances into >+namespaces during devlink reload operation. That ensures proper >+re-instantiation of driver objects, including netdevices. This sounds like a commit message :) Could you please re-phrase a bit? >+ >+example usage >+------------- >+ >+.. code:: shell >+ >+ $ devlink dev reload help >+ $ devlink dev reload DEV [ netns { PID | NAME | ID } ] [ action { driver_reinit | fw_activate } ] [limit_level no_reset] >+ >+ # Run reload command for devlink driver entities re-initialization: >+ $ devlink dev reload pci/0000:82:00.0 action driver_reinit >+ reload_actions_performed: >+ driver_reinit >+ >+ # Run reload command to activate firmware: >+ # Note that mlx5 driver reloads the driver while activating firmware >+ $ devlink dev reload pci/0000:82:00.0 action fw_activate >+ reload_actions_performed: >+ driver_reinit fw_activate This looks fine to me. >diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst >index 7684ae5c4a4a..d82874760ae2 100644 >--- a/Documentation/networking/devlink/index.rst >+++ b/Documentation/networking/devlink/index.rst >@@ -20,6 +20,7 @@ general. > devlink-params > devlink-region > devlink-resource >+ devlink-reload > devlink-trap > > Driver-specific documentation >-- >2.17.1 >
On Mon, 14 Sep 2020 13:28:29 +0200 Jiri Pirko wrote: > Mon, Sep 14, 2020 at 11:54:55AM CEST, vasundhara-v.volam@broadcom.com wrote: > >On Mon, Sep 14, 2020 at 3:02 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> +mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink, enum devlink_reload_action action, > >> >> + struct netlink_ext_ack *extack, > >> >> + unsigned long *actions_performed) > >> >Sorry for repeating again, for fw_activate action on our device, all > >> >the driver entities undergo reset asynchronously once user initiates > >> >"devlink dev reload action fw_activate" and reload_up does not have > >> >much to do except reporting actions that will be/being performed. > >> > > >> >Once reset is complete, the health reporter will be notified using > >> > >> Hmm, how is the fw reset related to health reporter recovery? Recovery > >> happens after some error event. I don't believe it is wise to mix it. > >Our device has a fw_reset health reporter, which is updated on reset > >events and firmware activation is one among them. All non-fatal > >firmware reset events are reported on fw_reset health reporter. > > Hmm, interesting. In that case, assuming this is fine, should we have > some standard in this. I mean, if the driver supports reset, should it > also define the "fw_reset" reporter to report such events? > > Jakub, what is your take here? Sounds doubly wrong to me. As you say health reporters should trigger on error events, communicating completion of an action requested by the user seems very wrong. IIUC operators should monitor and collect health failures. In this case looks like all events from fw_reset would need to be discarded, since they are not meaningful without the context of what triggered them. And secondly, reporting the completion via some async mechanism that user has to monitor is just plain lazy. That's pushing out the work that has to be done out to user space. Wait for the completion in the driver. > >> Instead, why don't you block in reload_up() until the reset is complete? > > > >Though user initiate "devlink dev reload" event on a single interface, > >all driver entities undergo reset and all entities recover > >independently. I don't think we can block the reload_up() on the > >interface(that user initiated the command), until whole reset is > >complete. > > Why not? mlxsw reset takes up to like 10 seconds for example. +1, why?
On Mon, 14 Sep 2020 09:07:48 +0300 Moshe Shemesh wrote: > @@ -3011,12 +3060,41 @@ 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 || action > DEVLINK_RELOAD_ACTION_MAX) { > + 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"); > + 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; > + > + WARN_ON(!actions_performed); > + 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); I think generating the reply may break existing users. Only generate the reply if request contained DEVLINK_ATTR_RELOAD_ACTION (or any other new attribute which existing users can't pass).
On Mon, Sep 14, 2020 at 2:31 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 14 Sep 2020 13:28:29 +0200 Jiri Pirko wrote: > > >> Instead, why don't you block in reload_up() until the reset is complete? > > > > > >Though user initiate "devlink dev reload" event on a single interface, > > >all driver entities undergo reset and all entities recover > > >independently. I don't think we can block the reload_up() on the > > >interface(that user initiated the command), until whole reset is > > >complete. > > > > Why not? mlxsw reset takes up to like 10 seconds for example. > > +1, why? Yes, we should be able to block until the reset sequence is complete. I don't see any problem. I will work with Vasundhara on this.
Tue, Sep 15, 2020 at 12:06:19AM CEST, michael.chan@broadcom.com wrote: >On Mon, Sep 14, 2020 at 2:31 PM Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Mon, 14 Sep 2020 13:28:29 +0200 Jiri Pirko wrote: >> > >> Instead, why don't you block in reload_up() until the reset is complete? >> > > >> > >Though user initiate "devlink dev reload" event on a single interface, >> > >all driver entities undergo reset and all entities recover >> > >independently. I don't think we can block the reload_up() on the >> > >interface(that user initiated the command), until whole reset is >> > >complete. >> > >> > Why not? mlxsw reset takes up to like 10 seconds for example. >> >> +1, why? > >Yes, we should be able to block until the reset sequence is complete. >I don't see any problem. I will work with Vasundhara on this. Could you please also remove fw_reset as it is apparently misuse of devlink health mechanism? Thanks!
On 9/14/2020 3:27 PM, Jiri Pirko wrote: > Mon, Sep 14, 2020 at 08:07:48AM CEST, moshe@mellanox.com 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> >> --- >> 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 | 14 ++- >> .../net/ethernet/mellanox/mlx5/core/devlink.c | 15 ++- >> drivers/net/ethernet/mellanox/mlxsw/core.c | 25 ++-- >> drivers/net/netdevsim/dev.c | 16 ++- >> include/net/devlink.h | 7 +- >> include/uapi/linux/devlink.h | 19 +++ >> net/core/devlink.c | 111 +++++++++++++++++- >> 7 files changed, 180 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c >> index 70cf24ba71e4..aadf1676a0ed 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,20 @@ 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; >> + } >> + if (actions_performed) > Nit, pass the unsigned long allways (even when it would be unused) and > avoid check in every driver. > Ack. >> + *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, >> }; > [..] > > >> @@ -2969,29 +2975,72 @@ 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); > Here, please add a WARN_ON() check: > WARN_ON(!(*actions_performed & action)); > > The requested action should be always performed. > Good point, I will add. >> devlink_reload_failed_set(devlink, !!err); >> return err; >> } >> >> +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)) >> @@ -3011,12 +3060,41 @@ 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 || action > DEVLINK_RELOAD_ACTION_MAX) { >> + NL_SET_ERR_MSG_MOD(info->extack, "Invalid reload action"); > Hmm, I understand the unspec check, but the max check is not needed. The > following check will take care of it. > Ack. >> + 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" ? > Yes, will fix. >> + 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; >> + >> + WARN_ON(!actions_performed); >> + 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, >> @@ -7047,6 +7125,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[] = { >> @@ -7372,6 +7451,20 @@ static struct genl_family devlink_nl_family __ro_after_init = { >> .n_mcgrps = ARRAY_SIZE(devlink_nl_mcgrps), >> }; >> >> +static int devlink_reload_actions_verify(struct devlink *devlink) >> +{ >> + const struct devlink_ops *ops; >> + >> + if (!devlink_reload_supported(devlink)) > If reload is not supported, the supported_reload_actions should be 0. > Please check that with WARN_ON too. > OK, same goes to supported limit levels. >> + return 0; >> + >> + ops = devlink->ops; >> + if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || >> + ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) >> + return -EINVAL; >> + return 0; >> +} >> + >> /** >> * devlink_alloc - Allocate new devlink instance resources >> * >> @@ -7392,6 +7485,11 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) >> if (!devlink) >> return NULL; >> devlink->ops = ops; >> + if (devlink_reload_actions_verify(devlink)) { > Move this check to the beginning. You don't need devlink instance for > the check, just ops. Right, will fix. > also, your devlink_reload_actions_verify() function returns > 0/-ESOMETHING. Treat it accordingly here. Well, yes, but I rather return NULL here since devlink_alloc() failed. If devlink_reload_actions_verify() fails it has WARN_ON which will lead the driver developer to his bug. > >> + kfree(devlink); >> + return NULL; >> + } >> + >> xa_init_flags(&devlink->snapshot_ids, XA_FLAGS_ALLOC); >> __devlink_net_set(devlink, &init_net); >> INIT_LIST_HEAD(&devlink->port_list); >> @@ -9657,7 +9755,8 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) >> if (net_eq(devlink_net(devlink), net)) { >> if (WARN_ON(!devlink_reload_supported(devlink))) >> continue; >> - err = devlink_reload(devlink, &init_net, NULL); >> + err = devlink_reload(devlink, &init_net, >> + DEVLINK_RELOAD_ACTION_DRIVER_REINIT, NULL, NULL); >> if (err && err != -EOPNOTSUPP) >> pr_warn("Failed to reload devlink instance into init_net\n"); >> } >> -- >> 2.17.1 >>
On 9/14/2020 4:10 PM, Jiri Pirko wrote: > Mon, Sep 14, 2020 at 08:07:49AM CEST, moshe@mellanox.com wrote: > > [..] > > >> diff --git a/include/net/devlink.h b/include/net/devlink.h >> index b09db891db04..dddd9ee5b8a9 100644 >> --- a/include/net/devlink.h >> +++ b/include/net/devlink.h >> @@ -1012,9 +1012,13 @@ enum devlink_trap_group_generic_id { >> >> struct devlink_ops { >> unsigned long supported_reload_actions; >> + unsigned long supported_reload_action_limit_levels; >> int (*reload_down)(struct devlink *devlink, bool netns_change, >> - enum devlink_reload_action action, struct netlink_ext_ack *extack); >> + enum devlink_reload_action action, >> + enum devlink_reload_action_limit_level limit_level, >> + struct netlink_ext_ack *extack); >> int (*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); >> int (*port_type_set)(struct devlink_port *devlink_port, >> enum devlink_port_type port_type); >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> index a6f64db0bdf3..b19686fd80ff 100644 >> --- a/include/uapi/linux/devlink.h >> +++ b/include/uapi/linux/devlink.h >> @@ -287,6 +287,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 { >> + 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, >> @@ -478,6 +494,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 f4be1e1bf864..60aa0c4a3726 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -468,6 +468,13 @@ devlink_reload_action_is_supported(struct devlink *devlink, enum devlink_reload_ >> 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); >> +} >> + >> static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink, >> enum devlink_command cmd, u32 portid, >> u32 seq, int flags) >> @@ -2975,22 +2982,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); >> return err; >> } >> @@ -3036,6 +3044,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; >> @@ -3073,7 +3082,20 @@ 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 (limit_level > DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX) { > Again, not needed, devlink_reload_action_limit_level_is_supported() will > take case of it. Ack. >> + NL_SET_ERR_MSG_MOD(info->extack, "Invalid limit level"); >> + return -EINVAL; >> + } else 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"? Ack. > >> + return -EOPNOTSUPP; >> + } >> + err = devlink_reload(devlink, dest_net, action, limit_level, info->extack, >> + &actions_performed); >> >> if (dest_net) >> put_net(dest_net); >> @@ -7126,6 +7148,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 }, >> }; >> >> static const struct genl_ops devlink_nl_ops[] = { >> @@ -7462,6 +7485,10 @@ static int devlink_reload_actions_verify(struct devlink *devlink) >> if (WARN_ON(ops->supported_reload_actions >= BIT(__DEVLINK_RELOAD_ACTION_MAX) || >> ops->supported_reload_actions <= BIT(DEVLINK_RELOAD_ACTION_UNSPEC))) >> return -EINVAL; >> + if (WARN_ON(!ops->supported_reload_action_limit_levels || >> + ops->supported_reload_action_limit_levels >= >> + BIT(__DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX))) >> + return -EINVAL; > I think that you can check some insane driver combinations like: > supports only driver-reinit, supports LEVEL_NO_RESET - that is > impossible and should be refused here. > > Same goes to the actual user command call. If the user calls for > driver-reinit with LEVEL_NO_RESET, devlink should refuse with proper > extack I actually holds a counter for this combination too, we said no_reset can apply to any action, but not really. > >> return 0; >> } >> >> @@ -9756,7 +9783,8 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) >> if (WARN_ON(!devlink_reload_supported(devlink))) >> continue; >> err = devlink_reload(devlink, &init_net, >> - DEVLINK_RELOAD_ACTION_DRIVER_REINIT, NULL, NULL); >> + DEVLINK_RELOAD_ACTION_DRIVER_REINIT, >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE, NULL, NULL); >> if (err && err != -EOPNOTSUPP) >> pr_warn("Failed to reload devlink instance into init_net\n"); >> } >> -- >> 2.17.1 >>
On 9/15/2020 12:33 AM, Jakub Kicinski wrote: > External email: Use caution opening links or attachments > > > On Mon, 14 Sep 2020 09:07:48 +0300 Moshe Shemesh wrote: >> @@ -3011,12 +3060,41 @@ 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 || action > DEVLINK_RELOAD_ACTION_MAX) { >> + 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"); >> + 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; >> + >> + WARN_ON(!actions_performed); >> + 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); > I think generating the reply may break existing users. Only generate > the reply if request contained DEVLINK_ATTR_RELOAD_ACTION (or any other > new attribute which existing users can't pass). OK, I can do that. But I update stats and generate devlink notification anyway, that should fine, right ?
Tue, Sep 15, 2020 at 02:12:25PM CEST, moshe@nvidia.com wrote: > >On 9/14/2020 3:27 PM, Jiri Pirko wrote: >> Mon, Sep 14, 2020 at 08:07:48AM CEST, moshe@mellanox.com wrote: [..] >> > @@ -7392,6 +7485,11 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) >> > if (!devlink) >> > return NULL; >> > devlink->ops = ops; >> > + if (devlink_reload_actions_verify(devlink)) { >> Move this check to the beginning. You don't need devlink instance for >> the check, just ops. > > >Right, will fix. > >> also, your devlink_reload_actions_verify() function returns >> 0/-ESOMETHING. Treat it accordingly here. > > >Well, yes, but I rather return NULL here since devlink_alloc() failed. If >devlink_reload_actions_verify() fails it has WARN_ON which will lead the >driver developer to his bug. So let the verify() return bool. My point is, if a function return 0/-ESOMETHING, you should not check the return value directly but you should use int err/ret. > >> >> > + kfree(devlink); >> > + return NULL; >> > + } [...]
Tue, Sep 15, 2020 at 02:56:48PM CEST, moshe@nvidia.com wrote: > >On 9/15/2020 12:33 AM, Jakub Kicinski wrote: >> External email: Use caution opening links or attachments >> >> >> On Mon, 14 Sep 2020 09:07:48 +0300 Moshe Shemesh wrote: >> > @@ -3011,12 +3060,41 @@ 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 || action > DEVLINK_RELOAD_ACTION_MAX) { >> > + 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"); >> > + 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; >> > + >> > + WARN_ON(!actions_performed); >> > + 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); >> I think generating the reply may break existing users. Only generate >> the reply if request contained DEVLINK_ATTR_RELOAD_ACTION (or any other >> new attribute which existing users can't pass). > > >OK, I can do that. But I update stats and generate devlink notification >anyway, that should fine, right ? Yes. >
On Tue, 15 Sep 2020 15:56:48 +0300 Moshe Shemesh wrote: > On 9/15/2020 12:33 AM, Jakub Kicinski wrote: > >> + if (err) > >> + return err; > >> + > >> + WARN_ON(!actions_performed); > >> + 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); > > I think generating the reply may break existing users. Only generate > > the reply if request contained DEVLINK_ATTR_RELOAD_ACTION (or any other > > new attribute which existing users can't pass). > > OK, I can do that. But I update stats and generate devlink notification > anyway, that should fine, right ? Yes, that should be fine.
On Mon, 14 Sep 2020 09:08:02 +0300 Moshe Shemesh wrote: > + * - ``no_reset`` > + - No reset allowed, no down time allowed, no link flap and no > + configuration is lost. It still takes the PCI link down for up to 2sec. So there is down time, right?
On 9/15/2020 7:04 PM, Jakub Kicinski wrote: > External email: Use caution opening links or attachments > > > On Mon, 14 Sep 2020 09:08:02 +0300 Moshe Shemesh wrote: >> + * - ``no_reset`` >> + - No reset allowed, no down time allowed, no link flap and no >> + configuration is lost. > It still takes the PCI link down for up to 2sec. So there is down time, > right? No, the fw reset with PCI link down is categorized as fw_activate with limit level none. fw_live_patch keeps the pci link up and it fits to "no_reset" limit level.
On 9/15/2020 4:26 PM, Jiri Pirko wrote: > Tue, Sep 15, 2020 at 02:12:25PM CEST, moshe@nvidia.com wrote: >> On 9/14/2020 3:27 PM, Jiri Pirko wrote: >>> Mon, Sep 14, 2020 at 08:07:48AM CEST, moshe@mellanox.com wrote: > [..] > >>>> @@ -7392,6 +7485,11 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) >>>> if (!devlink) >>>> return NULL; >>>> devlink->ops = ops; >>>> + if (devlink_reload_actions_verify(devlink)) { >>> Move this check to the beginning. You don't need devlink instance for >>> the check, just ops. >> >> Right, will fix. >> >>> also, your devlink_reload_actions_verify() function returns >>> 0/-ESOMETHING. Treat it accordingly here. >> >> Well, yes, but I rather return NULL here since devlink_alloc() failed. If >> devlink_reload_actions_verify() fails it has WARN_ON which will lead the >> driver developer to his bug. > So let the verify() return bool. > My point is, if a function return 0/-ESOMETHING, you should not check > the return value directly but you should use int err/ret. OK, will fix. >>>> + kfree(devlink); >>>> + return NULL; >>>> + } > [...]
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 14 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 | 80 +++ Documentation/networking/devlink/index.rst | 1 + drivers/net/ethernet/mellanox/mlx4/main.c | 17 +- .../net/ethernet/mellanox/mlx5/core/Makefile | 2 +- .../net/ethernet/mellanox/mlx5/core/devlink.c | 124 ++++- .../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 | 28 +- drivers/net/netdevsim/dev.c | 18 +- include/linux/mlx5/device.h | 1 + include/linux/mlx5/driver.h | 4 + include/net/devlink.h | 20 +- include/uapi/linux/devlink.h | 39 ++ net/core/devlink.c | 243 +++++++++- 20 files changed, 1089 insertions(+), 49 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