| 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: <Ys1+ujEnt+dvNw2y@nanopsycho>
Date: Tue, 12 Jul 2022 16:01:30 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Ido Schimmel <idosch@...dia.com>
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
Tue, Jul 12, 2022 at 03:33:14PM CEST, idosch@...dia.com wrote:
>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
Yep.
>
>>
>> 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()
Will check.
>
>
>> }
>>
>> 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.
Correct. I have another patch in queue which removes devlink_mutex from
register/unregister, so that fixed it for me.
>
>> }
>> 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