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

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

[Björn Töpel]

alloc_etherdev_mq() initializes the real queue counts to the
allocation maximum. fbnic selects smaller default queue counts and can
change its private queue counts while the device is down, but it does
not publish either value until the next open.

Resume also rebuilds the queue layout. It independently caps the RX
and TX ring counts at the number of available NAPI vectors, then sets
the NAPI count to the larger ring count. This happens even when the
number of available vectors has not changed. Combined configurations
with more rings than vectors lose rings, while separate RX and TX
channels are paired into combined channels where possible.

These mismatches can expose an inactive queue for memory-provider
binding or hide an existing binding. A later channel expansion can
then allocate the queue without recognizing its memory provider.

Publish the default and offline channel counts immediately.

Keep the configured queue and NAPI counts across resume. Suspend
clears num_napi when it frees the vectors, so track the configured
count separately. If resume gets fewer vectors than the configured
layout needs, fail with -ENOSPC rather than silently change the queue
configuration and its bindings. The netdev then stays detached until a
later recovery or a device reprobe.

Fixes: da43127a8edc ("eth: fbnic: support queue ops / zero-copy Rx")
Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
Link: https://lore.kernel.org/netdev/178915061000.219967.7726187707862333281@kernel.org/
Signed-off-by: Björn Töpel <bjorn@kernel.org>

---
Hmm, this was trickier than expected...

The policy when resume gets fewer MSI-X vectors than the configured
layout needs is not obvious. This version fails resume, which keeps
the queues and their bindings intact, but leaves the netdev detached
until a later recovery or a device reprobe.

Is it intended that fbnic_set_channels() accepts combined_count above
the number of available IRQs? It spreads the queues over fewer NAPIs,
and fbnic_get_channels() reports the ring count as combined channels.

If so, resume could keep combined layouts and convert mismatched rx
and tx layouts to combined channels instead of failing, so no rx queue
would have to be removed.

If not, set_channels needs fixing as well. Trimming queues on resume
would also have to repeat the core's channel-shrink checks for memory
providers, RSS indirection tables and ntuple rules, and account for
queue leases, but the helpers behind those checks are not exported.

Then again... I don't know how common the "resume and fewer IRQs"
scenario is. Maybe this approach is good enough?

Changes in v2:
- Keep the configured NAPI count in num_napi_cfg and restore it on
  resume. v1 recomputed it from the queue counts, which changed
  dedicated rx and tx layouts even when the vector count was
  unchanged. (Breno, Sashiko)
- Fail resume with -ENOSPC when fewer vectors come back than the
  configured layout needs. v1 kept the queue counts on fewer vectors,
  which could report more channels than NAPIs, in a layout that
  fbnic_set_channels() rejects. (Sashiko)
- Fix the comment and commit message that did not match the code.
  (Breno, Sashiko)
- Keep the RSS indirection reset in fbnic_netdev_alloc().
- Drop Breno's Reviewed-by, since the resume path changed.

v1: https://lore.kernel.org/netdev/20260915180859.4157646-1-bjorn@kernel.org/
---
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   | 16 +++++++++++---
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    | 22 ++++++++++++-------
 .../net/ethernet/meta/fbnic/fbnic_netdev.h    |  4 ++--
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   | 14 ++++++++++--
 4 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 76e9a545bb16..6be7ab0faa80 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -189,6 +189,7 @@ static void fbnic_clone_swap_cfg(struct fbnic_net *orig,
 	swap(clone->num_rx_queues, orig->num_rx_queues);
 	swap(clone->num_tx_queues, orig->num_tx_queues);
 	swap(clone->num_napi, orig->num_napi);
+	swap(clone->num_napi_cfg, orig->num_napi_cfg);
 	swap(clone->hds_thresh, orig->hds_thresh);
 }
 
