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

[Daniel Zahka]

On Sun Sep 27, 2026 at 8:01 PM EDT, netdev-bot+sashiko wrote:
> 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.
>

For these two reasons (second call idempotent, msix auto enrollment in
devrez slated for removal), I think it is ok to keep as is. Let me know if
that is a problem.

> 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);
>>  }