Re: [PATCH net-next v3 2/3] dinghai: add MSI-X interrupt pools

[han.junyang]

On Mon, Sep 21, 2026 at 02:59:46PM +0800, han.junyang@xxxxxxxxxx wrote:
> > From: Junyang Han <han.junyang@xxxxxxxxxx>
> >
> > Allocate the fixed MSI-X vector layout of the device and manage the
> > vectors in per-purpose pools: the async event queues first, a range
> > reserved for RDMA in between and the vq queue pairs last. This series
> > wires up the async pool; the vq pool comes with the netdev series.
> >
> > IRQs are reference counted so that several event queues can share one
> > vector. The pool hands out the least loaded matching IRQ once it passes
> > the pool minimum threshold, and binds newly created IRQs to the least
> > loaded CPU of the requested affinity mask. Interrupt delivery fans out
> > through an atomic notifier chain attached to each IRQ, which the async
> > event queue setup posted later in this series hooks into.
> >
> > Signed-off-by: Junyang Han <han.junyang@xxxxxxxxxx>

...

> > @@ -516,10 +604,24 @@ static int zxdh_pf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > goto err_modern_cfg;
> > }
> >
> > + ret = zxdh_pf_irq_table_init(zxdh_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "zxdh_pf_irq_table_init failed: %d\n", ret);
> > + goto err_modern_cfg;
> > + }
> > +
> > + ret = zxdh_pf_irq_table_create(zxdh_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "zxdh_pf_irq_table_create failed: %d\n", ret);
> > + goto err_irq_table;
> > + }

> I am wondering if you considered calling zxdh_pf_irq_table_create()
> from zxdh_pf_irq_table_init(). And likewise for
> zxdh_pf_eq_table_init()/zxdh_pf_eq_table_create() in patch 3/3.

> I mainly ask because it seems like it would simplify zxdh_pf_probe()
> slightly. But I don't feel strongly about this.

Both taken in v4: the create() steps are folded into their init() counterparts,
and every probe error path now goes through the same destroy() functions
remove() uses, so each patch carries one cleanup authority from the start.
This also drops the internal pci_free_irq_vectors() on the pools failure
path - destroy() releases the vectors exactly once for any partial init state.

> > +
> > devlink_register(devlink);
> >
> > return 0;
> >
> > +err_irq_table:
> > + kvfree(zxdh_dev->irq_table.priv);

> As a counter to my previous comment: I see the line is changed to call
> zxdh_pf_irq_table_destroy() in patch 3/3. But I'm wondering if it should be
> (or would be nicer if it was) zxdh_pf_irq_table_destroy() in this patch.

> > err_modern_cfg:
> > zxdh_pf_modern_cfg_uninit(zxdh_dev);
> > err_cfg_init:

...

> > diff --git a/drivers/net/ethernet/zte/dinghai/zxdh_irq.c b/drivers/net/ethernet/zte/dinghai/zxdh_irq.c

...

> > +static struct zxdh_irq *zxdh_irq_alloc(struct zxdh_irq_pool *pool, int vecidx,
> > + const struct cpumask *affinity)
> > +{
> > + struct zxdh_core_dev *zxdh_dev = pool->dev;
> > + struct zxdh_irq *irq;
> > + int err;
> > + int cpu;
> > +
> > + irq = kzalloc_obj(*irq, GFP_KERNEL);
> > + if (!irq)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + irq->pool = pool;
> > + irq->irqn = pci_irq_vector(zxdh_dev->pdev, vecidx);
> > + if (irq->irqn < 0) {
> > + err = irq->irqn;
> > + goto err_irqn;
> > + }
> > +
> > + ATOMIC_INIT_NOTIFIER_HEAD(&irq->nh);
> > + snprintf(irq->name, ZXDH_MAX_IRQ_NAME, "%s_%d@pci:%s", pool->name,
> > + vecidx, pci_name(zxdh_dev->pdev));

> W=1 builds complain about this. E.g. GCC 16.2.0 on x86_64 says:

> CC [M] drivers/net/ethernet/zte/dinghai/zxdh_irq.o
> drivers/net/ethernet/zte/dinghai/zxdh_irq.c: In function 'zxdh_irq_alloc':
> drivers/net/ethernet/zte/dinghai/zxdh_irq.c:94:52: warning: '%d' directive output may be truncated writing between 1 and 11 bytes into a region of size between 0 and 99 [-Wformat-truncation=]
> 94 | snprintf(irq->name, ZXDH_MAX_IRQ_NAME, "%s_%d@pci:%s", pool->name,
> | ^~
> drivers/net/ethernet/zte/dinghai/zxdh_irq.c:94:9: note: 'snprintf' output 8 or more bytes (assuming 107) into a destination of size 100
> 94 | snprintf(irq->name, ZXDH_MAX_IRQ_NAME, "%s_%d@pci:%s", pool->name,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 95 | vecidx, pci_name(zxdh_dev->pdev));
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> ....

Thanks for testing with GCC 16. Fixed in v4: the pool name now has a
buffer bound of its own (16 bytes; the longest pool name is 13) instead
of sharing the 100-byte IRQ name buffer, and the copy uses strscpy().
The series is now built per-patch with W=1 before submission.