@@ -1586,13 +1587,14 @@ static void fbnic_get_channels(struct net_device *netdev,
 	ch->max_combined = min(ch->max_rx, ch->max_tx);
 	ch->max_other =	FBNIC_NON_NAPI_VECTORS;
 
-	if (fbn->num_rx_queues > fbn->num_napi ||
-	    fbn->num_tx_queues > fbn->num_napi)
+	if (fbn->num_rx_queues > fbn->num_napi_cfg ||
+	    fbn->num_tx_queues > fbn->num_napi_cfg)
 		ch->combined_count = min(fbn->num_rx_queues,
 					 fbn->num_tx_queues);
 	else
 		ch->combined_count =
-			fbn->num_rx_queues + fbn->num_tx_queues - fbn->num_napi;
+			fbn->num_rx_queues + fbn->num_tx_queues -
+			fbn->num_napi_cfg;
 	ch->rx_count = fbn->num_rx_queues - ch->combined_count;
 	ch->tx_count = fbn->num_tx_queues - ch->combined_count;
 	ch->other_count = FBNIC_NON_NAPI_VECTORS;
@@ -1605,6 +1607,7 @@ static void fbnic_set_queues(struct fbnic_net *fbn, struct ethtool_channels *ch,
 	fbn->num_tx_queues = ch->tx_count + ch->combined_count;
 	fbn->num_napi = min(ch->rx_count + ch->tx_count + ch->combined_count,
 			    max_napis);
+	fbn->num_napi_cfg = fbn->num_napi;
 }
 
 static int fbnic_set_channels(struct net_device *netdev,
@@ -1631,6 +1634,13 @@ static int fbnic_set_channels(struct net_device *netdev,
 		return -EINVAL;
 
 	if (!netif_running(netdev)) {
+		unsigned int rxq = ch->rx_count + ch->combined_count;
+		unsigned int txq = ch->tx_count + ch->combined_count;
+
+		err = netif_set_real_num_queues(netdev, txq, rxq);
+		if (err)
+			return err;
+
 		fbnic_set_queues(fbn, ch, max_napis);
 		fbnic_reset_indir_tbl(fbn);
 		return 0;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index 10bf99be3f24..895add19131b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -702,8 +702,8 @@ static const struct netdev_stat_ops fbnic_stat_ops = {
 	.get_base_stats		= fbnic_get_base_stats,
 };
 
-void fbnic_reset_queues(struct fbnic_net *fbn,
-			unsigned int tx, unsigned int rx)
+static int fbnic_reset_queues(struct fbnic_net *fbn,
+			      unsigned int tx, unsigned int rx)
 {
 	struct fbnic_dev *fbd = fbn->fbd;
 	unsigned int max_napis;
@@ -717,6 +717,9 @@ void fbnic_reset_queues(struct fbnic_net *fbn,
 	fbn->num_rx_queues = rx;
 
 	fbn->num_napi = max(tx, rx);
+	fbn->num_napi_cfg = fbn->num_napi;
+
+	return netif_set_real_num_queues(fbn->netdev, tx, rx);
 }
 
 /**
@@ -785,7 +788,8 @@ struct net_device *fbnic_netdev_alloc(struct fbnic_dev *fbd)
 	if (default_queues > fbd->max_num_queues)
 		default_queues = fbd->max_num_queues;
 
-	fbnic_reset_queues(fbn, default_queues, default_queues);
+	if (fbnic_reset_queues(fbn, default_queues, default_queues))
+		goto err_free_netdev;
 
 	fbnic_reset_indir_tbl(fbn);
 	fbnic_rss_key_fill(fbn->rss_key);
@@ -832,13 +836,15 @@ struct net_device *fbnic_netdev_alloc(struct fbnic_dev *fbd)
 
 	netif_tx_stop_all_queues(netdev);
 
-	if (fbnic_phylink_create(netdev)) {
-		free_netdev(netdev);
-		fbd->netdev = NULL;
-		return NULL;
-	}
+	if (fbnic_phylink_create(netdev))
+		goto err_free_netdev;
 
 	return netdev;
+
+err_free_netdev:
+	free_netdev(netdev);
+	fbd->netdev = NULL;
+	return NULL;
 }
 
 static int fbnic_dsn_to_mac_addr(u64 dsn, char *addr)
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
index eded20b0e9e4..89944c1bdaa4 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
@@ -41,6 +41,8 @@ struct fbnic_net {
 	u32 rx_max_frames;
 
 	u16 num_napi;
+	/* Configured count, retained while suspend clears num_napi */
+	u16 num_napi_cfg;
 
 	struct phylink *phylink;
 	struct phylink_config phylink_config;
@@ -86,8 +88,6 @@ struct net_device *fbnic_netdev_alloc(struct fbnic_dev *fbd);
 void fbnic_netdev_free(struct fbnic_dev *fbd);
 int fbnic_netdev_register(struct net_device *netdev);
 void fbnic_netdev_unregister(struct net_device *netdev);
-void fbnic_reset_queues(struct fbnic_net *fbn,
-			unsigned int tx, unsigned int rx);
 
 void fbnic_set_ethtool_ops(struct net_device *dev);
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index c6698e3002a1..48279a5214c8 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -479,6 +479,7 @@ static int __fbnic_pm_resume(struct device *dev)
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
 	struct net_device *netdev = fbd->netdev;
 	void __iomem * const *iomap_table;
+	unsigned int max_napis;
 	struct fbnic_net *fbn;
 	int err;
 
@@ -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;
+	}
+
+	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)

base-commit: fc6d80eb504458d6416b75a94188b268c95c6533
-- 
2.55.0

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

[Breno Leitao]

Hello Björn,

On Thu, Sep 24, 2026 at 09:18:15PM +0200, Björn Töpel wrote:
> If resume gets fewer vectors than the configured
> layout needs, fail with -ENOSPC rather than silently change the queue
> configuration and its bindings. The netdev then stays detached until a
> later recovery or a device reprobe.

I am a bit surprised that I is better to get the device permanetly
disconnected other than just getting it running with fewer queues on
resume.

What is reprobing it later?
> Fixes: da43127a8edc ("eth: fbnic: support queue ops / zero-copy Rx")
> Reported-by: Sashiko <netdev-bot+sashiko@kernel.org> Link:
> https://lore.kernel.org/netdev/178915061000.219967.7726187707862333281@kernel.org/
> Signed-off-by: Björn Töpel <bjorn@kernel.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

PS: The question above shouldn't block the merge of the patch, it is
just for my own curiosity.

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

[Björn Töpel]

Breno!

Thanks for having a look!

Breno Leitao <leitao@debian.org> writes:

> Hello Björn,
>
> On Thu, Sep 24, 2026 at 09:18:15PM +0200, Björn Töpel wrote:
>> If resume gets fewer vectors than the configured
>> layout needs, fail with -ENOSPC rather than silently change the queue
>> configuration and its bindings. The netdev then stays detached until a
>> later recovery or a device reprobe.
>
> I am a bit surprised that I is better to get the device permanetly
> disconnected other than just getting it running with fewer queues on
> resume.

What's the queue selection policy?

 - Memory providers and AF_XDP                                                                          
 - RSS indirection and contexts                                                                         
 - ntuple rules                                                                                         
 - Queue leases                                                                                         

> What is reprobing it later?

Nothing does it automatically. The netdev stays detached until a later
suspend/resume gets enough vectors, or until someone rebinds the driver.
A rebind resets the channels and drops the memory-provider bindings
anyway *shrug*.

This only happens when resume gets fewer MSI-X vectors than the
configured layout uses, for example when CPUs were offlined after the
channels were set.

...*is* this a real issue, or mostly theoretical?


Enjoy the weekend!
Björn

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

[netdev-bot+sashiko]

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