Re: [PATCH net-next v2 3/8] eth: mpnic: allocate MSI-X vectors

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The driver frees its MSI-X vectors both by hand and through
  devres.

--

eth: mpnic: allocate MSI-X vectors

This commit adds mpnic_irq.c, which allocates one MSI-X vector for FW
interrupts and one vector per online CPU for the completion queues. The
allocation happens in mpnic_probe(). The vectors are freed on the probe
error path and in mpnic_remove().

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_irq.c b/drivers/net/ethernet/meta/mpnic/mpnic_irq.c
> new file mode 100644
> index 0000000000000..bcc33655cbbea
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_irq.c

[ ... ]

> +void mpnic_free_irqs(struct mpnic_dev *mpd)
> +{
> +	struct pci_dev *pdev = to_pci_dev(mpd->dev);
> +
> +	mpd->num_irqs = 0;
> +	pci_free_irq_vectors(pdev);
> +}

[Severity: Low]
Is it safe to call pci_free_irq_vectors() here when mpnic_probe() enables
the device with pcim_enable_device()?

pcim_enable_device() sets pdev->is_managed. Because of that,
pci_alloc_irq_vectors() in mpnic_alloc_irqs() ends up in
pcim_setup_msi_release():

drivers/pci/msi/msi.c:pcim_setup_msi_release() {
    ...
	if (!pci_is_managed(dev) || dev->is_msi_managed)
		return 0;

	ret = devm_add_action(&dev->dev, pcim_msi_release, dev);
    ...
}

The kernel-doc for pci_free_irq_vectors() says not to do this:

 * WARNING: Do not call this function if the device has been enabled
 * with pcim_enable_device(). In that case, IRQ vectors are automatically
 * managed via pcim_msi_release() and calling pci_free_irq_vectors() can
 * lead to double-free issues.

With this patch, both the err_free_irqs path in mpnic_probe() and
mpnic_remove() call mpnic_free_irqs()->pci_free_irq_vectors(). Devres
then runs pcim_msi_release()->pci_free_irq_vectors() a second time.

Today the second call does nothing, because pci_disable_msix() returns
early when !dev->msix_enabled. That makes it an internal guard, though,
not part of the API contract. The TODO in pcim_setup_msi_release() also
says the implicit managed mode is slated for removal.

The pattern is still present at the end of the series.

Could the driver pick one owner for the vectors? It could drop the manual
pci_free_irq_vectors() and let devres free them. Or it could stop relying
on the implicit managed mode, so the explicit free is the only one.

> +
> +int mpnic_alloc_irqs(struct mpnic_dev *mpd)
> +{

[ ... ]

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> index 68a64377e6c5a..127b71b44b0fb 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> @@ -112,12 +112,18 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

[ ... ]

> +err_free_irqs:
> +	mpnic_free_irqs(mpd);
>  err_free_mpd:
>  	kfree(mpd);
>  
> @@ -132,6 +138,7 @@ static void mpnic_remove(struct pci_dev *pdev)
>  {
>  	struct mpnic_dev *mpd = pci_get_drvdata(pdev);
>  
> +	mpnic_free_irqs(mpd);
>  	kfree(mpd);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-linux-mpnic-v2-0-4badc9b58b9e%40gmail.com