From 367dfa1212050b9418b890a2f74a3550e31b571d Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:00 -0700 Subject: [PATCH 1/9] net/mlx5: Remove devl_unlock from mlx5_eswtich_mode_callback_enter The function mlx5_eswtich_mode_callback_enter() was added as a temporary workaround once devlink instance lock was added to devlink eswitch callbacks. However, code review and testing show that all the callbacks part to eswitch_mode_set don't take devlink instance lock in any flow and so unlocking devlink instance lock while entering these functions is not needed. Remove devl_lock from mlx5_eswtich_mode_callback_enter() and devl_unlock from mlx5_eswtich_mode_callback_exit(). Also remove the functions mlx5_eswtich_mode_callback_enter()/exit() as they are not needed any more. The callback eswitch_mode_set will be treated separately in the following patches. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- .../mellanox/mlx5/core/eswitch_offloads.c | 43 +++++-------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index e224ec7005a6..3bd843e6d66a 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -3342,27 +3342,6 @@ static int esw_inline_mode_to_devlink(u8 mlx5_mode, u8 *mode) return 0; } -/* FIXME: devl_unlock() followed by devl_lock() inside driver callback - * is never correct and prone to races. It's a transitional workaround, - * never repeat this pattern. - * - * This code MUST be fixed before removing devlink_mutex as it is safe - * to do only because of that mutex. - */ -static void mlx5_eswtich_mode_callback_enter(struct devlink *devlink, - struct mlx5_eswitch *esw) -{ - devl_unlock(devlink); - down_write(&esw->mode_lock); -} - -static void mlx5_eswtich_mode_callback_exit(struct devlink *devlink, - struct mlx5_eswitch *esw) -{ - up_write(&esw->mode_lock); - devl_lock(devlink); -} - int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, struct netlink_ext_ack *extack) { @@ -3431,9 +3410,9 @@ int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode) if (IS_ERR(esw)) return PTR_ERR(esw); - mlx5_eswtich_mode_callback_enter(devlink, esw); + down_write(&esw->mode_lock); err = esw_mode_to_devlink(esw->mode, mode); - mlx5_eswtich_mode_callback_exit(devlink, esw); + up_write(&esw->mode_lock); return err; } @@ -3480,7 +3459,7 @@ int mlx5_devlink_eswitch_inline_mode_set(struct devlink *devlink, u8 mode, if (IS_ERR(esw)) return PTR_ERR(esw); - mlx5_eswtich_mode_callback_enter(devlink, esw); + down_write(&esw->mode_lock); switch (MLX5_CAP_ETH(dev, wqe_inline_mode)) { case MLX5_CAP_INLINE_MODE_NOT_REQUIRED: @@ -3514,11 +3493,11 @@ int mlx5_devlink_eswitch_inline_mode_set(struct devlink *devlink, u8 mode, goto out; esw->offloads.inline_mode = mlx5_mode; - mlx5_eswtich_mode_callback_exit(devlink, esw); + up_write(&esw->mode_lock); return 0; out: - mlx5_eswtich_mode_callback_exit(devlink, esw); + up_write(&esw->mode_lock); return err; } @@ -3531,9 +3510,9 @@ int mlx5_devlink_eswitch_inline_mode_get(struct devlink *devlink, u8 *mode) if (IS_ERR(esw)) return PTR_ERR(esw); - mlx5_eswtich_mode_callback_enter(devlink, esw); + down_write(&esw->mode_lock); err = esw_inline_mode_to_devlink(esw->offloads.inline_mode, mode); - mlx5_eswtich_mode_callback_exit(devlink, esw); + up_write(&esw->mode_lock); return err; } @@ -3549,7 +3528,7 @@ int mlx5_devlink_eswitch_encap_mode_set(struct devlink *devlink, if (IS_ERR(esw)) return PTR_ERR(esw); - mlx5_eswtich_mode_callback_enter(devlink, esw); + down_write(&esw->mode_lock); if (encap != DEVLINK_ESWITCH_ENCAP_MODE_NONE && (!MLX5_CAP_ESW_FLOWTABLE_FDB(dev, reformat) || @@ -3592,7 +3571,7 @@ int mlx5_devlink_eswitch_encap_mode_set(struct devlink *devlink, } unlock: - mlx5_eswtich_mode_callback_exit(devlink, esw); + up_write(&esw->mode_lock); return err; } @@ -3605,9 +3584,9 @@ int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink, if (IS_ERR(esw)) return PTR_ERR(esw); - mlx5_eswtich_mode_callback_enter(devlink, esw); + down_write(&esw->mode_lock); *encap = esw->offloads.encap; - mlx5_eswtich_mode_callback_exit(devlink, esw); + up_write(&esw->mode_lock); return 0; } From 03f9c47d0f7983bc73854ee34be6814b580ac7fc Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:01 -0700 Subject: [PATCH 2/9] net/mlx5: Use devl_ API for rate nodes destroy Use devl_rate_nodes_destroy() instead of devlink_rate_nodes_destroy(). Add devlink instance lock in the driver paths to this function to have it locked while calling devl_ API function. This will be used by the downstream patch to invoke mlx5_devlink_eswitch_mode_set() with devlink lock held. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 14 ++++++++++++-- .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c index b938632f89ff..571114e4878f 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c @@ -1330,9 +1330,13 @@ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs) /* When disabling sriov, free driver level resources. */ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw, bool clear_vf) { + struct devlink *devlink; + if (!mlx5_esw_allowed(esw)) return; + devlink = priv_to_devlink(esw->dev); + devl_lock(devlink); down_write(&esw->mode_lock); /* If driver is unloaded, this function is called twice by remove_one() * and mlx5_unload(). Prevent the second call. @@ -1354,13 +1358,14 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw, bool clear_vf) struct devlink *devlink = priv_to_devlink(esw->dev); esw_offloads_del_send_to_vport_meta_rules(esw); - devlink_rate_nodes_destroy(devlink); + devl_rate_nodes_destroy(devlink); } esw->esw_funcs.num_vfs = 0; unlock: up_write(&esw->mode_lock); + devl_unlock(devlink); } /* Free resources for corresponding eswitch mode. It is called by devlink @@ -1389,18 +1394,23 @@ void mlx5_eswitch_disable_locked(struct mlx5_eswitch *esw) mlx5_esw_acls_ns_cleanup(esw); if (esw->mode == MLX5_ESWITCH_OFFLOADS) - devlink_rate_nodes_destroy(devlink); + devl_rate_nodes_destroy(devlink); } void mlx5_eswitch_disable(struct mlx5_eswitch *esw) { + struct devlink *devlink; + if (!mlx5_esw_allowed(esw)) return; mlx5_lag_disable_change(esw->dev); + devlink = priv_to_devlink(esw->dev); + devl_lock(devlink); down_write(&esw->mode_lock); mlx5_eswitch_disable_locked(esw); up_write(&esw->mode_lock); + devl_unlock(devlink); mlx5_lag_enable_change(esw->dev); } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index 3bd843e6d66a..f1640e4cb719 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -3377,7 +3377,9 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, if (cur_mlx5_mode == mlx5_mode) goto unlock; + devl_lock(devlink); mlx5_eswitch_disable_locked(esw); + devl_unlock(devlink); if (mode == DEVLINK_ESWITCH_MODE_SWITCHDEV) { if (mlx5_devlink_trap_get_num_active(esw->dev)) { NL_SET_ERR_MSG_MOD(extack, From 868232f5cd382c37a5005deb7be0620c6f7bc08d Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:02 -0700 Subject: [PATCH 3/9] devlink: Remove unused function devlink_rate_nodes_destroy The previous patch removed the last usage of the function devlink_rate_nodes_destroy(). Thus, remove this function from devlink API. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- include/net/devlink.h | 1 - net/core/devlink.c | 18 ------------------ 2 files changed, 19 deletions(-) diff --git a/include/net/devlink.h b/include/net/devlink.h index 2a2a2a0c93f7..0e163cc87d45 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1571,7 +1571,6 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, bool external); int devlink_rate_leaf_create(struct devlink_port *port, void *priv); void devlink_rate_leaf_destroy(struct devlink_port *devlink_port); -void devlink_rate_nodes_destroy(struct devlink *devlink); void devlink_port_linecard_set(struct devlink_port *devlink_port, struct devlink_linecard *linecard); struct devlink_linecard * diff --git a/net/core/devlink.c b/net/core/devlink.c index db61f3a341cb..1588e2246234 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -10095,24 +10095,6 @@ void devl_rate_nodes_destroy(struct devlink *devlink) } EXPORT_SYMBOL_GPL(devl_rate_nodes_destroy); -/** - * devlink_rate_nodes_destroy - destroy all devlink rate nodes on device - * - * @devlink: devlink instance - * - * Unset parent for all rate objects and destroy all rate nodes - * on specified device. - * - * Context: Takes and release devlink->lock . - */ -void devlink_rate_nodes_destroy(struct devlink *devlink) -{ - mutex_lock(&devlink->lock); - devl_rate_nodes_destroy(devlink); - mutex_unlock(&devlink->lock); -} -EXPORT_SYMBOL_GPL(devlink_rate_nodes_destroy); - /** * devlink_port_linecard_set - Link port with a linecard * From f1bc646c9a06f09aad5d8bacb87103b5573ee45e Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:03 -0700 Subject: [PATCH 4/9] net/mlx5: Use devl_ API in mlx5_esw_offloads_devlink_port_register The function mlx5_esw_offloads_devlink_port_register() calls devlink_port_register() and devlink_rate_leaf_create(). Use devl_ API to call devl_port_register() and devl_rate_leaf_create() accordingly and add devlink instance lock in driver paths to this function. Similarly, use devl_ API to call devl_port_unregister() and devl_rate_leaf_destroy() in mlx5_esw_offloads_devlink_port_unregister() and ensure locking devlink instance lock on the paths to this function too. This will be used by the downstream patch to invoke mlx5_devlink_eswitch_mode_set() with devlink lock held. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- .../net/ethernet/mellanox/mlx5/core/esw/devlink_port.c | 10 +++++----- drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 4 ++++ .../net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 10 ++++++++++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c index 7f9b96d9537e..a8f7618831f5 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c @@ -87,11 +87,11 @@ int mlx5_esw_offloads_devlink_port_register(struct mlx5_eswitch *esw, u16 vport_ devlink = priv_to_devlink(dev); dl_port_index = mlx5_esw_vport_to_devlink_port_index(dev, vport_num); - err = devlink_port_register(devlink, dl_port, dl_port_index); + err = devl_port_register(devlink, dl_port, dl_port_index); if (err) goto reg_err; - err = devlink_rate_leaf_create(dl_port, vport); + err = devl_rate_leaf_create(dl_port, vport); if (err) goto rate_err; @@ -99,7 +99,7 @@ int mlx5_esw_offloads_devlink_port_register(struct mlx5_eswitch *esw, u16 vport_ return 0; rate_err: - devlink_port_unregister(dl_port); + devl_port_unregister(dl_port); reg_err: mlx5_esw_dl_port_free(dl_port); return err; @@ -118,10 +118,10 @@ void mlx5_esw_offloads_devlink_port_unregister(struct mlx5_eswitch *esw, u16 vpo if (vport->dl_port->devlink_rate) { mlx5_esw_qos_vport_update_group(esw, vport, NULL, NULL); - devlink_rate_leaf_destroy(vport->dl_port); + devl_rate_leaf_destroy(vport->dl_port); } - devlink_port_unregister(vport->dl_port); + devl_port_unregister(vport->dl_port); mlx5_esw_dl_port_free(vport->dl_port); vport->dl_port = NULL; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c index 571114e4878f..b95f75431882 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c @@ -1296,6 +1296,7 @@ int mlx5_eswitch_enable_locked(struct mlx5_eswitch *esw, int num_vfs) */ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs) { + struct devlink *devlink; bool toggle_lag; int ret; @@ -1307,6 +1308,8 @@ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs) if (toggle_lag) mlx5_lag_disable_change(esw->dev); + devlink = priv_to_devlink(esw->dev); + devl_lock(devlink); down_write(&esw->mode_lock); if (!mlx5_esw_is_fdb_created(esw)) { ret = mlx5_eswitch_enable_locked(esw, num_vfs); @@ -1320,6 +1323,7 @@ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs) esw->esw_funcs.num_vfs = num_vfs; } up_write(&esw->mode_lock); + devl_unlock(devlink); if (toggle_lag) mlx5_lag_enable_change(esw->dev); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index f1640e4cb719..1bfbc88f513f 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -2177,8 +2177,10 @@ static int esw_create_restore_table(struct mlx5_eswitch *esw) static int esw_offloads_start(struct mlx5_eswitch *esw, struct netlink_ext_ack *extack) { + struct devlink *devlink = priv_to_devlink(esw->dev); int err, err1; + devl_lock(devlink); esw->mode = MLX5_ESWITCH_OFFLOADS; err = mlx5_eswitch_enable_locked(esw, esw->dev->priv.sriov.num_vfs); if (err) { @@ -2200,6 +2202,7 @@ static int esw_offloads_start(struct mlx5_eswitch *esw, "Inline mode is different between vports"); } } + devl_unlock(devlink); return err; } @@ -3064,6 +3067,7 @@ static void esw_offloads_steering_cleanup(struct mlx5_eswitch *esw) static void esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, const u32 *out) { + struct devlink *devlink; bool host_pf_disabled; u16 new_num_vfs; @@ -3075,6 +3079,8 @@ esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, const u32 *out) if (new_num_vfs == esw->esw_funcs.num_vfs || host_pf_disabled) return; + devlink = priv_to_devlink(esw->dev); + devl_lock(devlink); /* Number of VFs can only change from "0 to x" or "x to 0". */ if (esw->esw_funcs.num_vfs > 0) { mlx5_eswitch_unload_vf_vports(esw, esw->esw_funcs.num_vfs); @@ -3087,6 +3093,7 @@ esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, const u32 *out) return; } esw->esw_funcs.num_vfs = new_num_vfs; + devl_unlock(devlink); } static void esw_functions_changed_event_handler(struct work_struct *work) @@ -3236,8 +3243,10 @@ int esw_offloads_enable(struct mlx5_eswitch *esw) static int esw_offloads_stop(struct mlx5_eswitch *esw, struct netlink_ext_ack *extack) { + struct devlink *devlink = priv_to_devlink(esw->dev); int err, err1; + devl_lock(devlink); esw->mode = MLX5_ESWITCH_LEGACY; err = mlx5_eswitch_enable_locked(esw, MLX5_ESWITCH_IGNORE_NUM_VFS); if (err) { @@ -3249,6 +3258,7 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw, "Failed setting eswitch back to offloads"); } } + devl_unlock(devlink); return err; } From da212bd29d7fdc326f0be47d15183d62e9c7c172 Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:04 -0700 Subject: [PATCH 5/9] net/mlx5: Use devl_ API in mlx5_esw_devlink_sf_port_register The function mlx5_esw_devlink_sf_port_register() calls devlink_port_register() and devlink_rate_leaf_create(). Use devl_ API to call devl_port_register() and devl_rate_leaf_create() accordingly and add devlink instance lock in driver paths to this function. Similarly, use devl_ API to call devl_port_unregister() and devl_rate_leaf_destroy() in mlx5_esw_devlink_sf_port_unregister() and ensure locking devlink instance lock on all the paths to this function too. This will be used by the downstream patch to invoke mlx5_devlink_eswitch_mode_set() with devlink lock held. Note this patch is taking devlink lock on mlx5_devlink_sf_port_new/del() which are devlink callbacks for port_new/del(). We will take these locks off once these callbacks will be locked by devlink too. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- .../net/ethernet/mellanox/mlx5/core/esw/devlink_port.c | 10 +++++----- drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c | 4 ++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c index a8f7618831f5..9bc7be95db54 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c @@ -156,11 +156,11 @@ int mlx5_esw_devlink_sf_port_register(struct mlx5_eswitch *esw, struct devlink_p devlink_port_attrs_pci_sf_set(dl_port, controller, pfnum, sfnum, !!controller); devlink = priv_to_devlink(dev); dl_port_index = mlx5_esw_vport_to_devlink_port_index(dev, vport_num); - err = devlink_port_register(devlink, dl_port, dl_port_index); + err = devl_port_register(devlink, dl_port, dl_port_index); if (err) return err; - err = devlink_rate_leaf_create(dl_port, vport); + err = devl_rate_leaf_create(dl_port, vport); if (err) goto rate_err; @@ -168,7 +168,7 @@ int mlx5_esw_devlink_sf_port_register(struct mlx5_eswitch *esw, struct devlink_p return 0; rate_err: - devlink_port_unregister(dl_port); + devl_port_unregister(dl_port); return err; } @@ -182,9 +182,9 @@ void mlx5_esw_devlink_sf_port_unregister(struct mlx5_eswitch *esw, u16 vport_num if (vport->dl_port->devlink_rate) { mlx5_esw_qos_vport_update_group(esw, vport, NULL, NULL); - devlink_rate_leaf_destroy(vport->dl_port); + devl_rate_leaf_destroy(vport->dl_port); } - devlink_port_unregister(vport->dl_port); + devl_port_unregister(vport->dl_port); vport->dl_port = NULL; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c index 7d955a4d9f14..2068c22ff367 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c @@ -355,7 +355,9 @@ int mlx5_devlink_sf_port_new(struct devlink *devlink, "Port add is only supported in eswitch switchdev mode or SF ports are disabled."); return -EOPNOTSUPP; } + devl_lock(devlink); err = mlx5_sf_add(dev, table, new_attr, extack, new_port_index); + devl_unlock(devlink); mlx5_sf_table_put(table); return err; } @@ -400,7 +402,9 @@ int mlx5_devlink_sf_port_del(struct devlink *devlink, unsigned int port_index, goto sf_err; } + devl_lock(devlink); mlx5_esw_offloads_sf_vport_disable(esw, sf->hw_fn_id); + devl_unlock(devlink); mlx5_sf_id_erase(table, sf); mutex_lock(&table->sf_state_lock); From df539fc62b069ed2b2395d8d1c77f7758c5001a6 Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:05 -0700 Subject: [PATCH 6/9] devlink: Remove unused functions devlink_rate_leaf_create/destroy The previous patch removed the last usage of the functions devlink_rate_leaf_create() and devlink_rate_nodes_destroy(). Thus, remove these function from devlink API. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- include/net/devlink.h | 2 -- net/core/devlink.c | 42 +++++++----------------------------------- 2 files changed, 7 insertions(+), 37 deletions(-) diff --git a/include/net/devlink.h b/include/net/devlink.h index 0e163cc87d45..5150deb67fab 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1569,8 +1569,6 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 controller, u16 pf, u32 sf, bool external); -int devlink_rate_leaf_create(struct devlink_port *port, void *priv); -void devlink_rate_leaf_destroy(struct devlink_port *devlink_port); void devlink_port_linecard_set(struct devlink_port *devlink_port, struct devlink_linecard *linecard); struct devlink_linecard * diff --git a/net/core/devlink.c b/net/core/devlink.c index 1588e2246234..970e5c2a52bd 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -10006,20 +10006,13 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv) } EXPORT_SYMBOL_GPL(devl_rate_leaf_create); -int -devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv) -{ - struct devlink *devlink = devlink_port->devlink; - int ret; - - mutex_lock(&devlink->lock); - ret = devl_rate_leaf_create(devlink_port, priv); - mutex_unlock(&devlink->lock); - - return ret; -} -EXPORT_SYMBOL_GPL(devlink_rate_leaf_create); - +/** + * devl_rate_leaf_destroy - destroy devlink rate leaf + * + * @devlink_port: devlink port linked to the rate object + * + * Destroy the devlink rate object of type leaf on provided @devlink_port. + */ void devl_rate_leaf_destroy(struct devlink_port *devlink_port) { struct devlink_rate *devlink_rate = devlink_port->devlink_rate; @@ -10037,27 +10030,6 @@ void devl_rate_leaf_destroy(struct devlink_port *devlink_port) } EXPORT_SYMBOL_GPL(devl_rate_leaf_destroy); -/** - * devlink_rate_leaf_destroy - destroy devlink rate leaf - * - * @devlink_port: devlink port linked to the rate object - * - * Context: Takes and release devlink->lock . - */ -void devlink_rate_leaf_destroy(struct devlink_port *devlink_port) -{ - struct devlink_rate *devlink_rate = devlink_port->devlink_rate; - struct devlink *devlink = devlink_port->devlink; - - if (!devlink_rate) - return; - - mutex_lock(&devlink->lock); - devl_rate_leaf_destroy(devlink_port); - mutex_unlock(&devlink->lock); -} -EXPORT_SYMBOL_GPL(devlink_rate_leaf_destroy); - /** * devl_rate_nodes_destroy - destroy all devlink rate nodes on device * @devlink: devlink instance From 7b19119f4c7dc9412b556ea12fae6b32574e2810 Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:06 -0700 Subject: [PATCH 7/9] net/mlx5: Use devl_ API in mlx5e_devlink_port_register As part of the flows invoked by mlx5_devlink_eswitch_mode_set() get to mlx5_rescan_drivers_locked() which can call mlx5e_probe()/mlx5e_remove and register/unregister mlx5e driver ports accordingly. This can lead to deadlock once mlx5_devlink_eswitch_mode_set() will use devlink lock. Use devl_port_register/unregister() instead of devlink_port_register/unregister() and add devlink instance locks in the driver paths to this function to have it locked while calling devl_ API function. If remove or probe were called by module init or module cleanup flows, need to lock devlink just before calling devl_port_register(), otherwise it is called by attach/detach or register/unregister flow and we can have the flow locked. Added flag to distinguish between these cases. This will be used by the downstream patch to invoke mlx5_devlink_eswitch_mode_set() with devlink locked. Signed-off-by: Moshe Shemesh Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- drivers/net/ethernet/mellanox/mlx5/core/dev.c | 29 +++++++++++++++++-- .../ethernet/mellanox/mlx5/core/en/devlink.c | 16 ++++++++-- .../mellanox/mlx5/core/eswitch_offloads.c | 2 ++ include/linux/mlx5/driver.h | 4 +++ 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c index 50422b56a64d..ccf2068d2e79 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c @@ -335,13 +335,16 @@ static void del_adev(struct auxiliary_device *adev) int mlx5_attach_device(struct mlx5_core_dev *dev) { + struct devlink *devlink = priv_to_devlink(dev); struct mlx5_priv *priv = &dev->priv; struct auxiliary_device *adev; struct auxiliary_driver *adrv; int ret = 0, i; + devl_lock(devlink); mutex_lock(&mlx5_intf_mutex); priv->flags &= ~MLX5_PRIV_FLAGS_DETACH; + priv->flags |= MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW; for (i = 0; i < ARRAY_SIZE(mlx5_adev_devices); i++) { if (!priv->adev[i]) { bool is_supported = false; @@ -389,19 +392,24 @@ int mlx5_attach_device(struct mlx5_core_dev *dev) break; } } + priv->flags &= ~MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW; mutex_unlock(&mlx5_intf_mutex); + devl_unlock(devlink); return ret; } void mlx5_detach_device(struct mlx5_core_dev *dev) { + struct devlink *devlink = priv_to_devlink(dev); struct mlx5_priv *priv = &dev->priv; struct auxiliary_device *adev; struct auxiliary_driver *adrv; pm_message_t pm = {}; int i; + devl_lock(devlink); mutex_lock(&mlx5_intf_mutex); + priv->flags |= MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW; for (i = ARRAY_SIZE(mlx5_adev_devices) - 1; i >= 0; i--) { if (!priv->adev[i]) continue; @@ -430,18 +438,24 @@ void mlx5_detach_device(struct mlx5_core_dev *dev) del_adev(&priv->adev[i]->adev); priv->adev[i] = NULL; } + priv->flags &= ~MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW; priv->flags |= MLX5_PRIV_FLAGS_DETACH; mutex_unlock(&mlx5_intf_mutex); + devl_unlock(devlink); } int mlx5_register_device(struct mlx5_core_dev *dev) { + struct devlink *devlink; int ret; + devlink = priv_to_devlink(dev); + devl_lock(devlink); mutex_lock(&mlx5_intf_mutex); dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV; ret = mlx5_rescan_drivers_locked(dev); mutex_unlock(&mlx5_intf_mutex); + devl_unlock(devlink); if (ret) mlx5_unregister_device(dev); @@ -450,10 +464,15 @@ int mlx5_register_device(struct mlx5_core_dev *dev) void mlx5_unregister_device(struct mlx5_core_dev *dev) { + struct devlink *devlink; + + devlink = priv_to_devlink(dev); + devl_lock(devlink); mutex_lock(&mlx5_intf_mutex); dev->priv.flags = MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV; mlx5_rescan_drivers_locked(dev); mutex_unlock(&mlx5_intf_mutex); + devl_unlock(devlink); } static int add_drivers(struct mlx5_core_dev *dev) @@ -526,16 +545,22 @@ static void delete_drivers(struct mlx5_core_dev *dev) int mlx5_rescan_drivers_locked(struct mlx5_core_dev *dev) { struct mlx5_priv *priv = &dev->priv; + int err = 0; lockdep_assert_held(&mlx5_intf_mutex); if (priv->flags & MLX5_PRIV_FLAGS_DETACH) return 0; + priv->flags |= MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW; delete_drivers(dev); if (priv->flags & MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV) - return 0; + goto out; - return add_drivers(dev); + err = add_drivers(dev); + +out: + priv->flags &= ~MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW; + return err; } bool mlx5_same_hw_devs(struct mlx5_core_dev *dev, struct mlx5_core_dev *peer_dev) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/en/devlink.c index ae52e7f38306..b69f9d10ccbd 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/devlink.c @@ -21,6 +21,7 @@ int mlx5e_devlink_port_register(struct mlx5e_priv *priv) struct netdev_phys_item_id ppid = {}; struct devlink_port *dl_port; unsigned int dl_port_index; + int ret; if (mlx5_core_is_pf(priv->mdev)) { attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; @@ -41,7 +42,13 @@ int mlx5e_devlink_port_register(struct mlx5e_priv *priv) memset(dl_port, 0, sizeof(*dl_port)); devlink_port_attrs_set(dl_port, &attrs); - return devlink_port_register(devlink, dl_port, dl_port_index); + if (!(priv->mdev->priv.flags & MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW)) + devl_lock(devlink); + ret = devl_port_register(devlink, dl_port, dl_port_index); + if (!(priv->mdev->priv.flags & MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW)) + devl_unlock(devlink); + + return ret; } void mlx5e_devlink_port_type_eth_set(struct mlx5e_priv *priv) @@ -54,8 +61,13 @@ void mlx5e_devlink_port_type_eth_set(struct mlx5e_priv *priv) void mlx5e_devlink_port_unregister(struct mlx5e_priv *priv) { struct devlink_port *dl_port = mlx5e_devlink_get_dl_port(priv); + struct devlink *devlink = priv_to_devlink(priv->mdev); - devlink_port_unregister(dl_port); + if (!(priv->mdev->priv.flags & MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW)) + devl_lock(devlink); + devl_port_unregister(dl_port); + if (!(priv->mdev->priv.flags & MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW)) + devl_unlock(devlink); } struct devlink_port *mlx5e_get_devlink_port(struct net_device *dev) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index 1bfbc88f513f..ccda3a0a2594 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -3400,7 +3400,9 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, err = esw_offloads_start(esw, extack); } else if (mode == DEVLINK_ESWITCH_MODE_LEGACY) { err = esw_offloads_stop(esw, extack); + devl_lock(devlink); mlx5_rescan_drivers(esw->dev); + devl_unlock(devlink); } else { err = -EINVAL; } diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h index 76d7661e3e63..bd882884b23c 100644 --- a/include/linux/mlx5/driver.h +++ b/include/linux/mlx5/driver.h @@ -551,6 +551,10 @@ enum { * creation/deletion on drivers rescan. Unset during device attach. */ MLX5_PRIV_FLAGS_DETACH = 1 << 2, + /* Distinguish between mlx5e_probe/remove called by module init/cleanup + * and called by other flows which can already hold devlink lock + */ + MLX5_PRIV_FLAGS_MLX5E_LOCKED_FLOW = 1 << 3, }; struct mlx5_adev { From 973598d46ede27bb3b2a54ff45135196aeb9efb0 Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:07 -0700 Subject: [PATCH 8/9] net/mlx5: Remove devl_unlock from mlx5_devlink_eswitch_mode_set The callback mlx5_devlink_eswitch_mode_set() had unlocked devlink as a temporary workaround once devlink instance lock was added to devlink eswitch callbacks. Now that all flows triggered by this function that took devlink lock are using devl_ API and all parallel paths are locked we can remove this workaround. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- .../mellanox/mlx5/core/eswitch_offloads.c | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index ccda3a0a2594..d3da52e3fc67 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -2177,10 +2177,8 @@ static int esw_create_restore_table(struct mlx5_eswitch *esw) static int esw_offloads_start(struct mlx5_eswitch *esw, struct netlink_ext_ack *extack) { - struct devlink *devlink = priv_to_devlink(esw->dev); int err, err1; - devl_lock(devlink); esw->mode = MLX5_ESWITCH_OFFLOADS; err = mlx5_eswitch_enable_locked(esw, esw->dev->priv.sriov.num_vfs); if (err) { @@ -2202,7 +2200,6 @@ static int esw_offloads_start(struct mlx5_eswitch *esw, "Inline mode is different between vports"); } } - devl_unlock(devlink); return err; } @@ -3243,10 +3240,8 @@ int esw_offloads_enable(struct mlx5_eswitch *esw) static int esw_offloads_stop(struct mlx5_eswitch *esw, struct netlink_ext_ack *extack) { - struct devlink *devlink = priv_to_devlink(esw->dev); int err, err1; - devl_lock(devlink); esw->mode = MLX5_ESWITCH_LEGACY; err = mlx5_eswitch_enable_locked(esw, MLX5_ESWITCH_IGNORE_NUM_VFS); if (err) { @@ -3258,7 +3253,6 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw, "Failed setting eswitch back to offloads"); } } - devl_unlock(devlink); return err; } @@ -3366,15 +3360,6 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, if (esw_mode_from_devlink(mode, &mlx5_mode)) return -EINVAL; - /* FIXME: devl_unlock() followed by devl_lock() inside driver callback - * is never correct and prone to races. It's a transitional workaround, - * never repeat this pattern. - * - * This code MUST be fixed before removing devlink_mutex as it is safe - * to do only because of that mutex. - */ - devl_unlock(devlink); - mlx5_lag_disable_change(esw->dev); err = mlx5_esw_try_lock(esw); if (err < 0) { @@ -3387,9 +3372,7 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, if (cur_mlx5_mode == mlx5_mode) goto unlock; - devl_lock(devlink); mlx5_eswitch_disable_locked(esw); - devl_unlock(devlink); if (mode == DEVLINK_ESWITCH_MODE_SWITCHDEV) { if (mlx5_devlink_trap_get_num_active(esw->dev)) { NL_SET_ERR_MSG_MOD(extack, @@ -3400,9 +3383,7 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, err = esw_offloads_start(esw, extack); } else if (mode == DEVLINK_ESWITCH_MODE_LEGACY) { err = esw_offloads_stop(esw, extack); - devl_lock(devlink); mlx5_rescan_drivers(esw->dev); - devl_unlock(devlink); } else { err = -EINVAL; } @@ -3411,7 +3392,6 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode, mlx5_esw_unlock(esw); enable_lag: mlx5_lag_enable_change(esw->dev); - devl_lock(devlink); return err; } From f0680ef0f9497f88b513dea1ae54664f0806ecfb Mon Sep 17 00:00:00 2001 From: Moshe Shemesh Date: Mon, 11 Jul 2022 01:14:08 -0700 Subject: [PATCH 9/9] devlink: Hold the instance lock in port_new / port_del callbacks Let the core take the devlink instance lock around port_new and port_del callbacks and remove the now redundant locking in the only driver that currently use them. Signed-off-by: Moshe Shemesh Reviewed-by: Leon Romanovsky Signed-off-by: Saeed Mahameed Signed-off-by: Paolo Abeni --- drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c | 4 ---- net/core/devlink.c | 6 +----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c index 2068c22ff367..7d955a4d9f14 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c @@ -355,9 +355,7 @@ int mlx5_devlink_sf_port_new(struct devlink *devlink, "Port add is only supported in eswitch switchdev mode or SF ports are disabled."); return -EOPNOTSUPP; } - devl_lock(devlink); err = mlx5_sf_add(dev, table, new_attr, extack, new_port_index); - devl_unlock(devlink); mlx5_sf_table_put(table); return err; } @@ -402,9 +400,7 @@ int mlx5_devlink_sf_port_del(struct devlink *devlink, unsigned int port_index, goto sf_err; } - devl_lock(devlink); mlx5_esw_offloads_sf_vport_disable(esw, sf->hw_fn_id); - devl_unlock(devlink); mlx5_sf_id_erase(table, sf); mutex_lock(&table->sf_state_lock); diff --git a/net/core/devlink.c b/net/core/devlink.c index 970e5c2a52bd..e206cc90bec5 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -1712,7 +1712,7 @@ static int devlink_port_new_notifiy(struct devlink *devlink, if (!msg) return -ENOMEM; - mutex_lock(&devlink->lock); + lockdep_assert_held(&devlink->lock); devlink_port = devlink_port_get_by_index(devlink, port_index); if (!devlink_port) { err = -ENODEV; @@ -1725,11 +1725,9 @@ static int devlink_port_new_notifiy(struct devlink *devlink, goto out; err = genlmsg_reply(msg, info); - mutex_unlock(&devlink->lock); return err; out: - mutex_unlock(&devlink->lock); nlmsg_free(msg); return err; } @@ -9067,13 +9065,11 @@ static const struct genl_small_ops devlink_nl_ops[] = { .cmd = DEVLINK_CMD_PORT_NEW, .doit = devlink_nl_cmd_port_new_doit, .flags = GENL_ADMIN_PERM, - .internal_flags = DEVLINK_NL_FLAG_NO_LOCK, }, { .cmd = DEVLINK_CMD_PORT_DEL, .doit = devlink_nl_cmd_port_del_doit, .flags = GENL_ADMIN_PERM, - .internal_flags = DEVLINK_NL_FLAG_NO_LOCK, }, { .cmd = DEVLINK_CMD_LINECARD_GET,