Re: [PATCH net-next RFC v3 01/14] devlink: Add reload action option to devlink reload command

[Jiri Pirko <jiri@resnulli.us>]

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.

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)

Each has very clear meaning.

Also, then the "actions_done" would be a simple enum, directly returned
to the user. No bitfield needed.


> 	.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,

[...]