From: Jiri Pirko <jiri@resnulli.us>
To: Ido Schimmel <idosch@nvidia.com>
Cc: netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org,
pabeni@redhat.com, edumazet@google.com, mlxsw@nvidia.com,
saeedm@nvidia.com, moshe@nvidia.com, tariqt@nvidia.com
Subject: Re: [patch net-next RFC 07/10] mlxsw: convert driver to use unlocked devlink API during init/fini
Date: Tue, 12 Jul 2022 16:01:30 +0200 [thread overview]
Message-ID: <Ys1+ujEnt+dvNw2y@nanopsycho> (raw)
In-Reply-To: <Ys14Gorcg1JV5UIF@shredder>
Tue, Jul 12, 2022 at 03:33:14PM CEST, idosch@nvidia.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);
next prev parent reply other threads:[~2022-07-12 14:02 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-12 11:05 [patch net-next RFC 00/12] net: devlink: prepare mlxsw and netdevsim for locked reload Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 01/10] net: devlink: avoid false DEADLOCK warning reported by lockdep Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 02/10] net: devlink: add unlocked variants of devling_trap*() functions Jiri Pirko
2022-07-12 13:15 ` Ido Schimmel
2022-07-12 11:05 ` [patch net-next RFC 03/10] net: devlink: add unlocked variants of devlink_resource*() functions Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 04/10] net: devlink: add unlocked variants of devlink_sb*() functions Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 05/10] net: devlink: add unlocked variants of devlink_dpipe*() functions Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 06/10] net: devlink: add unlocked variants of devlink_trap_policers*() functions Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 07/10] mlxsw: convert driver to use unlocked devlink API during init/fini Jiri Pirko
2022-07-12 13:33 ` Ido Schimmel
2022-07-12 14:01 ` Jiri Pirko [this message]
2022-07-12 14:16 ` Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 08/10] net: devlink: add unlocked variants of devlink_region_create/destroy() functions Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 09/10] netdevsim: convert driver to use unlocked devlink API during init/fini Jiri Pirko
2022-07-12 11:05 ` [patch net-next RFC 10/10] net: devlink: remove unused locked functions Jiri Pirko
2022-07-12 13:43 ` [patch net-next RFC 00/12] net: devlink: prepare mlxsw and netdevsim for locked reload Ido Schimmel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ys1+ujEnt+dvNw2y@nanopsycho \
--to=jiri@resnulli.us \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=idosch@nvidia.com \
--cc=kuba@kernel.org \
--cc=mlxsw@nvidia.com \
--cc=moshe@nvidia.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=saeedm@nvidia.com \
--cc=tariqt@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).