Messages in this thread | | | Subject | Re: [PATCH net-next RFC v3 01/14] devlink: Add reload action option to devlink reload command | From | Moshe Shemesh <> | Date | Tue, 1 Sep 2020 22:43:00 +0300 |
| |
On 8/31/2020 3:15 PM, Jiri Pirko wrote: > Sun, Aug 30, 2020 at 05:27:21PM 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 done. >> However, in case fw_activate_no_reset action is selected, then no other >> reload action is allowed. >> Reload actions supported are: >> driver_reinit: driver entities re-initialization, applying devlink-param >> and devlink-resource values. >> fw_activate: firmware activate. >> fw_activate_no_reset: Activate new firmware image without any reset. >> (also known as: firmware live patching). >> >> command examples: >> $devlink dev reload pci/0000:82:00.0 action driver_reinit >> reload_actions_done: >> driver_reinit >> >> $devlink dev reload pci/0000:82:00.0 action fw_activate >> reload_actions_done: >> driver_reinit fw_activate >> >> Signed-off-by: Moshe Shemesh <moshe@mellanox.com> >> --- >> 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 >> --- > [...] > > >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c >> index 08d101138fbe..c42b66d88884 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,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_done) >> { >> 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_done) >> + *actions_done = 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 +1276,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), > This is confusing and open to interpretation. Does this mean that the > driver supports: > 1) REINIT && FW_ACTIVATE > 2) REINIT || FW_ACTIVATE > ? > > Because mlxsw supports only 1. I guess that mlx5 supports both. This > needs to be distinguished.
Mlxsw supports 1, so it supports fw_activation and performs also reinit and vice versa.
Mlx5 supports fw_activate and performs also reinit. However, it supports reinit without performing fw_activate.
> I think you need an array of combinations. Or perhaps rather to extend > the enum with combinations. You kind of have it already with > DEVLINK_RELOAD_ACTION_FW_ACTIVATE_NO_RESET > > Maybe we can have something like: > DEVLINK_RELOAD_ACTION_DRIVER_REINIT > DEVLINK_RELOAD_ACTION_DRIVER_REINIT_FW_ACTIVATE_RESET > DEVLINK_RELOAD_ACTION_FW_ACTIVATE_RESET > DEVLINK_RELOAD_ACTION_FW_ACTIVATE (this is the original FW_ACTIVATE_NO_RESET)
The FW_ACTIVATE_NO_RESET meant also to emphasize that driver implementation for this one should not do any reset.
So maybe we can have
DEVLINK_RELOAD_ACTION_FW_ACTIVATE_RESET DEVLINK_RELOAD_ACTION_FW_ACTIVATE_NO_RESET
> Each has very clear meaning.
Yes, it the driver support here is more clear.
> Also, then the "actions_done" would be a simple enum, directly returned > to the user. No bitfield needed.
I agree it is more clear on the driver support side, but what about the uAPI ? Do we need such change there too or keep it as is, each action by itself and return what was performed ?
> >> .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, > [...]
| |