* Re: [PATCH net v2] mlxsw: pci: Quiesce EQ tasklet and CQ NAPI before teardown
2026-07-21 12:57 [PATCH net v2] mlxsw: pci: Quiesce EQ tasklet and CQ NAPI before teardown Myeonghun Pak
2026-07-22 9:20 ` Petr Machata
@ 2026-07-22 10:07 ` Petr Machata
1 sibling, 0 replies; 3+ messages in thread
From: Petr Machata @ 2026-07-22 10:07 UTC (permalink / raw)
To: Myeonghun Pak
Cc: Ido Schimmel, Petr Machata, Andrew Lunn, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, kernel test robot,
netdev, linux-kernel, Ijae Kim
Myeonghun Pak <mhun512@gmail.com> writes:
> mlxsw_pci_eq_irq_handler() schedules the EQ tasklet. The tasklet reads
> the EQ ring and schedules CQ NAPI instances. The CQ poll callbacks, in
> turn, dereference the RDQ or SDQ associated with the CQ.
>
> mlxsw_pci_fini() unregisters the IRQ and immediately tears down the
> asynchronous queues in RDQ, SDQ, CQ, EQ order. free_irq() waits for IRQ
> handlers, but not for a tasklet already scheduled by one. In addition,
> mlxsw_pci_cq_fini() disables each CQ NAPI only after all RDQs and SDQs
> have been freed. A pending tasklet or NAPI poll can therefore access
> freed queue storage.
>
> Kill the EQ tasklet after free_irq() so it cannot schedule any more CQ
> NAPI instances. Disable all CQ NAPI instances before freeing the first
> descriptor queue, ensuring their poll callbacks have completed. Track
> the enabled state per CQ to avoid disabling a NAPI instance twice when
> the CQ is later destroyed, while preserving the partial initialization
> unwind.
>
> Fixes: eda6500a987a ("mlxsw: Add PCI bus implementation")
> Cc: stable@vger.kernel.org
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> v2:
> - Fix typo in napi_enabled assignment reported by kernel test robot.
>
> v1: https://lore.kernel.org/r/20260721062105.55014-1-mhun512@gmail.com/
>
> Found by static analysis on v7.2-rc2; not tested on hardware.
>
> drivers/net/ethernet/mellanox/mlxsw/pci.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> index 0da85d36647d..feeb32134d2a 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> @@ -86,6 +86,7 @@ struct mlxsw_pci_queue {
> enum mlxsw_pci_cqe_v v;
> struct mlxsw_pci_queue *dq;
> struct napi_struct napi;
> + bool napi_enabled;
> struct page_pool *page_pool;
> } cq;
> struct {
> @@ -989,6 +990,15 @@ static void mlxsw_pci_cq_napi_teardown(struct mlxsw_pci_queue *q)
> netif_napi_del(&q->u.cq.napi);
> }
>
> +static void mlxsw_pci_cq_napi_disable(struct mlxsw_pci_queue *q)
> +{
> + if (!q->u.cq.napi_enabled)
> + return;
> +
> + napi_disable(&q->u.cq.napi);
> + q->u.cq.napi_enabled = false;
> +}
> +
> static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q,
> enum mlxsw_pci_cq_type cq_type)
> {
> @@ -1064,6 +1074,7 @@ static int mlxsw_pci_cq_init(struct mlxsw_pci *mlxsw_pci, char *mbox,
> goto err_page_pool_init;
>
> napi_enable(&q->u.cq.napi);
> + q->u.cq.napi_enabled = true;
> mlxsw_pci_queue_doorbell_consumer_ring(mlxsw_pci, q);
> mlxsw_pci_queue_doorbell_arm_consumer_ring(mlxsw_pci, q);
> return 0;
> @@ -1078,7 +1089,7 @@ static void mlxsw_pci_cq_fini(struct mlxsw_pci *mlxsw_pci,
> {
> enum mlxsw_pci_cq_type cq_type = mlxsw_pci_cq_type(mlxsw_pci, q);
>
> - napi_disable(&q->u.cq.napi);
> + mlxsw_pci_cq_napi_disable(q);
> mlxsw_pci_cq_page_pool_fini(q, cq_type);
> mlxsw_pci_cq_napi_teardown(q);
> mlxsw_cmd_hw2sw_cq(mlxsw_pci->core, q->num);
> @@ -1439,6 +1450,14 @@ err_cqs_init:
>
> static void mlxsw_pci_aqs_fini(struct mlxsw_pci *mlxsw_pci)
> {
> + struct mlxsw_pci_queue_type_group *queue_group;
> + int i;
> +
> + queue_group = mlxsw_pci_queue_type_group_get(mlxsw_pci,
> + MLXSW_PCI_QUEUE_TYPE_CQ);
> + for (i = 0; i < queue_group->count; i++)
> + mlxsw_pci_cq_napi_disable(&queue_group->q[i]);
> +
> mlxsw_pci_queue_group_fini(mlxsw_pci, &mlxsw_pci_rdq_ops);
> mlxsw_pci_queue_group_fini(mlxsw_pci, &mlxsw_pci_sdq_ops);
> mlxsw_pci_queue_group_fini(mlxsw_pci, &mlxsw_pci_cq_ops);
> @@ -2089,6 +2108,7 @@ static void mlxsw_pci_fini(void *bus_priv)
> struct mlxsw_pci *mlxsw_pci = bus_priv;
>
> free_irq(pci_irq_vector(mlxsw_pci->pdev, 0), mlxsw_pci);
> + tasklet_kill(&mlxsw_pci_eq_get(mlxsw_pci)->u.eq.tasklet);
> mlxsw_pci_aqs_fini(mlxsw_pci);
> mlxsw_pci_napi_devs_fini(mlxsw_pci);
> mlxsw_pci_fw_area_fini(mlxsw_pci);
Hm, my first instinct is to just reorder stuff so that it's correct, but
this is not just init / fini ordering issue. The init ordering between
CQ and EQ is the way it is because CQ refers to EQ, it should be like
this. Fini then needs to be the other way around.
The processes have the opposite requirements though: tasklet refers to
NAPI refer to queues, so it's EQ to CQ to queues.
I think the fix needs to be done differently though. The patch has
tasklet_kill() invoked from overall PCI fini, while the corresponding
tasklet_setup() is in mlxsw_pci_eq_init(), the EQ-specific init
callback. Likewise the patch touches the NAPIs directly in aqs fini,
even though they are CQ-specific. Then the napi_enabled flag seems to
just be there to prevent repeated calls to napi_disable(). I mean, just
don't call it the second time?
So how about something like the below patch instead?
The idea is that since we can't relax the order in which HW queues are
initialized, we need a different queue-specific entry points that we can
invoke to specifically initialize the SW processes. By ordering them
correcly (and not forgetting a tasklet_kill()) we guarantee that NAPI is
created after the queues, but torn down before them, and tasklet is
created after NAPI, but torn down before it.
I took it for a light test and it didn't catastrophically explode.
From 116f63a959bcf9d658302605708e449588da8a69 Mon Sep 17 00:00:00 2001
Message-ID: <116f63a959bcf9d658302605708e449588da8a69.1784725020.git.petrm@nvidia.com>
From: Petr Machata <petrm@nvidia.com>
Date: Wed, 22 Jul 2026 14:49:30 +0200
Subject: [PATCH net-next] mlxsw: pci: Le fix [xxx]
To: <netdev@vger.kernel.org>
Cc: <mlxsw@nvidia.com>
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlxsw/pci.c | 63 +++++++++++++++++++++--
1 file changed, 60 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
index 0da85d36647d..30111ef16443 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
@@ -1063,7 +1063,6 @@ static int mlxsw_pci_cq_init(struct mlxsw_pci *mlxsw_pci, char *mbox,
if (err)
goto err_page_pool_init;
- napi_enable(&q->u.cq.napi);
mlxsw_pci_queue_doorbell_consumer_ring(mlxsw_pci, q);
mlxsw_pci_queue_doorbell_arm_consumer_ring(mlxsw_pci, q);
return 0;
@@ -1078,12 +1077,23 @@ static void mlxsw_pci_cq_fini(struct mlxsw_pci *mlxsw_pci,
{
enum mlxsw_pci_cq_type cq_type = mlxsw_pci_cq_type(mlxsw_pci, q);
- napi_disable(&q->u.cq.napi);
mlxsw_pci_cq_page_pool_fini(q, cq_type);
mlxsw_pci_cq_napi_teardown(q);
mlxsw_cmd_hw2sw_cq(mlxsw_pci->core, q->num);
}
+static void mlxsw_pci_cq_start(struct mlxsw_pci *mlxsw_pci,
+ struct mlxsw_pci_queue *q)
+{
+ napi_enable(&q->u.cq.napi);
+}
+
+static void mlxsw_pci_cq_stop(struct mlxsw_pci *mlxsw_pci,
+ struct mlxsw_pci_queue *q)
+{
+ napi_disable(&q->u.cq.napi);
+}
+
static u16 mlxsw_pci_cq_elem_count(const struct mlxsw_pci_queue *q)
{
return q->u.cq.v == MLXSW_PCI_CQE_V2 ? MLXSW_PCI_CQE2_COUNT :
@@ -1177,7 +1187,6 @@ static int mlxsw_pci_eq_init(struct mlxsw_pci *mlxsw_pci, char *mbox,
err = mlxsw_cmd_sw2hw_eq(mlxsw_pci->core, mbox, q->num);
if (err)
return err;
- tasklet_setup(&q->u.eq.tasklet, mlxsw_pci_eq_tasklet);
mlxsw_pci_queue_doorbell_consumer_ring(mlxsw_pci, q);
mlxsw_pci_queue_doorbell_arm_consumer_ring(mlxsw_pci, q);
return 0;
@@ -1189,6 +1198,18 @@ static void mlxsw_pci_eq_fini(struct mlxsw_pci *mlxsw_pci,
mlxsw_cmd_hw2sw_eq(mlxsw_pci->core, q->num);
}
+static void mlxsw_pci_eq_start(struct mlxsw_pci *mlxsw_pci,
+ struct mlxsw_pci_queue *q)
+{
+ tasklet_setup(&q->u.eq.tasklet, mlxsw_pci_eq_tasklet);
+}
+
+static void mlxsw_pci_eq_stop(struct mlxsw_pci *mlxsw_pci,
+ struct mlxsw_pci_queue *q)
+{
+ tasklet_kill(&q->u.eq.tasklet);
+}
+
struct mlxsw_pci_queue_ops {
const char *name;
enum mlxsw_pci_queue_type type;
@@ -1198,6 +1219,10 @@ struct mlxsw_pci_queue_ops {
struct mlxsw_pci_queue *q);
void (*fini)(struct mlxsw_pci *mlxsw_pci,
struct mlxsw_pci_queue *q);
+ void (*start)(struct mlxsw_pci *mlxsw_pci,
+ struct mlxsw_pci_queue *q);
+ void (*stop)(struct mlxsw_pci *mlxsw_pci,
+ struct mlxsw_pci_queue *q);
u16 (*elem_count_f)(const struct mlxsw_pci_queue *q);
u8 (*elem_size_f)(const struct mlxsw_pci_queue *q);
u16 elem_count;
@@ -1225,6 +1250,8 @@ static const struct mlxsw_pci_queue_ops mlxsw_pci_cq_ops = {
.pre_init = mlxsw_pci_cq_pre_init,
.init = mlxsw_pci_cq_init,
.fini = mlxsw_pci_cq_fini,
+ .start = mlxsw_pci_cq_start,
+ .stop = mlxsw_pci_cq_stop,
.elem_count_f = mlxsw_pci_cq_elem_count,
.elem_size_f = mlxsw_pci_cq_elem_size
};
@@ -1233,6 +1260,8 @@ static const struct mlxsw_pci_queue_ops mlxsw_pci_eq_ops = {
.type = MLXSW_PCI_QUEUE_TYPE_EQ,
.init = mlxsw_pci_eq_init,
.fini = mlxsw_pci_eq_fini,
+ .start = mlxsw_pci_eq_start,
+ .stop = mlxsw_pci_eq_stop,
.elem_count = MLXSW_PCI_EQE_COUNT,
.elem_size = MLXSW_PCI_EQE_SIZE
};
@@ -1349,6 +1378,28 @@ static void mlxsw_pci_queue_group_fini(struct mlxsw_pci *mlxsw_pci,
kfree(queue_group->q);
}
+static void mlxsw_pci_queue_group_start(struct mlxsw_pci *mlxsw_pci,
+ const struct mlxsw_pci_queue_ops *q_ops)
+{
+ struct mlxsw_pci_queue_type_group *queue_group;
+ int i;
+
+ queue_group = mlxsw_pci_queue_type_group_get(mlxsw_pci, q_ops->type);
+ for (i = 0; i < queue_group->count; i++)
+ q_ops->start(mlxsw_pci, &queue_group->q[i]);
+}
+
+static void mlxsw_pci_queue_group_stop(struct mlxsw_pci *mlxsw_pci,
+ const struct mlxsw_pci_queue_ops *q_ops)
+{
+ struct mlxsw_pci_queue_type_group *queue_group;
+ int i;
+
+ queue_group = mlxsw_pci_queue_type_group_get(mlxsw_pci, q_ops->type);
+ for (i = 0; i < queue_group->count; i++)
+ q_ops->stop(mlxsw_pci, &queue_group->q[i]);
+}
+
static int mlxsw_pci_aqs_init(struct mlxsw_pci *mlxsw_pci, char *mbox)
{
struct pci_dev *pdev = mlxsw_pci->pdev;
@@ -1426,6 +1477,9 @@ static int mlxsw_pci_aqs_init(struct mlxsw_pci *mlxsw_pci, char *mbox)
goto err_rdqs_init;
}
+ mlxsw_pci_queue_group_start(mlxsw_pci, &mlxsw_pci_cq_ops);
+ mlxsw_pci_queue_group_start(mlxsw_pci, &mlxsw_pci_eq_ops);
+
return 0;
err_rdqs_init:
@@ -1439,6 +1493,9 @@ static int mlxsw_pci_aqs_init(struct mlxsw_pci *mlxsw_pci, char *mbox)
static void mlxsw_pci_aqs_fini(struct mlxsw_pci *mlxsw_pci)
{
+ mlxsw_pci_queue_group_stop(mlxsw_pci, &mlxsw_pci_eq_ops);
+ mlxsw_pci_queue_group_stop(mlxsw_pci, &mlxsw_pci_cq_ops);
+
mlxsw_pci_queue_group_fini(mlxsw_pci, &mlxsw_pci_rdq_ops);
mlxsw_pci_queue_group_fini(mlxsw_pci, &mlxsw_pci_sdq_ops);
mlxsw_pci_queue_group_fini(mlxsw_pci, &mlxsw_pci_cq_ops);
--
2.54.0
^ permalink raw reply related [flat|nested] 3+ messages in thread