[PATCH net v3 0/2] eth: fbnic: Queue and channel count fixes

[PATCH net v3 0/2] eth: fbnic: Queue and channel count fixes

[Björn Töpel]

Two fixes for what fbnic tells the core about its queues.

Patch 1 keeps the channel layout across suspend. Resume rebuilt it
from the queue counts, turning standalone channels into combined ones
or dropping combined queues, even with an unchanged IRQ count.

Patch 2 publishes the real queue counts at probe and on "ethtool -L"
while down, instead of at the next open.

Resume with fewer IRQs than the layout needs now fails instead of
shrinking the layout. That case is left for a separate change.

Changes in v3:
- Split into two patches, one per bug, each blaming the commit that
  introduced it.
- Keep num_napi across suspend instead of a separate num_napi_cfg.
- Drop the -ENOSPC check on resume, so an interface that is down no
  longer fails resume. (Breno, Sashiko)
- Skip ndo_stop when no NAPI vectors are allocated.
- Drop Breno's Reviewed-by, since the resume path changed.

Changes in v2:
- Keep the configured NAPI count 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. (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.

v2: https://lore.kernel.org/netdev/20260924191817.1843726-1-bjorn@kernel.org/
v1: https://lore.kernel.org/netdev/20260915180859.4157646-1-bjorn@kernel.org/

Björn Töpel (2):
  eth: fbnic: Preserve channels across resume
  eth: fbnic: Publish real queue counts

 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  7 ++++
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    | 36 ++++++++++++++-----
 .../net/ethernet/meta/fbnic/fbnic_netdev.h    |  3 --
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   |  9 -----
 4 files changed, 34 insertions(+), 21 deletions(-)


base-commit: 71a77ab76e74131a101f4d2d2afb0dcbf81b4e3c
-- 
2.55.0

[PATCH net v3 1/2] eth: fbnic: Preserve channels across resume

[Björn Töpel]

Suspend clears the configured NAPI count. Resume then reconstructs the
channel layout from queue counts, changing standalone channels into
combined ones or dropping combined queues despite unchanged IRQs.

Keep the selected NAPI count across suspend and reuse the layout on
resume. A failed recovery leaves no live NAPI vectors, so skip a
redundant stop rather than dereferencing them.

If fewer IRQs return, reopening still fails. Handle that separately.

Found by code inspection while reviewing the queue-count fix. The
layout change has not been reproduced on fbnic hardware.

Fixes: 3a481cc72673 ("eth: fbnic: support ring channel get and set while down")
Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 drivers/net/ethernet/meta/fbnic/fbnic_netdev.c | 11 +++++++++--
 drivers/net/ethernet/meta/fbnic/fbnic_netdev.h |  3 ---
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c    |  9 ---------
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index 10bf99be3f24..d1ed29312d54 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -86,6 +86,13 @@ static int fbnic_stop(struct net_device *netdev)
 {
 	struct fbnic_net *fbn = netdev_priv(netdev);
 
+	/* Suspend frees NAPI vectors but keeps num_napi for resume.
+	 * If recovery fails, netif_running() remains set; a later stop
+	 * must not walk the freed vectors again.
+	 */
+	if (!fbn->napi[0])
+		return 0;
+
 	fbnic_mac_free_irq(fbn->fbd);
 	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
 
@@ -702,8 +709,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 void fbnic_reset_queues(struct fbnic_net *fbn,
+			       unsigned int tx, unsigned int rx)
 {
 	struct fbnic_dev *fbd = fbn->fbd;
 	unsigned int max_napis;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
index eded20b0e9e4..ddf1b674737a 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.h
@@ -86,9 +86,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);
 
 int fbnic_ptp_setup(struct fbnic_dev *fbd);
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index c6698e3002a1..ec916c9f7faa 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -434,7 +434,6 @@ static int fbnic_pm_suspend(struct device *dev)
 {
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
 	struct net_device *netdev = fbd->netdev;
-	struct fbnic_net *fbn;
 
 	if (fbnic_init_failure(fbd))
 		goto null_uc_addr;
@@ -442,16 +441,11 @@ static int fbnic_pm_suspend(struct device *dev)
 	rtnl_lock();
 	netdev_lock(netdev);
 
-	fbn = netdev_priv(netdev);
-
 	netif_device_detach(netdev);
 
 	if (netif_running(netdev))
 		netdev->netdev_ops->ndo_stop(netdev);
 
-	/* The IRQs are about to be freed, so drop the napi vector count */
-	fbn->num_napi = 0;
-
 	netdev_unlock(netdev);
 	rtnl_unlock();
 
@@ -519,9 +513,6 @@ 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);
-
 	if (netif_running(netdev)) {
 		err = __fbnic_open(fbn);
 		/* On failure the vectors are freed, so drop the count */
-- 
2.55.0

[PATCH net v3 2/2] eth: fbnic: Publish real queue counts

[Björn Töpel]

fbnic exposes more queues than it configures until open, including
after ethtool -L while down. A memory provider can bind to an
unconfigured queue.

Publish the selected counts at probe and on offline channel changes.

Sashiko flagged the mismatch by code inspection; it was not reproduced
on physical hardware. Tested with fbnic QEMU on Debian sid: offline
ethtool -L changed RX/TX queues from 2 to 1 to 3. Reopening passed
DHCP and ping. Network selftests were not run.

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>
---
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  7 +++++
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    | 29 +++++++++++++------
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 76e9a545bb16..b96354e56517 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -1631,6 +1631,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 d1ed29312d54..1e16e10f3505 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -709,11 +709,12 @@ static const struct netdev_stat_ops fbnic_stat_ops = {
 	.get_base_stats		= fbnic_get_base_stats,
 };
 
-static 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;
+	int err;
 
 	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
 
@@ -724,6 +725,14 @@ static void fbnic_reset_queues(struct fbnic_net *fbn,
 	fbn->num_rx_queues = rx;
 
 	fbn->num_napi = max(tx, rx);
+
+	err = netif_set_real_num_queues(fbn->netdev, tx, rx);
+	if (err)
+		return err;
+
+	fbnic_reset_indir_tbl(fbn);
+
+	return 0;
 }
 
 /**
@@ -792,9 +801,9 @@ 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);
 	fbnic_rss_init_en_mask(fbn);
 
@@ -839,13 +848,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)
-- 
2.55.0

Re: [PATCH net v3 1/2] eth: fbnic: Preserve channels across resume

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 (2) · 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] This patch makes resume fail outright when fewer MSI-X vectors
  come back than before suspend, and the commit message describes that…
- [High] After one failed resume, num_napi stays 0 for good, and the next
  open divides by zero and oopses.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  __fbnic_pm_attach() calls fbnic_up() without holding rtnl or the…
- [Medium] This problem wasn't introduced by this patch, but
  fbnic_service_task_stop() uses cancel_delayed_work(), which does not…

--

eth: fbnic: Preserve channels across resume

This patch keeps fbn->num_napi across suspend instead of clearing it. It
also drops the fbnic_reset_queues() call from __fbnic_pm_resume(), so the
pre-suspend channel layout is reused on resume. fbnic_stop() gets a new
early return for when the NAPI vectors have already been freed.

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

[ ... ]

> @@ -442,16 +441,11 @@ static int fbnic_pm_suspend(struct device *dev)
>  	rtnl_lock();
>  	netdev_lock(netdev);
>  
> -	fbn = netdev_priv(netdev);
> -
>  	netif_device_detach(netdev);
>  
>  	if (netif_running(netdev))
>  		netdev->netdev_ops->ndo_stop(netdev);

[Severity: Medium]
This is a pre-existing issue, but should fbnic_service_task_stop() use
cancel_delayed_work_sync() here?

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task_stop() {
	...
	cancel_delayed_work(&fbd->service_task);
}

fbnic_service_task() takes rtnl_lock(), and at the end it re-arms itself:

	if (netif_running(netdev))
		schedule_delayed_work(&fbd->service_task, HZ);

fbnic_pm_suspend() calls ndo_stop directly, so __LINK_STATE_START stays
set. Suppose an instance is already running and blocked on rtnl_lock()
while suspend holds rtnl. The cancel on this path does not wait for it:

fbnic_stop()->fbnic_down()->fbnic_down_noidle()->fbnic_service_task_stop()

Suspend then drops rtnl and goes on to fbnic_fw_free_mbx(),
fbnic_free_irqs() and clearing uc_addr0/uc_addr4. Can the service task
then run fbnic_get_hw_stats32(), fbnic_fw_check_heartbeat() and
fbnic_health_check() against hardware that is being torn down? After that
it would re-arm every HZ while the device is suspended.

The MMIO accessors and the mailbox TX path have their own guards, so no
concrete crash has been shown. With this patch num_napi now stays
non-zero while napi[] is NULL. fbnic_napi_depletion_check() only runs
when netif_carrier_ok(), and phylink_suspend() turns the carrier off, so
that walk does not seem to be reached.

>  
> -	/* The IRQs are about to be freed, so drop the napi vector count */
> -	fbn->num_napi = 0;
> -
>  	netdev_unlock(netdev);
>  	rtnl_unlock();
>  
> @@ -519,9 +513,6 @@ 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);
> -

[Severity: High]
Wasn't this call the only place where the channel layout was clamped to
the MSI-X vectors actually granted on resume?

Earlier in __fbnic_pm_resume(), fbnic_alloc_irqs() renegotiates
fbd->num_irqs from the current online CPU count. It accepts as few as
FBNIC_NON_NAPI_VECTORS + 1:

	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);
	...
	fbd->num_irqs = num_irqs;

Before this patch, fbnic_reset_queues() clamped tx/rx to max_napis =
fbd->num_irqs - FBNIC_NON_NAPI_VECTORS and set num_napi = max(tx, rx).
The interface came back with fewer queues but still worked.

Fewer vectors can come back if CPUs went offline after probe or the
MSI-X grant is smaller. With this call removed, num_napi can then be
larger than num_irqs - FBNIC_NON_NAPI_VECTORS, which breaks the bound
fbnic_set_channels() enforces. In that case:

__fbnic_open()
  fbnic_alloc_napi_vectors()
    fbnic_alloc_napi_vector()
      fbnic_napi_request_irq()
        fbnic_request_irq()
          pci_irq_vector()   <- -EINVAL for an unallocated MSI-X index

__fbnic_open() then fails, and __fbnic_pm_resume() unwinds through
err_free_mbx. netif_device_attach() is never called, so the NIC stays
unusable until the driver is reloaded.

The AER path also goes through __fbnic_pm_resume(). There,
fbnic_err_slot_reset() now returns PCI_ERS_RESULT_DISCONNECT where it
used to return PCI_ERS_RESULT_RECOVERED.

The commit message says:

    If fewer IRQs return, reopening still fails. Handle that separately.

Is that accurate for the code before this patch? The removed
fbnic_reset_queues() call seems to have handled exactly this case, so
this failure looks like it comes from this patch. The follow-up "eth:
fbnic: Publish real queue counts" does not touch the resume path, so the
problem is still present at the end of the series.

>  	if (netif_running(netdev)) {
>  		err = __fbnic_open(fbn);
>  		/* On failure the vectors are freed, so drop the count */

[Severity: High]
With fbnic_reset_queues() gone from resume, what restores num_napi after
this failure path?

Right below this comment, the error path still does:

		if (err)
			fbn->num_napi = 0;

Before this patch, the fbnic_reset_queues() call on the next resume
rebuilt num_napi from num_tx_queues/num_rx_queues. fbnic_reset_queues()
is now static and only called from fbnic_netdev_alloc(). So num_napi
stays 0 while the queue counts stay non-zero and netif_running() stays
set. ethtool can't repair it either, because the device is detached.

On the next suspend, the new guard in fbnic_stop() returns early:

	if (!fbn->napi[0])
		return 0;

On the next resume, num_napi == 0 does not match num_tx + num_rx, so
fbnic_alloc_napi_vectors() skips the 1:1 branch and reaches:

	while (num_rx | num_tx) {
		int tqpv = DIV_ROUND_UP(num_tx, num_napi - txq_idx);

Here num_napi == 0 and txq_idx == 0. Can this divide by zero while
rtnl_lock and the netdev instance lock are held?

There is also a second path. After the failed resume, run ip link set
down: fbnic_stop() takes the early return. A later suspend/resume then
succeeds and re-attaches the device. Then ip link set up calls
fbnic_open()->__fbnic_open() with num_napi still 0.

The first failure doesn't need fewer MSI-X vectors (see above). It can
also come from -ENOMEM in fbnic_alloc_resources(), a FW ownership message
failure, a heartbeat init failure, or a fbnic_mac_request_irq() failure.

This also seems to contradict the new comment in fbnic_stop(), which says
suspend "keeps num_napi for resume". fbnic_get_channels() would also
derive its combined/standalone split from num_napi == 0, not from what
the user configured.

[Severity: Medium]
This isn't a bug introduced by this patch, but on the same resume path,
__fbnic_pm_attach() calls fbnic_up() without rtnl or the netdev instance
lock:

	rtnl_lock();
	fbnic_reset_hw_stats(fbd);
	rtnl_unlock();
	...
	if (netif_running(netdev))
		fbnic_up(fbn);

fbnic_up()->fbnic_napi_enable() walks fbn->napi[0..num_napi) and calls
napi_enable_locked(). Everywhere else (ndo_open, set_channels) this runs
under rtnl + netdev_lock.

During AER recovery the two halves are separate callbacks.
fbnic_err_slot_reset()->__fbnic_pm_resume() allocates the NAPI vectors
under rtnl + netdev_lock and then drops both locks.
fbnic_err_resume()->__fbnic_pm_attach() runs later. Userspace is not
frozen in between.

Could an ip link set down in that window reach
fbnic_stop()->fbnic_down()->fbnic_napi_disable() on NAPIs that were never
enabled? NAPI_STATE_SCHED is still set from netif_napi_add(), so
napi_disable_locked() would keep looping in its usleep_range() wait with
rtnl held.

And if the close overlaps fbnic_up() instead, can fbnic_free_napi_vectors()
free fbn->napi[i] while fbnic_up() is still using it?

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