Re: [PATCH net v2] mlxsw: pci: Quiesce EQ tasklet and CQ NAPI before teardown

[Myeonghun Pak]

Thanks for the review.

The extra a in truea was accidentally introduced while I was preparing
the patch email after the build test. Sorry about that.

I also understand that this should target net-next rather than
net/stable, and I will drop the stable Cc. Your proposed start/stop
approach also looks cleaner. I will review it carefully and build-test
the next revision before sending it, after allowing sufficient time
for further feedback.

2026년 7월 22일 (수) 오후 10:27, Petr Machata <petrm@xxxxxxxxxx>님이 작성:
>
>
> Myeonghun Pak <mhun512@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> > v2:
> > - Fix typo in napi_enabled assignment reported by kernel test robot.
> >
> > v1: https://lore.kernel.org/r/20260721062105.55014-1-mhun512@xxxxxxxxx/
> >
> > 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@xxxxxxxxxx>
> From: Petr Machata <petrm@xxxxxxxxxx>
> Date: Wed, 22 Jul 2026 14:49:30 +0200
> Subject: [PATCH net-next] mlxsw: pci: Le fix [xxx]
> To: <netdev@xxxxxxxxxxxxxxx>
> Cc: <mlxsw@xxxxxxxxxx>
>
> Signed-off-by: Petr Machata <petrm@xxxxxxxxxx>
> ---
>  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