Re: [PATCH net v2] eth: fbnic: Keep real queue counts synchronized

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds a hard `-ENOSPC` exit to `__fbnic_pm_resume()`
  (drivers/net/ethernet/meta/fbnic/fbnic_pci.c:523-529).

--

eth: fbnic: Keep real queue counts synchronized

The patch publishes the default channel counts, and the counts set while
the device is down, to the core with netif_set_real_num_queues() right
away. It adds num_napi_cfg so the configured NAPI count survives suspend.
Resume now fails with -ENOSPC if fewer vectors come back than that layout
needs.

Link: https://lore.kernel.org/netdev/178915061000.219967.7726187707862333281@kernel.org/

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index c6698e3002a13..48279a5214c8a 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c

[ ... ]

> @@ -519,8 +520,16 @@ static int __fbnic_pm_resume(struct device *dev)
>  
>  	fbn = netdev_priv(netdev);
>  
> -	/* Reset the queues if needed */
> -	fbnic_reset_queues(fbn, fbn->num_tx_queues, fbn->num_rx_queues);
> +	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
> +	if (fbn->num_napi_cfg > max_napis) {
> +		netdev_err(netdev,
> +			   "Unable to restore channel configuration: %u NAPI vectors required, only %u available\n",
> +			   fbn->num_napi_cfg, max_napis);
> +		err = -ENOSPC;
> +		goto unlock;
> +	}

[Severity: High]
Can this leave the NIC unusable after a resume or an AER slot reset that
used to recover?

This patch does not change fbnic_alloc_irqs(). It still sizes the MSI-X
request from the CPUs online at resume time. It also accepts as few as
FBNIC_NON_NAPI_VECTORS + 1 vectors:

drivers/net/ethernet/meta/fbnic/fbnic_irq.c:fbnic_alloc_irqs() {
    ...
	wanted_irqs += min_t(unsigned int, num_online_cpus(), FBNIC_MAX_RXQS);
	num_irqs = pci_alloc_irq_vectors(pdev, FBNIC_NON_NAPI_VECTORS + 1,
					 wanted_irqs, PCI_IRQ_MSIX);
    ...
}

It never asks for num_napi_cfg vectors. Say CPUs are taken offline after
probe or after ethtool -L, for example the default 8 RSS queues with only
4 CPUs online. Wouldn't every resume then hit this check, even when the
hardware could supply enough vectors?

The check also runs before netif_running(). Is it intended that an
interface that is administratively down also fails resume, even though it
doesn't need any NAPI vectors yet?

On failure the code goes unlock -> err_free_mbx -> err_free_irqs ->
err_invalidate_uc_addr. That path disables the FW log and frees the
mailbox and all MSI-X vectors. It also clears uc_addr0 and uc_addr4.
fbnic_pm_resume() then skips __fbnic_pm_attach():

	err = __fbnic_pm_resume(dev);
	if (!err)
		__fbnic_pm_attach(dev);

The netdev stays detached, so netif_device_present() is false. Both
ethnl_ops_begin() and the ethtool ioctl path then return -ENODEV. Doesn't
that stop userspace from shrinking the channels to recover?

fbnic_err_slot_reset() calls the same function:

	err = __fbnic_pm_resume(&pdev->dev);

	return err ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;

So the new -ENOSPC turns a slot reset that used to recover into a
disconnect. Before this patch, fbnic_reset_queues() capped the rx and tx
counts at the available vectors, and both paths succeeded.

For sleep resume, the "later recovery" in the commit message seems to
mean another suspend. fbnic_pm_suspend() would then call ndo_stop() again,
on a netdev that is still marked running but has already been torn down:

	if (netif_running(netdev))
		netdev->netdev_ops->ndo_stop(netdev);

Combined layouts are rejected here too. fbnic_set_channels() already
accepts combined layouts with more rings than NAPIs, through
fbnic_set_queues() doing num_napi = min(..., max_napis). Could resume
spread those rings over fewer vectors instead of failing, so no queue is
dropped?

> +
> +	fbn->num_napi = fbn->num_napi_cfg;
>  
>  	if (netif_running(netdev)) {
>  		err = __fbnic_open(fbn);
> @@ -529,6 +538,7 @@ static int __fbnic_pm_resume(struct device *dev)
>  			fbn->num_napi = 0;
>  	}
>  
> +unlock:
>  	netdev_unlock(netdev);
>  	rtnl_unlock();
>  	if (err)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924191817.1843726-1-bjorn%40kernel.org