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

[PATCH net] 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 defaults and can change its
private queue counts while the device is down, but does not publish
either value until the next open.

Resume can also allocate fewer IRQs and silently clamp the private
queue counts. Queues can share NAPI vectors, so changing the
configured queue counts is unnecessary.

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

Publish default and offline channel counts immediately. Reset RSS
indirection whenever the queue-count helper runs. Preserve queue
counts across resume and share the remaining NAPI vectors when fewer
IRQs are available. The RSS table remains valid because it indexes RX
queues rather than NAPI vectors.

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 +++++++++++++------
 .../net/ethernet/meta/fbnic/fbnic_netdev.h    |  3 --
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   | 12 ++++++--
 4 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 0e47088ec44b..bec611387333 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -1626,6 +1626,13 @@ static int fbnic_set_channels(struct net_device *netdev,
 		return -EINVAL;
 
 	if (!netif_running(netdev)) {
+		unsigned int rx_count = ch->rx_count + ch->combined_count;
+		unsigned int tx_count = ch->tx_count + ch->combined_count;
+
+		err = netif_set_real_num_queues(netdev, tx_count, rx_count);
+		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..3b44a0ec2a8d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -702,11 +702,12 @@ 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;
+	int err;
 
 	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
 
@@ -717,6 +718,14 @@ 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;
 }
 
 /**
@@ -785,9 +794,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);
 
@@ -832,13 +841,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..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 8b9bc9e8ea56..4dda566d580f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -472,6 +472,7 @@ static int __fbnic_pm_resume(struct device *dev)
 {
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
 	struct net_device *netdev = fbd->netdev;
+	unsigned int max_queues, max_napis;
 	void __iomem * const *iomap_table;
 	struct fbnic_net *fbn;
 	int err;
@@ -510,12 +511,17 @@ 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);
-
 	rtnl_lock();
 	netdev_lock(netdev);
 
+	/* Preserve queue counts, as RX queues may have memory providers bound.
+	 * The RSS table indexes RX queues and remains valid. Rebuild only the
+	 * NAPI layout, sharing vectors if fewer IRQs are available.
+	 */
+	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
+	max_queues = max(fbn->num_tx_queues, fbn->num_rx_queues);
+	fbn->num_napi = min(max_queues, max_napis);
+
 	if (netif_running(netdev))
 		err = __fbnic_open(fbn);
 

base-commit: 83a945a529d6e002dd7339c532288a931f463dba
-- 
2.55.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 2 · Low: 0

- [Medium] Comment and changelog do not match the code:
  `__fbnic_pm_resume()`…
- [Medium] The new resume path makes a channel state reachable that
  fbnic's own ethtool setter rejects.

Pre-existing issues:
- [High] NULL pointer dereference on interface open for a fully split
  channel configuration at the 128-vector boundary.
- [High] Error pointer passed to `page_pool_destroy()` plus leak of the
  header page pool in `fbnic_alloc_qt_page_pools()`…

--

eth: fbnic: Keep real queue counts synchronized

The patch publishes the driver's private queue counts to the netdev real
queue counts as soon as they change (defaults at alloc time and channel
changes while the device is down), and stops clamping the private queue
counts on resume so that memory-provider bindings on Rx queues are
preserved. Only the NAPI layout is recomputed on resume.

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

A few questions on the resume path and on some surrounding code below.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> index 0e47088ec44ba..bec6113873338 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> @@ -1626,6 +1626,13 @@ static int fbnic_set_channels(struct net_device *netdev,
>  		return -EINVAL;
>  
>  	if (!netif_running(netdev)) {
> +		unsigned int rx_count = ch->rx_count + ch->combined_count;
> +		unsigned int tx_count = ch->tx_count + ch->combined_count;
> +
> +		err = netif_set_real_num_queues(netdev, tx_count, rx_count);
> +		if (err)
> +			return err;
> +
>  		fbnic_set_queues(fbn, ch, max_napis);
>  		fbnic_reset_indir_tbl(fbn);
>  		return 0;

[Severity: High]

This isn't a bug introduced by this patch, but while looking at
fbnic_set_channels() the validation just above this hunk seems to accept a
configuration that the NAPI allocator cannot build.

The check is:

	if ((ch->rx_count && ch->tx_count && ch->combined_count) ||
	    (standalone && standalone + ch->combined_count > max_napis) ||

so on a host where max_napis == 128, ethtool -L eth0 rx 64 tx 64 combined 0
passes (128 > 128 is false), and fbnic_set_queues() records
num_rx_queues=64, num_tx_queues=64, num_napi=min(128, 128)=128.

On the next open, fbnic_alloc_napi_vectors() skips its dedicated Tx/Rx
branch because of the strict comparison at num_napi == FBNIC_MAX_TXQS:

	if (num_napi < FBNIC_MAX_TXQS && num_napi == num_tx + num_rx) {

The fallback pairing loop then allocates one Tx/Rx pair per vector
(tqpv = DIV_ROUND_UP(64, 128) = 1, rqpv = 1) and returns 0 after filling
only fbn->napi[0..63].

fbnic_alloc_resources() still walks the configured count:

	for (i = 0; i < fbn->num_napi; i++) {
		err = fbnic_alloc_nv_resources(fbn, fbn->napi[i]);

and fbnic_alloc_nv_resources() dereferences the NULL entry at
for (i = 0; i < nv->txt_count; i++).

Can this oops on open?  fbnic_free_napi_vectors() guards with
if (fbn->napi[i]), which suggests partially populated arrays are expected,
while the alloc walk has no such guard.

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index 8b9bc9e8ea56c..4dda566d580f4 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -510,12 +511,17 @@ 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);
> -
>  	rtnl_lock();
>  	netdev_lock(netdev);
>  
> +	/* Preserve queue counts, as RX queues may have memory providers bound.
> +	 * The RSS table indexes RX queues and remains valid. Rebuild only the
> +	 * NAPI layout, sharing vectors if fewer IRQs are available.
> +	 */
> +	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
> +	max_queues = max(fbn->num_tx_queues, fbn->num_rx_queues);
> +	fbn->num_napi = min(max_queues, max_napis);

[Severity: Medium]

The comment says vectors are shared "if fewer IRQs are available", and the
changelog says "share the remaining NAPI vectors when fewer IRQs are
available".  Does this code look at the previously configured
fbn->num_napi at all?

Take a standalone layout configured with ethtool -L eth0 rx 4 tx 4
combined 0.  fbnic_set_queues() records:

	fbn->num_rx_queues = 4;
	fbn->num_tx_queues = 4;
	fbn->num_napi = min(4 + 4 + 0, max_napis) = 8;

After a suspend/resume that obtains exactly the same number of IRQs,
num_napi becomes min(max(4, 4), max_napis) = 4, so
fbnic_alloc_napi_vectors() no longer takes its one queue per vector
branch:

	if (num_napi < FBNIC_MAX_TXQS && num_napi == num_tx + num_rx) {

and fbnic_get_channels() then reports combined=4, rx=0, tx=0 instead of
the rx=4 / tx=4 that was configured, with half the interrupt contexts.

The collapse to max(tx, rx) matches what the removed
fbnic_reset_queues() call already computed, so the behaviour itself is not
new here, but the comment and changelog now describe something else.
Would min(fbn->num_napi, max_napis) implement what they describe?

[Severity: Medium]

Preserving asymmetric queue counts while lowering num_napi also seems to
make a state reachable that fbnic's own setter rejects.  Before this
change, fbnic_reset_queues() clamped both queue counts to max_napis and
then set num_napi = max(tx, rx), so num_napi >= num_rx_queues always held
after resume.

Now consider rx_count=4, combined_count=4 set while max_napis >= 8, so
num_rx_queues=8, num_tx_queues=4, num_napi=8.  If resume gets only 4 NAPI
IRQs, num_napi = min(8, 4) = 4, and fbnic_get_channels() takes this
branch:

	if (fbn->num_rx_queues > fbn->num_napi ||
	    fbn->num_tx_queues > fbn->num_napi)
		ch->combined_count = min(fbn->num_rx_queues,
					 fbn->num_tx_queues);
	...
	ch->rx_count = fbn->num_rx_queues - ch->combined_count;
	ch->tx_count = fbn->num_tx_queues - ch->combined_count;

reporting combined=4, rx=4, tx=0, i.e. 8 channels while only 4 NAPI
vectors exist.  Feeding those same values back to fbnic_set_channels()
hits:

	    (standalone && standalone + ch->combined_count > max_napis) ||

with 4 + 4 > 4 and returns -EINVAL.  Is it intended that ethtool -l output
can no longer be replayed through ethtool -L after such a resume, and that
it describes more channels than there are vectors?

One more observation outside the diff, in fbnic_alloc_qt_page_pools() in
drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:

[Severity: High]

This is a pre-existing issue and not something this patch changes, but for
an Rx queue with an unreadable memory provider bound, the header pool is
stored in qt->sub0.page_pool and the local pp is then reused for the
payload pool.  When the second page_pool_create() fails, the error label
is reached with pp holding the ERR_PTR:

	err_destroy_sub0:
		page_pool_destroy(pp);
		return PTR_ERR(pp);

page_pool_destroy() only checks for NULL:

	void page_pool_destroy(struct page_pool *pool)
	{
		if (!pool)
			return;

		if (!page_pool_put(pool))
			return;

so does this dereference the error pointer in page_pool_put()?  And does
the label also leak the header pool in qt->sub0.page_pool, which is never
destroyed here?  Should the label destroy qt->sub0.page_pool while still
returning PTR_ERR(pp)?

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

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

[Björn Töpel]

netdev-bot+sashiko@kernel.org writes:

>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> index 0e47088ec44ba..bec6113873338 100644
>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> @@ -1626,6 +1626,13 @@ static int fbnic_set_channels(struct net_device *netdev,
>>  		return -EINVAL;
>>  
>>  	if (!netif_running(netdev)) {
>> +		unsigned int rx_count = ch->rx_count + ch->combined_count;
>> +		unsigned int tx_count = ch->tx_count + ch->combined_count;
>> +
>> +		err = netif_set_real_num_queues(netdev, tx_count, rx_count);
>> +		if (err)
>> +			return err;
>> +
>>  		fbnic_set_queues(fbn, ch, max_napis);
>>  		fbnic_reset_indir_tbl(fbn);
>>  		return 0;
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but while looking at
> fbnic_set_channels() the validation just above this hunk seems to accept a
> configuration that the NAPI allocator cannot build.
>
> The check is:
>
> 	if ((ch->rx_count && ch->tx_count && ch->combined_count) ||
> 	    (standalone && standalone + ch->combined_count > max_napis) ||
>
> so on a host where max_napis == 128, ethtool -L eth0 rx 64 tx 64 combined 0
> passes (128 > 128 is false), and fbnic_set_queues() records
> num_rx_queues=64, num_tx_queues=64, num_napi=min(128, 128)=128.
>
> On the next open, fbnic_alloc_napi_vectors() skips its dedicated Tx/Rx
> branch because of the strict comparison at num_napi == FBNIC_MAX_TXQS:
>
> 	if (num_napi < FBNIC_MAX_TXQS && num_napi == num_tx + num_rx) {
>
> The fallback pairing loop then allocates one Tx/Rx pair per vector
> (tqpv = DIV_ROUND_UP(64, 128) = 1, rqpv = 1) and returns 0 after filling
> only fbn->napi[0..63].
>
> fbnic_alloc_resources() still walks the configured count:
>
> 	for (i = 0; i < fbn->num_napi; i++) {
> 		err = fbnic_alloc_nv_resources(fbn, fbn->napi[i]);
>
> and fbnic_alloc_nv_resources() dereferences the NULL entry at
> for (i = 0; i < nv->txt_count; i++).
>
> Can this oops on open?  fbnic_free_napi_vectors() guards with
> if (fbn->napi[i]), which suggests partially populated arrays are expected,
> while the alloc walk has no such guard.

Ya, this can oops. It is addressed by:
  https://lore.kernel.org/netdev/178942020457.7700.13129750616387075931.stgit@ahduyck-xeon-server.home.arpa/
                                                                                                                                  
The fix includes the 128-vector case in the standalone allocation path.
   
>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>> index 8b9bc9e8ea56c..4dda566d580f4 100644
>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>> @@ -510,12 +511,17 @@ 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);
>> -
>>  	rtnl_lock();
>>  	netdev_lock(netdev);
>>  
>> +	/* Preserve queue counts, as RX queues may have memory providers bound.
>> +	 * The RSS table indexes RX queues and remains valid. Rebuild only the
>> +	 * NAPI layout, sharing vectors if fewer IRQs are available.
>> +	 */
>> +	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
>> +	max_queues = max(fbn->num_tx_queues, fbn->num_rx_queues);
>> +	fbn->num_napi = min(max_queues, max_napis);
>
> [Severity: Medium]
>
> The comment says vectors are shared "if fewer IRQs are available", and the
> changelog says "share the remaining NAPI vectors when fewer IRQs are
> available".  Does this code look at the previously configured
> fbn->num_napi at all?
>
> Take a standalone layout configured with ethtool -L eth0 rx 4 tx 4
> combined 0.  fbnic_set_queues() records:
>
> 	fbn->num_rx_queues = 4;
> 	fbn->num_tx_queues = 4;
> 	fbn->num_napi = min(4 + 4 + 0, max_napis) = 8;
>
> After a suspend/resume that obtains exactly the same number of IRQs,
> num_napi becomes min(max(4, 4), max_napis) = 4, so
> fbnic_alloc_napi_vectors() no longer takes its one queue per vector
> branch:
>
> 	if (num_napi < FBNIC_MAX_TXQS && num_napi == num_tx + num_rx) {
>
> and fbnic_get_channels() then reports combined=4, rx=0, tx=0 instead of
> the rx=4 / tx=4 that was configured, with half the interrupt contexts.
>
> The collapse to max(tx, rx) matches what the removed
> fbnic_reset_queues() call already computed, so the behaviour itself is not
> new here, but the comment and changelog now describe something else.
> Would min(fbn->num_napi, max_napis) implement what they describe?
>
> [Severity: Medium]
>
> Preserving asymmetric queue counts while lowering num_napi also seems to
> make a state reachable that fbnic's own setter rejects.  Before this
> change, fbnic_reset_queues() clamped both queue counts to max_napis and
> then set num_napi = max(tx, rx), so num_napi >= num_rx_queues always held
> after resume.
>
> Now consider rx_count=4, combined_count=4 set while max_napis >= 8, so
> num_rx_queues=8, num_tx_queues=4, num_napi=8.  If resume gets only 4 NAPI
> IRQs, num_napi = min(8, 4) = 4, and fbnic_get_channels() takes this
> branch:
>
> 	if (fbn->num_rx_queues > fbn->num_napi ||
> 	    fbn->num_tx_queues > fbn->num_napi)
> 		ch->combined_count = min(fbn->num_rx_queues,
> 					 fbn->num_tx_queues);
> 	...
> 	ch->rx_count = fbn->num_rx_queues - ch->combined_count;
> 	ch->tx_count = fbn->num_tx_queues - ch->combined_count;
>
> reporting combined=4, rx=4, tx=0, i.e. 8 channels while only 4 NAPI
> vectors exist.  Feeding those same values back to fbnic_set_channels()
> hits:
>
> 	    (standalone && standalone + ch->combined_count > max_napis) ||
>
> with 4 + 4 > 4 and returns -EINVAL.  Is it intended that ethtool -l output
> can no longer be replayed through ethtool -L after such a resume, and that
> it describes more channels than there are vectors?
>
> One more observation outside the diff, in fbnic_alloc_qt_page_pools() in
> drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:

Hmm, the resume path does indeed need some more thinking. I'll spin a v2
of this patch!

> [Severity: High]
>
> This is a pre-existing issue and not something this patch changes, but for
> an Rx queue with an unreadable memory provider bound, the header pool is
> stored in qt->sub0.page_pool and the local pp is then reused for the
> payload pool.  When the second page_pool_create() fails, the error label
> is reached with pp holding the ERR_PTR:
>
> 	err_destroy_sub0:
> 		page_pool_destroy(pp);
> 		return PTR_ERR(pp);
>
> page_pool_destroy() only checks for NULL:
>
> 	void page_pool_destroy(struct page_pool *pool)
> 	{
> 		if (!pool)
> 			return;
>
> 		if (!page_pool_put(pool))
> 			return;
>
> so does this dereference the error pointer in page_pool_put()?  And does
> the label also leak the header pool in qt->sub0.page_pool, which is never
> destroyed here?  Should the label destroy qt->sub0.page_pool while still
> returning PTR_ERR(pp)?

Yes. This has been fixed by:
  https://lore.kernel.org/netdev/20260915104917.3978113-1-bjorn@kernel.org/


Björn

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

[Breno Leitao]

On Tue, Sep 15, 2026 at 08:08:57PM +0200, Björn Töpel wrote:
> alloc_etherdev_mq() initializes the real queue counts to the
> allocation maximum. fbnic selects smaller defaults and can change its
> private queue counts while the device is down, but does not publish
> either value until the next open.
> 
> Resume can also allocate fewer IRQs and silently clamp the private
> queue counts. Queues can share NAPI vectors, so changing the
> configured queue counts is unnecessary.
> 
> These mismatches can expose an inactive queue for memory-provider
> binding or hide an existing binding. A later channel expansion can
> allocate the queue without recognizing its memory provider.
> 
> Publish default and offline channel counts immediately. Reset RSS
> indirection whenever the queue-count helper runs. Preserve queue
> counts across resume and share the remaining NAPI vectors when fewer
> IRQs are available. The RSS table remains valid because it indexes RX
> queues rather than NAPI vectors.
> 
> 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>

> +	/* Preserve queue counts, as RX queues may have memory providers bound.
> +	 * The RSS table indexes RX queues and remains valid. Rebuild only the
> +	 * NAPI layout, sharing vectors if fewer IRQs are available.
> +	 */
> +	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
> +	max_queues = max(fbn->num_tx_queues, fbn->num_rx_queues);
> +	fbn->num_napi = min(max_queues, max_napis);

It seems the comment does not match the code.  num_napi is recomputed
unconditionally from max(num_tx, num_rx), which is not what num_napi
was, so isn't the NAPI layout is rebuilt even when the IRQ count is
unchanged?

--breno