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

[Moshe Shemesh]

On 8/31/2020 3:15 PM, Jiri Pirko wrote:
> Sun, Aug 30, 2020 at 05:27:21PM CEST, moshe@xxxxxxxxxxxx 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@xxxxxxxxxxxx>
> > ---
> > 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,
> [...]