| 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
| ||
|
Message-ID: <Ys14Gorcg1JV5UIF@shredder>
Date: Tue, 12 Jul 2022 16:33:14 +0300
From: Ido Schimmel <idosch@...dia.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, edumazet@...gle.com, mlxsw@...dia.com,
saeedm@...dia.com, moshe@...dia.com, tariqt@...dia.com
Subject: Re: [patch net-next RFC 07/10] mlxsw: convert driver to use unlocked
devlink API during init/fini
On Tue, Jul 12, 2022 at 01:05:08PM +0200, Jiri Pirko wrote:
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
> index ab1cebf227fb..b0267e4dca27 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> @@ -127,11 +127,11 @@ static int mlxsw_core_resources_ports_register(struct mlxsw_core *mlxsw_core)
> max_ports, 1,
> DEVLINK_RESOURCE_UNIT_ENTRY);
>
> - return devlink_resource_register(devlink,
> - DEVLINK_RESOURCE_GENERIC_NAME_PORTS,
> - max_ports, MLXSW_CORE_RESOURCE_PORTS,
> - DEVLINK_RESOURCE_ID_PARENT_TOP,
> - &ports_num_params);
> + return devl_resource_register(devlink,
> + DEVLINK_RESOURCE_GENERIC_NAME_PORTS,
> + max_ports, MLXSW_CORE_RESOURCE_PORTS,
> + DEVLINK_RESOURCE_ID_PARENT_TOP,
> + &ports_num_params);
> }
>
> static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
> @@ -157,8 +157,8 @@ static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
> goto err_resources_ports_register;
> }
> atomic_set(&mlxsw_core->active_ports_count, 0);
> - devlink_resource_occ_get_register(devlink, MLXSW_CORE_RESOURCE_PORTS,
> - mlxsw_ports_occ_get, mlxsw_core);
> + devl_resource_occ_get_register(devlink, MLXSW_CORE_RESOURCE_PORTS,
> + mlxsw_ports_occ_get, mlxsw_core);
>
> return 0;
>
> @@ -171,9 +171,9 @@ static void mlxsw_ports_fini(struct mlxsw_core *mlxsw_core, bool reload)
> {
> struct devlink *devlink = priv_to_devlink(mlxsw_core);
>
> - devlink_resource_occ_get_unregister(devlink, MLXSW_CORE_RESOURCE_PORTS);
> + devl_resource_occ_get_unregister(devlink, MLXSW_CORE_RESOURCE_PORTS);
> if (!reload)
> - devlink_resources_unregister(priv_to_devlink(mlxsw_core));
> + devl_resources_unregister(priv_to_devlink(mlxsw_core));
>
> kfree(mlxsw_core->ports);
> }
> @@ -1485,10 +1485,12 @@ mlxsw_devlink_core_bus_device_reload_down(struct devlink *devlink,
> {
> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>
> + devl_lock(devlink);
> if (!(mlxsw_core->bus->features & MLXSW_BUS_F_RESET))
> return -EOPNOTSUPP;
Not releasing the lock. You can take it after this check as these
features do not change
>
> mlxsw_core_bus_device_unregister(mlxsw_core, true);
> + devl_unlock(devlink);
> return 0;
> }
>
> @@ -1498,13 +1500,17 @@ mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink, enum devlink_re
> struct netlink_ext_ack *extack)
> {
> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
> + int err;
>
> + devl_lock(devlink);
> *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
> BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE);
> - 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);
> + devl_unlock(devlink);
> + return err;
> }
>
> static int mlxsw_devlink_flash_update(struct devlink *devlink,
> @@ -2102,6 +2108,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
> err = -ENOMEM;
> goto err_devlink_alloc;
> }
> + devl_lock(devlink);
Why not just take it in mlxsw_core_bus_device_register() if '!reload' ?
Easier to read and also consistent with the change in
mlxsw_core_bus_device_unregister()
> }
>
> mlxsw_core = devlink_priv(devlink);
> @@ -2188,6 +2195,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
> if (!reload) {
> devlink_set_features(devlink, DEVLINK_F_RELOAD);
> devlink_register(devlink);
> + devl_unlock(devlink);
Did you check this with lockdep? devlink_register() now acquires the
global devlink mutex under the per-instance lock, but devlink core uses
the reverse order.
> }
> return 0;
>
> @@ -2214,12 +2222,14 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
> mlxsw_ports_fini(mlxsw_core, reload);
> err_ports_init:
> if (!reload)
> - devlink_resources_unregister(devlink);
> + devl_resources_unregister(devlink);
> err_register_resources:
> mlxsw_bus->fini(bus_priv);
> err_bus_init:
> - if (!reload)
> + if (!reload) {
> + devl_unlock(devlink);
> devlink_free(devlink);
> + }
> err_devlink_alloc:
> return err;
> }
> @@ -2255,8 +2265,10 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
> {
> struct devlink *devlink = priv_to_devlink(mlxsw_core);
>
> - if (!reload)
> + if (!reload) {
> + devl_lock(devlink);
> devlink_unregister(devlink);
> + }
>
> if (devlink_is_reload_failed(devlink)) {
> if (!reload)
> @@ -2281,17 +2293,20 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
> kfree(mlxsw_core->lag.mapping);
> mlxsw_ports_fini(mlxsw_core, reload);
> if (!reload)
> - devlink_resources_unregister(devlink);
> + devl_resources_unregister(devlink);
> mlxsw_core->bus->fini(mlxsw_core->bus_priv);
> - if (!reload)
> + if (!reload) {
> devlink_free(devlink);
> + devl_unlock(devlink);
> + }
>
> return;
>
> reload_fail_deinit:
> mlxsw_core_params_unregister(mlxsw_core);
> - devlink_resources_unregister(devlink);
> + devl_resources_unregister(devlink);
> devlink_free(devlink);
> + devl_unlock(devlink);
> }
> EXPORT_SYMBOL(mlxsw_core_bus_device_unregister);
Powered by blists - more mailing lists