| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 1 Sep 2020 22:43:00 +0300
From: Moshe Shemesh <moshe@...dia.com>
To: Jiri Pirko <jiri@...nulli.us>, Moshe Shemesh <moshe@...lanox.com>
CC: "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jiri Pirko <jiri@...lanox.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next RFC v3 01/14] devlink: Add reload action option
to devlink reload command
On 8/31/2020 3:15 PM, Jiri Pirko wrote:
> Sun, Aug 30, 2020 at 05:27:21PM CEST, moshe@...lanox.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@...lanox.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,
> [...]
Powered by blists - more mailing lists