From: Mark Bloch The generation counter in mlx5_esw_functions is used to detect stale work items on the E-Switch work queue. Move it from mlx5_esw_functions to the top-level mlx5_eswitch struct so it can guard all work types, not just function-change events. This is a mechanical refactor: no behavioral change. Signed-off-by: Mark Bloch Reviewed-by: Cosmin Ratiu Signed-off-by: Tariq Toukan --- drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 3 ++- drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 2 +- drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c index 123c96716a54..1986d4d0e886 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c @@ -1075,7 +1075,7 @@ static void mlx5_eswitch_event_handler_unregister(struct mlx5_eswitch *esw) if (esw->mode == MLX5_ESWITCH_OFFLOADS && mlx5_eswitch_is_funcs_handler(esw->dev)) { mlx5_eq_notifier_unregister(esw->dev, &esw->esw_funcs.nb); - atomic_inc(&esw->esw_funcs.generation); + atomic_inc(&esw->generation); } } @@ -2072,6 +2072,7 @@ int mlx5_eswitch_init(struct mlx5_core_dev *dev) mutex_init(&esw->state_lock); init_rwsem(&esw->mode_lock); refcount_set(&esw->qos.refcnt, 0); + atomic_set(&esw->generation, 0); esw->enabled_vports = 0; esw->offloads.inline_mode = MLX5_INLINE_MODE_NONE; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h index 5128f5020dae..0c3d2bdebf8c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h @@ -340,7 +340,6 @@ struct mlx5_host_work { struct mlx5_esw_functions { struct mlx5_nb nb; - atomic_t generation; bool host_funcs_disabled; u16 num_vfs; u16 num_ec_vfs; @@ -410,6 +409,7 @@ struct mlx5_eswitch { struct mlx5_devcom_comp_dev *devcom; u16 enabled_ipsec_vf_count; bool eswitch_operation_in_progress; + atomic_t generation; }; void esw_offloads_disable(struct mlx5_eswitch *esw); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index a078d06f4567..b2e7294d3a5c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -3667,7 +3667,7 @@ esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, int work_gen, devl_lock(devlink); /* Stale work from one or more mode changes ago. Bail out. */ - if (work_gen != atomic_read(&esw->esw_funcs.generation)) + if (work_gen != atomic_read(&esw->generation)) goto unlock; new_num_vfs = MLX5_GET(query_esw_functions_out, out, @@ -3729,7 +3729,7 @@ int mlx5_esw_funcs_changed_handler(struct notifier_block *nb, unsigned long type esw = container_of(esw_funcs, struct mlx5_eswitch, esw_funcs); host_work->esw = esw; - host_work->work_gen = atomic_read(&esw_funcs->generation); + host_work->work_gen = atomic_read(&esw->generation); INIT_WORK(&host_work->work, esw_functions_changed_event_handler); queue_work(esw->work_queue, &host_work->work); -- 2.44.0 From: Mark Bloch Each E-Switch work item requires the same boilerplate: acquire the devlink lock, check whether the work is stale, dispatch to the appropriate handler, and release the lock. Factor this out. Add a func callback to mlx5_host_work so the generic handler esw_wq_handler() can dispatch to the right function without duplicating locking logic. Introduce mlx5_esw_add_work() as the single enqueue point: it stamps the work item with the current generation counter and queues it onto the E-Switch work queue. Refactor esw_vfs_changed_event_handler() to match the new contract: it no longer receives work_gen or out as parameters. It queries mlx5_esw_query_functions() itself and owns the kvfree() of the result. The devlink lock is acquired and released by esw_wq_handler() before dispatching, so the handler runs with the lock already held. Update mlx5_esw_funcs_changed_handler() to use mlx5_esw_add_work(). Signed-off-by: Mark Bloch Reviewed-by: Cosmin Ratiu Signed-off-by: Tariq Toukan --- .../net/ethernet/mellanox/mlx5/core/eswitch.h | 1 + .../mellanox/mlx5/core/eswitch_offloads.c | 77 +++++++++++-------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h index 0c3d2bdebf8c..e3ab8a30c174 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h @@ -336,6 +336,7 @@ struct mlx5_host_work { struct work_struct work; struct mlx5_eswitch *esw; int work_gen; + void (*func)(struct mlx5_eswitch *esw); }; struct mlx5_esw_functions { diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index b2e7294d3a5c..23af5a12dc07 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -3655,20 +3655,15 @@ static void esw_offloads_steering_cleanup(struct mlx5_eswitch *esw) mutex_destroy(&esw->fdb_table.offloads.vports.lock); } -static void -esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, int work_gen, - const u32 *out) +static void esw_vfs_changed_event_handler(struct mlx5_eswitch *esw) { - struct devlink *devlink; bool host_pf_disabled; u16 new_num_vfs; + const u32 *out; - devlink = priv_to_devlink(esw->dev); - devl_lock(devlink); - - /* Stale work from one or more mode changes ago. Bail out. */ - if (work_gen != atomic_read(&esw->generation)) - goto unlock; + out = mlx5_esw_query_functions(esw->dev); + if (IS_ERR(out)) + return; new_num_vfs = MLX5_GET(query_esw_functions_out, out, host_params_context.host_num_of_vfs); @@ -3676,7 +3671,7 @@ esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, int work_gen, host_params_context.host_pf_disabled); if (new_num_vfs == esw->esw_funcs.num_vfs || host_pf_disabled) - goto unlock; + goto free; /* Number of VFs can only change from "0 to x" or "x to 0". */ if (esw->esw_funcs.num_vfs > 0) { @@ -3686,54 +3681,70 @@ esw_vfs_changed_event_handler(struct mlx5_eswitch *esw, int work_gen, err = mlx5_eswitch_load_vf_vports(esw, new_num_vfs, MLX5_VPORT_UC_ADDR_CHANGE); - if (err) { - devl_unlock(devlink); - return; - } + if (err) + goto free; } esw->esw_funcs.num_vfs = new_num_vfs; -unlock: - devl_unlock(devlink); +free: + kvfree(out); } -static void esw_functions_changed_event_handler(struct work_struct *work) +static void esw_wq_handler(struct work_struct *work) { struct mlx5_host_work *host_work; struct mlx5_eswitch *esw; - const u32 *out; + struct devlink *devlink; host_work = container_of(work, struct mlx5_host_work, work); esw = host_work->esw; + devlink = priv_to_devlink(esw->dev); - out = mlx5_esw_query_functions(esw->dev); - if (IS_ERR(out)) - goto out; + devl_lock(devlink); - esw_vfs_changed_event_handler(esw, host_work->work_gen, out); - kvfree(out); -out: + /* Stale work from one or more mode changes ago. Bail out. */ + if (host_work->work_gen != atomic_read(&esw->generation)) + goto unlock; + + host_work->func(esw); + +unlock: + devl_unlock(devlink); kfree(host_work); } -int mlx5_esw_funcs_changed_handler(struct notifier_block *nb, unsigned long type, void *data) +static int mlx5_esw_add_work(struct mlx5_eswitch *esw, + void (*func)(struct mlx5_eswitch *esw)) { - struct mlx5_esw_functions *esw_funcs; struct mlx5_host_work *host_work; - struct mlx5_eswitch *esw; host_work = kzalloc_obj(*host_work, GFP_ATOMIC); if (!host_work) - return NOTIFY_DONE; - - esw_funcs = mlx5_nb_cof(nb, struct mlx5_esw_functions, nb); - esw = container_of(esw_funcs, struct mlx5_eswitch, esw_funcs); + return -ENOMEM; host_work->esw = esw; host_work->work_gen = atomic_read(&esw->generation); - INIT_WORK(&host_work->work, esw_functions_changed_event_handler); + host_work->func = func; + INIT_WORK(&host_work->work, esw_wq_handler); queue_work(esw->work_queue, &host_work->work); + return 0; +} + +int mlx5_esw_funcs_changed_handler(struct notifier_block *nb, + unsigned long type, void *data) +{ + struct mlx5_esw_functions *esw_funcs; + struct mlx5_eswitch *esw; + int ret; + + esw_funcs = mlx5_nb_cof(nb, struct mlx5_esw_functions, nb); + esw = container_of(esw_funcs, struct mlx5_eswitch, esw_funcs); + + ret = mlx5_esw_add_work(esw, esw_vfs_changed_event_handler); + if (ret) + return NOTIFY_DONE; + return NOTIFY_OK; } -- 2.44.0 From: Mark Bloch mlx5_eswitch_cleanup() calls destroy_workqueue() while holding the devlink lock through mlx5_uninit_one(). E-Switch workqueue workers also need the devlink lock, but previously took it before checking whether their work item was stale. This can deadlock when cleanup waits for a worker that is blocked on the same devlink lock. Mode changes have the same ordering hazard: the mode-change path holds devlink lock while tearing down the current mode, and old work may still be pending on the E-Switch workqueue. Fix this by making esw_wq_handler() check the generation counter before attempting to take devlink lock. The worker uses devl_trylock(); if the lock is busy and the work is still current, it sleeps on an E-Switch wait queue with a short timeout. Invalidation increments the generation counter and wakes the wait queue, so stale workers exit without spinning or blocking cleanup. Invalidate work at the earliest safe operation boundary. Cleanup invalidates before destroy_workqueue(), and QoS cleanup runs after the workqueue is destroyed. Mode teardown unregisters the work-producing notifiers first, then invalidates the queue before tearing down FDB/QoS/rate-node state. This prevents new notifier work from capturing the new generation while still making old work stale before expensive teardown starts. mlx5_devlink_eswitch_mode_set() now relies on mlx5_eswitch_disable_locked() for the mode-change invalidation instead of incrementing the generation after disable. mlx5_eswitch_disable() gets the same coverage. SR-IOV enable/disable paths invalidate before VF state changes so work against the old VF count or mode is discarded. Remove the conditional generation increment in mlx5_eswitch_event_handler_unregister(); mlx5_eswitch_disable_locked() now handles it unconditionally after the relevant notifiers are unregistered. Signed-off-by: Mark Bloch Reviewed-by: Cosmin Ratiu Signed-off-by: Tariq Toukan --- .../net/ethernet/mellanox/mlx5/core/eswitch.c | 19 +++++++++++++---- .../net/ethernet/mellanox/mlx5/core/eswitch.h | 2 ++ .../mellanox/mlx5/core/eswitch_offloads.c | 21 +++++++++++++++++-- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c index 1986d4d0e886..8ec52498be3f 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c @@ -1070,13 +1070,17 @@ static void mlx5_eswitch_event_handler_register(struct mlx5_eswitch *esw) } } +static void mlx5_eswitch_invalidate_wq(struct mlx5_eswitch *esw) +{ + atomic_inc(&esw->generation); + wake_up_all(&esw->work_queue_wait); +} + static void mlx5_eswitch_event_handler_unregister(struct mlx5_eswitch *esw) { if (esw->mode == MLX5_ESWITCH_OFFLOADS && - mlx5_eswitch_is_funcs_handler(esw->dev)) { + mlx5_eswitch_is_funcs_handler(esw->dev)) mlx5_eq_notifier_unregister(esw->dev, &esw->esw_funcs.nb); - atomic_inc(&esw->generation); - } } static void mlx5_eswitch_clear_vf_vports_info(struct mlx5_eswitch *esw) @@ -1701,6 +1705,8 @@ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs) if (toggle_lag) mlx5_lag_disable_change(esw->dev); + mlx5_eswitch_invalidate_wq(esw); + if (!mlx5_esw_is_fdb_created(esw)) { ret = mlx5_eswitch_enable_locked(esw, num_vfs); } else { @@ -1746,6 +1752,8 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw, bool clear_vf) esw->mode == MLX5_ESWITCH_LEGACY ? "LEGACY" : "OFFLOADS", esw->esw_funcs.num_vfs, esw->esw_funcs.num_ec_vfs, esw->enabled_vports); + mlx5_eswitch_invalidate_wq(esw); + if (!mlx5_core_is_ecpf(esw->dev)) { mlx5_eswitch_unload_vf_vports(esw, esw->esw_funcs.num_vfs); if (clear_vf) @@ -1785,6 +1793,7 @@ void mlx5_eswitch_disable_locked(struct mlx5_eswitch *esw) mlx5_eq_notifier_unregister(esw->dev, &esw->nb); mlx5_eswitch_event_handler_unregister(esw); + mlx5_eswitch_invalidate_wq(esw); esw_info(esw->dev, "Disable: mode(%s), nvfs(%d), necvfs(%d), active vports(%d)\n", esw->mode == MLX5_ESWITCH_LEGACY ? "LEGACY" : "OFFLOADS", @@ -2072,6 +2081,7 @@ int mlx5_eswitch_init(struct mlx5_core_dev *dev) mutex_init(&esw->state_lock); init_rwsem(&esw->mode_lock); refcount_set(&esw->qos.refcnt, 0); + init_waitqueue_head(&esw->work_queue_wait); atomic_set(&esw->generation, 0); esw->enabled_vports = 0; @@ -2110,8 +2120,9 @@ void mlx5_eswitch_cleanup(struct mlx5_eswitch *esw) esw_info(esw->dev, "cleanup\n"); - mlx5_esw_qos_cleanup(esw); + mlx5_eswitch_invalidate_wq(esw); destroy_workqueue(esw->work_queue); + mlx5_esw_qos_cleanup(esw); WARN_ON(refcount_read(&esw->qos.refcnt)); mutex_destroy(&esw->state_lock); WARN_ON(!xa_empty(&esw->offloads.vhca_map)); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h index e3ab8a30c174..8f4c47975c58 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -385,6 +386,7 @@ struct mlx5_eswitch { */ struct rw_semaphore mode_lock; atomic64_t user_count; + wait_queue_head_t work_queue_wait; /* Protected with the E-Switch qos domain lock. */ struct { diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index 23af5a12dc07..9e2ae217c224 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -3694,21 +3694,38 @@ static void esw_wq_handler(struct work_struct *work) struct mlx5_host_work *host_work; struct mlx5_eswitch *esw; struct devlink *devlink; + int work_gen; host_work = container_of(work, struct mlx5_host_work, work); esw = host_work->esw; + work_gen = host_work->work_gen; devlink = priv_to_devlink(esw->dev); - devl_lock(devlink); + /* Do not block on devlink lock until stale work is filtered out. + * Teardown can invalidate the generation and then wait for this + * workqueue while holding devlink lock. + */ + for (;;) { + if (work_gen != atomic_read(&esw->generation)) + goto free; + + if (devl_trylock(devlink)) + break; + + wait_event_timeout(esw->work_queue_wait, + work_gen != atomic_read(&esw->generation), + msecs_to_jiffies(60)); + } /* Stale work from one or more mode changes ago. Bail out. */ - if (host_work->work_gen != atomic_read(&esw->generation)) + if (work_gen != atomic_read(&esw->generation)) goto unlock; host_work->func(esw); unlock: devl_unlock(devlink); +free: kfree(host_work); } -- 2.44.0