[PATCH net-next v01 0/4] net: hinic3: Bugfix for later patch-set.

[PATCH net-next v01 0/4] net: hinic3: Bugfix for later patch-set.

[Fan Gong]

This patch-set fix 4 bugs.

Fixes: Solve an LRO stall issue
Fixes: Avoid concurrent updates on 32-bit systems.
Fixes: Ensure secure concurrent access to adaptive_rx_coal.
Fixes: Ensure max_qps is always greater than or equal to num_qps

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791858167.git.gongfan1%40huawei.com

This changes the return value when the loop exits on the LRO
replenish threshold. It used to return nr_pkts and now returns budget.
In hinic3_poll():
        work_done = hinic3_rx_poll(irq_cfg->rxq, budget);
        busy |= work_done >= budget;
        if (busy)
                return budget;
As a result, the threshold exit now skips napi_complete_done(),
hinic3_net_dim() and the MSI-X re-enable. NAPI keeps polling instead
of waiting for the next interrupt.
This seems unrelated to statistics, and the commit message does not
mention it. Was it intended? If it fixes an LRO stall, could it go in
a separate patch with its own explanation and a Fixes: tag?

https://sashiko.dev/#/patchset/cover.1792149958.git.gongfan1%40huawei.com

In hinic3_get_stats64() in hinic3_netdev_ops.c, the reader retry loops
accumulate txq_stats and rxq_stats using '+=':
    do {
        start = u64_stats_fetch_begin(&txq_stats->syncp);
        bytes += txq_stats->bytes;
        ...
    } while (u64_stats_fetch_retry(&txq_stats->syncp, start));
If the loop retries due to concurrent updates on 32-bit systems, the counters
are added multiple times. This was previously benign since the fields weren't
populated by the driver, but these new updates will cause incorrect statistics
on 32-bit platforms.

https://sashiko.dev/#/patchset/cover.1774684571.git.zhuyikai1%40h-partners.com

When disabling adaptive RX coalescing via ethtool, does the driver
cancel any pending Dynamic Interrupt Moderation (DIM) work items?
If adaptive coalescing is disabled but an already-scheduled DIM worker
executes asynchronously in a workqueue, could it blindly apply dynamically
calculated DIM values to the hardware, silently overwriting the user's newly
configured static parameters?

https://sashiko.dev/#/patchset/cover.1792671176.git.gongfan1%40huawei.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792671176.git.gongfan1%40huawei.com

After that, hinic3_sw_init()->hinic3_try_to_enable_rss() can shrink
max_qps to 1 when alloc_rss_resource() or hinic3_set_hw_rss_parameters()
fails:
hinic3_rss.c:hinic3_try_to_enable_rss() {
    ...
    err = alloc_rss_resource(netdev);
    if (err) {
        nic_dev->max_qps = 1;
        goto err_reset_q_params;
    }
    ...
}
Only hinic3_vport_up() lowers real_num_*_queues, and only on a successful
open. A device-scope dump before the first successful open (or after opens
that fail before hinic3_vport_up()) still does this:
netdev_nl_stats_by_netdev()
    netdev_stat_queue_sum(netdev, 0, netdev->real_num_rx_queues, &rx_sum,
                          0, netdev->real_num_tx_queues, &tx_sum);
There is no netdev->up check on this path, so i runs from 1 to
func_max_qnum - 1 against a one-entry array.
Would this read past the end of the rxqs and txqs slab objects and return
those values to user space? NETDEV_CMD_QSTATS_GET does not require
GENL_ADMIN_PERM, so any local user can issue it.
The same applies to nic_dev->txqs[i] in hinic3_get_queue_stats_tx().
Before this patch hinic3 had no stat_ops, and hinic3_get_stats64() bounds
its loops by max_qps.

Fan Gong (4):
  Fixes: Solve an LRO stall issue
  Fixes: Avoid concurrent updates on 32-bit systems.
  Fixes: Ensure secure concurrent access to adaptive_rx_coal.
  Fixes: Ensure max_qps is always greater than or equal to num_qps

 .../net/ethernet/huawei/hinic3/hinic3_irq.c   |  2 +-
 .../net/ethernet/huawei/hinic3/hinic3_main.c  |  2 +-
 .../huawei/hinic3/hinic3_netdev_ops.c         | 59 +++++++++----------
 .../net/ethernet/huawei/hinic3/hinic3_rss.c   | 11 ++--
 .../net/ethernet/huawei/hinic3/hinic3_rx.c    |  7 ++-
 5 files changed, 39 insertions(+), 42 deletions(-)


base-commit: 73022f605c447f31610b94ed4a983d2b4bf2c721
-- 
2.43.0

[PATCH net-next v01 1/4] Fixes: Solve an LRO stall issue

[Fan Gong]

As noted in the subsequent review.

This changes the return value when the loop exits on the LRO
replenish threshold. It used to return nr_pkts and now returns budget.
In hinic3_poll():
	work_done = hinic3_rx_poll(irq_cfg->rxq, budget);
	busy |= work_done >= budget;
	if (busy)
		return budget;
As a result, the threshold exit now skips napi_complete_done(),
hinic3_net_dim() and the MSI-X re-enable. NAPI keeps polling instead
of waiting for the next interrupt.
This seems unrelated to statistics, and the commit message does not
mention it. Was it intended? If it fixes an LRO stall, could it go in
a separate patch with its own explanation and a Fixes: tag?

Co-developed-by: Wu Di <wudi234@huawei.com>
Signed-off-by: Wu Di <wudi234@huawei.com>
Co-developed-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Fan Gong <gongfan1@huawei.com>
---
 drivers/net/ethernet/huawei/hinic3/hinic3_rx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
index 309ab5901379..9c4ab88998d4 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
@@ -555,6 +555,7 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
 	struct hinic3_nic_dev *nic_dev = netdev_priv(rxq->netdev);
 	u32 sw_ci, status, pkt_len, vlan_len;
 	struct hinic3_rq_cqe *rx_cqe;
+	bool poll_again = false;
 	u32 num_wqe = 0;
 	int nr_pkts = 0;
 	u16 num_lro;
@@ -581,12 +582,14 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
 
 		rx_cqe->status = 0;
 
-		if (num_wqe >= nic_dev->lro_replenish_thld)
+		if (num_wqe >= nic_dev->lro_replenish_thld) {
+			poll_again = true;
 			break;
+		}
 	}
 
 	if (rxq->delta >= HINIC3_RX_BUFFER_WRITE)
 		hinic3_rx_fill_buffers(rxq);
 
-	return nr_pkts;
+	return poll_again ? budget : nr_pkts;
 }
-- 
2.43.0

[PATCH net-next v01 2/4] Fixes: Avoid concurrent updates on 32-bit systems.

[Fan Gong]

As noted in the subsequent review.

In hinic3_get_stats64() in hinic3_netdev_ops.c, the reader retry loops
accumulate txq_stats and rxq_stats using '+=':
    do {
        start = u64_stats_fetch_begin(&txq_stats->syncp);
        bytes += txq_stats->bytes;
        ...
    } while (u64_stats_fetch_retry(&txq_stats->syncp, start));
If the loop retries due to concurrent updates on 32-bit systems,
the counters are added multiple times. This was previously benign
since the fields weren't populated by the driver, but these new
updates will cause incorrect statistics on 32-bit platforms.

Co-developed-by: Wu Di <wudi234@huawei.com>
Signed-off-by: Wu Di <wudi234@huawei.com>
Co-developed-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Fan Gong <gongfan1@huawei.com>
---
 .../huawei/hinic3/hinic3_netdev_ops.c         | 35 ++++++++-----------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
index da73811641a9..474ea81f1750 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
@@ -795,45 +795,38 @@ static void hinic3_get_stats64(struct net_device *netdev,
 	packets = 0;
 	dropped = 0;
 	for (i = 0; i < nic_dev->max_qps; i++) {
-		if (!nic_dev->txqs)
-			break;
-
 		txq = &nic_dev->txqs[i];
 		txq_stats = &txq->txq_stats;
 		do {
 			start = u64_stats_fetch_begin(&txq_stats->syncp);
-			bytes += txq_stats->bytes;
-			packets += txq_stats->packets;
-			dropped += txq_stats->dropped;
+			bytes = txq_stats->bytes;
+			packets = txq_stats->packets;
+			dropped = txq_stats->dropped;
 		} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
+		stats->tx_packets += packets;
+		stats->tx_bytes   += bytes;
+		stats->tx_dropped += dropped;
 	}
-	stats->tx_packets = packets;
-	stats->tx_bytes   = bytes;
-	stats->tx_dropped = dropped;
 
 	bytes = 0;
 	packets = 0;
 	errors = 0;
 	dropped = 0;
 	for (i = 0; i < nic_dev->max_qps; i++) {
-		if (!nic_dev->rxqs)
-			break;
-
 		rxq = &nic_dev->rxqs[i];
 		rxq_stats = &rxq->rxq_stats;
 		do {
 			start = u64_stats_fetch_begin(&rxq_stats->syncp);
-			bytes += rxq_stats->bytes;
-			packets += rxq_stats->packets;
-			errors += rxq_stats->csum_errors +
-				rxq_stats->other_errors;
-			dropped += rxq_stats->dropped;
+			bytes = rxq_stats->bytes;
+			packets = rxq_stats->packets;
+			errors = rxq_stats->other_errors;
+			dropped = rxq_stats->dropped;
 		} while (u64_stats_fetch_retry(&rxq_stats->syncp, start));
+		stats->rx_packets += packets;
+		stats->rx_bytes   += bytes;
+		stats->rx_errors  += errors;
+		stats->rx_dropped += dropped;
 	}
-	stats->rx_packets = packets;
-	stats->rx_bytes   = bytes;
-	stats->rx_errors  = errors;
-	stats->rx_dropped = dropped;
 }
 
 static void hinic3_nic_set_rx_mode(struct net_device *netdev)
-- 
2.43.0

[PATCH net-next v01 3/4] Fixes: Ensure secure concurrent access to adaptive_rx_coal.

[Fan Gong]

As noted in the subsequent review.

When disabling adaptive RX coalescing via ethtool, does the driver
cancel any pending Dynamic Interrupt Moderation (DIM) work items?
If adaptive coalescing is disabled but an already-scheduled DIM worker
executes asynchronously in a workqueue, could it blindly apply
dynamically calculated DIM values to the hardware, silently overwriting
the user's newly configured static parameters?

Co-developed-by: Wu Di <wudi234@huawei.com>
Signed-off-by: Wu Di <wudi234@huawei.com>
Co-developed-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Fan Gong <gongfan1@huawei.com>
---
 drivers/net/ethernet/huawei/hinic3/hinic3_irq.c  | 2 +-
 drivers/net/ethernet/huawei/hinic3/hinic3_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
index e7d6c2033b45..b4e58d4c8ba8 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
@@ -20,7 +20,7 @@ static void hinic3_net_dim(struct hinic3_nic_dev *nic_dev,
 	struct dim_sample sample = {};
 
 	if (!test_bit(HINIC3_INTF_UP, &nic_dev->flags) ||
-	    !nic_dev->adaptive_rx_coal)
+	    !READ_ONCE(nic_dev->adaptive_rx_coal))
 		return;
 
 	dim_update_sample(irq_cfg->total_events, rxq->rxq_stats.packets,
diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
index 0a888fe4c975..ad0e831ed82b 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
@@ -51,7 +51,7 @@ static void init_intr_coal_param(struct net_device *netdev)
 		info->rx_pending_limit_low = HINIC3_RX_PENDING_LIMIT_LOW;
 	}
 
-	nic_dev->adaptive_rx_coal = 1;
+	WRITE_ONCE(nic_dev->adaptive_rx_coal, 1);
 }
 
 static int hinic3_init_intr_coalesce(struct net_device *netdev)
-- 
2.43.0

[PATCH net-next v01 4/4] Fixes: Ensure max_qps is always greater than or equal to num_qps

[Fan Gong]

As noted in the subsequent review.

After that, hinic3_sw_init()->hinic3_try_to_enable_rss() can shrink
max_qps to 1 when alloc_rss_resource() or hinic3_set_hw_rss_parameters()
fails:
hinic3_rss.c:hinic3_try_to_enable_rss() {
    ...
    err = alloc_rss_resource(netdev);
    if (err) {
        nic_dev->max_qps = 1;
        goto err_reset_q_params;
    }
    ...
}
Only hinic3_vport_up() lowers real_num_*_queues, and only on a successful
open. A device-scope dump before the first successful open (or after opens
that fail before hinic3_vport_up()) still does this:
netdev_nl_stats_by_netdev()
    netdev_stat_queue_sum(netdev, 0, netdev->real_num_rx_queues, &rx_sum,
                          0, netdev->real_num_tx_queues, &tx_sum);
There is no netdev->up check on this path, so i runs from 1 to
func_max_qnum - 1 against a one-entry array.
Would this read past the end of the rxqs and txqs slab objects and return
those values to user space? NETDEV_CMD_QSTATS_GET does not require
GENL_ADMIN_PERM, so any local user can issue it.
The same applies to nic_dev->txqs[i] in hinic3_get_queue_stats_tx().
Before this patch hinic3 had no stat_ops, and hinic3_get_stats64() bounds
its loops by max_qps.

This patch also solve some defects reported by checkpatch.

Co-developed-by: Wu Di <wudi234@huawei.com>
Signed-off-by: Wu Di <wudi234@huawei.com>
Co-developed-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Fan Gong <gongfan1@huawei.com>
---
 .../huawei/hinic3/hinic3_netdev_ops.c         | 24 ++++++++++---------
 .../net/ethernet/huawei/hinic3/hinic3_rss.c   | 11 ++++-----
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
index 474ea81f1750..7428c4d98917 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
@@ -251,16 +251,17 @@ static void hinic3_remove_configure(struct net_device *netdev)
 		hinic3_rss_uninit(netdev);
 }
 
-static int hinic3_alloc_channel_resources(struct net_device *netdev,
-					  struct hinic3_dyna_qp_params *qp_params,
-					  struct hinic3_dyna_txrxq_params *trxq_params)
+static int
+hinic3_alloc_channel_resources(struct net_device *netdev,
+			       struct hinic3_dyna_qp_params *qp_params,
+			       struct hinic3_dyna_txrxq_params *q_params)
 {
 	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
 	int err;
 
-	qp_params->num_qps = trxq_params->num_qps;
-	qp_params->sq_depth = trxq_params->sq_depth;
-	qp_params->rq_depth = trxq_params->rq_depth;
+	qp_params->num_qps = q_params->num_qps;
+	qp_params->sq_depth = q_params->sq_depth;
+	qp_params->rq_depth = q_params->rq_depth;
 
 	err = hinic3_alloc_qps(nic_dev, qp_params);
 	if (err) {
@@ -268,7 +269,7 @@ static int hinic3_alloc_channel_resources(struct net_device *netdev,
 		return err;
 	}
 
-	err = hinic3_alloc_txrxq_resources(netdev, trxq_params);
+	err = hinic3_alloc_txrxq_resources(netdev, q_params);
 	if (err) {
 		netdev_err(netdev, "Failed to alloc txrxq resources\n");
 		hinic3_free_qps(nic_dev, qp_params);
@@ -278,13 +279,14 @@ static int hinic3_alloc_channel_resources(struct net_device *netdev,
 	return 0;
 }
 
-static void hinic3_free_channel_resources(struct net_device *netdev,
-					  struct hinic3_dyna_qp_params *qp_params,
-					  struct hinic3_dyna_txrxq_params *trxq_params)
+static void
+hinic3_free_channel_resources(struct net_device *netdev,
+			      struct hinic3_dyna_qp_params *qp_params,
+			      struct hinic3_dyna_txrxq_params *q_params)
 {
 	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
 
-	hinic3_free_txrxq_resources(netdev, trxq_params);
+	hinic3_free_txrxq_resources(netdev, q_params);
 	hinic3_free_qps(nic_dev, qp_params);
 }
 
diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
index 25db74d8c7dd..4aa725339e60 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
@@ -308,14 +308,14 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)
 
 	nic_dev->max_qps = hinic3_func_max_qnum(hwdev);
 	if (nic_dev->max_qps <= 1 ||
-	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS))
+	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS)) {
+		nic_dev->max_qps = 1;
 		goto err_reset_q_params;
+	}
 
 	err = alloc_rss_resource(netdev);
-	if (err) {
-		nic_dev->max_qps = 1;
+	if (err)
 		goto err_reset_q_params;
-	}
 
 	set_bit(HINIC3_RSS_ENABLE, &nic_dev->flags);
 	decide_num_qps(netdev);
@@ -324,7 +324,6 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)
 	if (err) {
 		dev_err(hwdev->dev, "Failed to set hardware rss parameters\n");
 		hinic3_clear_rss_config(netdev);
-		nic_dev->max_qps = 1;
 		goto err_reset_q_params;
 	}
 
@@ -332,5 +331,5 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)
 
 err_reset_q_params:
 	clear_bit(HINIC3_RSS_ENABLE, &nic_dev->flags);
-	nic_dev->q_params.num_qps = nic_dev->max_qps;
+	nic_dev->q_params.num_qps = 1;
 }
-- 
2.43.0

Re: [PATCH net-next v01 0/4] net: hinic3: Bugfix for later patch-set.

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next v01 0/4] net: hinic3: Bugfix for later patch-set.

[Fan Gong]

> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

These patch-set have been tested on hinic3 hardware.

Re: [PATCH net-next v01 1/4] Fixes: Solve an LRO stall issue

[netdev-bot+sashiko]

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

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

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).

- [Medium] The commit message does not describe the change.
- [Low] The patch says it fixes a user-visible RX stall ("Solve an LRO
  stall issue") and the reviewer text it quotes asks for a Fixes: tag,…

--

Fixes: Solve an LRO stall issue

This patch changes hinic3_rx_poll() to return budget instead of nr_pkts
when the RX loop exits on the LRO replenish threshold. As a result,
hinic3_poll() stays in polling mode and does not complete NAPI or
re-enable the MSI-X vector.

> Fixes: Solve an LRO stall issue
>
> As noted in the subsequent review.
>
> This changes the return value when the loop exits on the LRO
> replenish threshold. It used to return nr_pkts and now returns budget.

[ ... ]

> This seems unrelated to statistics, and the commit message does not
> mention it. Was it intended? If it fixes an LRO stall, could it go in
> a separate patch with its own explanation and a Fixes: tag?

[Severity: Medium]
Could the commit message be rewritten so it describes this change on its
own?

The subject has no "hinic3:" prefix. It also starts with "Fixes:", and
tools that grep for Fixes: trailers can misread that.

"As noted in the subsequent review." points to a mailing list thread that
won't be in git history.

The rest of the body is the review text copied word for word. That
includes the questions "Was it intended?" and "could it go in a separate
patch with its own explanation and a Fixes: tag?", and they are left
unanswered.

The message doesn't say what stalls, when it stalls, or why returning
budget from hinic3_rx_poll() on the lro_replenish_thld exit fixes it. The
threshold defaults to HINIC3_LRO_REPLENISH_THLD, which is 256.

Is this the mechanism? Before the patch, the threshold break returned
nr_pkts < budget, so hinic3_poll() went on to run:

	if (likely(napi_complete_done(napi, work_done))) {
		hinic3_net_dim(nic_dev, irq_cfg);
		hinic3_set_msix_state(nic_dev->hwdev, irq_cfg->msix_entry_idx,
				      HINIC3_MSIX_ENABLE);

At that point, CQEs with RXDONE already set could still be in the ring.
If the device doesn't raise a new interrupt for CQEs it wrote before the
vector was re-enabled, those packets would stay there until more traffic
arrives.

Could the message say whether this device behaviour is what the patch
works around? The change was split out as the review asked, but the
explanation the review asked for seems to be missing.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> index 309ab59013797..9c4ab88998d47 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c

[ ... ]

> @@ -581,12 +582,14 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
>  
>  		rx_cqe->status = 0;
>  
> -		if (num_wqe >= nic_dev->lro_replenish_thld)
> +		if (num_wqe >= nic_dev->lro_replenish_thld) {
> +			poll_again = true;
>  			break;
> +		}
>  	}
>  
>  	if (rxq->delta >= HINIC3_RX_BUFFER_WRITE)
>  		hinic3_rx_fill_buffers(rxq);
>  
> -	return nr_pkts;
> +	return poll_again ? budget : nr_pkts;
>  }

[Severity: Low]
The patch is described as a fix for an RX stall, so should it carry a
Fixes: tag? The threshold early exit in hinic3_rx_poll(), and the
return nr_pkts it reaches, came in with:

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")

Without that tag and a net tree target, stable kernels that ship hinic3
won't pick up the fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com

Re: [PATCH net-next v01 2/4] Fixes: Avoid concurrent updates on 32-bit systems.

[netdev-bot+sashiko]

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

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

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).

- [Medium] The subject and body of commit "Fixes: Avoid concurrent
  updates on 32-bit systems." do not match the change.
- [Medium] hinic3_get_stats64()
  (drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c) quietly…
- [Low] The patch fixes a double-counting bug that cannot happen at this
  tree state, or at the end of the series.
- [Low] hinic3: dead counter initializations in hinic3_get_stats64()

--

Fixes: Avoid concurrent updates on 32-bit systems.

This commit changes the u64_stats reader retry loops in
hinic3_get_stats64(). The per-queue values are now assigned inside the
loop and added into the rtnl_link_stats64 fields after it. It also drops
the nic_dev->txqs and nic_dev->rxqs NULL checks and stops adding
csum_errors into rx_errors.

> Fixes: Avoid concurrent updates on 32-bit systems.

[Severity: Medium]
Could the subject be reworked?

It has no subsystem prefix. Something like "net: hinic3:" would be
expected.

It also starts with "Fixes:", which is a trailer tag. That text isn't
followed by a commit reference, and the tags have no real
Fixes: <sha> ("...") trailer. Tools that grep for '^Fixes:' may also
match this subject.

The subject says the patch avoids concurrent updates, but none of the
writers are changed. The only change is on the reader side of
hinic3_get_stats64(), where '+=' becomes '=' inside the retry loop.

> As noted in the subsequent review.

The body starts with this line, then repeats a reviewer's comment about
the old code. It never says what this patch does. Could the changelog
describe the change itself?

The other patches in the series use the same layout. In "Fixes: Solve an
LRO stall issue" the body even keeps the reviewer's question "Was it
intended? ... could it go in a separate patch".

Were these review fixups meant to be folded into the patches they
address, rather than sent as standalone commits?

> If the loop retries due to concurrent updates on 32-bit systems,
> the counters are added multiple times. This was previously benign
> since the fields weren't populated by the driver, but these new
> updates will cause incorrect statistics on 32-bit platforms.

[Severity: Low]
Which new updates does this refer to?

In drivers/net/ethernet/huawei/hinic3/, the only writers of txq_stats
and rxq_stats are hinic3_txq_clean_stats() and hinic3_rxq_clean_stats().
Both only zero the counters:

hinic3_rx.c:hinic3_rxq_clean_stats() {
	u64_stats_update_begin(&rxq_stats->syncp);
	rxq_stats->bytes = 0;
	rxq_stats->packets = 0;
	...
}

The rx checksum error path in hinic3_rx_csum() doesn't count anything
either:

	if (unlikely(csum_err)) {
		/* pkt type is recognized by HW, and csum is wrong */
		skb->ip_summed = CHECKSUM_NONE;
		return;
	}

So a retry of the old '+=' loop would only add zeros. No per-packet
updates are added by this patch, by the baseline, or by any later patch
in the series.

Should this be squashed into the patch that starts populating the
counters, instead of being a standalone fix?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9d..474ea81f17508 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> @@ -795,45 +795,38 @@ static void hinic3_get_stats64(struct net_device *netdev,

[ ... ]

>  	for (i = 0; i < nic_dev->max_qps; i++) {
> -		if (!nic_dev->rxqs)
> -			break;
> -
>  		rxq = &nic_dev->rxqs[i];
>  		rxq_stats = &rxq->rxq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&rxq_stats->syncp);
> -			bytes += rxq_stats->bytes;
> -			packets += rxq_stats->packets;
> -			errors += rxq_stats->csum_errors +
> -				rxq_stats->other_errors;
> -			dropped += rxq_stats->dropped;
> +			bytes = rxq_stats->bytes;
> +			packets = rxq_stats->packets;
> +			errors = rxq_stats->other_errors;
> +			dropped = rxq_stats->dropped;

[Severity: Medium]
Is dropping csum_errors from rx_errors intended here? The old code did:

	errors += rxq_stats->csum_errors + rxq_stats->other_errors;

The new code reads only other_errors, so rtnl_link_stats64.rx_errors no
longer includes checksum errors. The retry fix didn't need this, and the
commit message doesn't mention it.

struct hinic3_rxq_stats still declares csum_errors, and
hinic3_rxq_clean_stats() still resets it under the syncp. After this
patch, nothing reads it.

The sibling hinic driver still reports errors = csum_errors +
other_errors in hinic_rxq_get_stats(), which is what hinic3 did before
this patch.

Once the rx path starts populating csum_errors, would checksum failures
drop out of rx_errors in 'ip -s link', with no other counter reporting
them? hinic3_get_stats64() is unchanged at the end of the series, so no
later patch restores it.

The same patch also removes the nic_dev->txqs and nic_dev->rxqs NULL
checks. That looks safe: hinic3_sw_init() allocates txqs and rxqs before
register_netdev(). In hinic3_nic_remove(), hinic3_sw_uninit() frees them
after unregister_netdev().

It's still unrelated to the stated fix, though. Could these two changes
be explained in the changelog or moved into separate patches?

[ ... ]

> @@ -795,45 +795,38 @@ static void hinic3_get_stats64(struct net_device *netdev,
>  	packets = 0;
>  	dropped = 0;
>  	for (i = 0; i < nic_dev->max_qps; i++) {
> -		if (!nic_dev->txqs)
> -			break;
> -
>  		txq = &nic_dev->txqs[i];
>  		txq_stats = &txq->txq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&txq_stats->syncp);
> -			bytes += txq_stats->bytes;
> -			packets += txq_stats->packets;
> -			dropped += txq_stats->dropped;
> +			bytes = txq_stats->bytes;
> +			packets = txq_stats->packets;
> +			dropped = txq_stats->dropped;
>  		} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
> +		stats->tx_packets += packets;
> +		stats->tx_bytes   += bytes;
> +		stats->tx_dropped += dropped;
>  	}
> -	stats->tx_packets = packets;
> -	stats->tx_bytes   = bytes;
> -	stats->tx_dropped = dropped;
>  
>  	bytes = 0;
>  	packets = 0;
>  	errors = 0;
>  	dropped = 0;
>  	for (i = 0; i < nic_dev->max_qps; i++) {
> -		if (!nic_dev->rxqs)
> -			break;
> -
>  		rxq = &nic_dev->rxqs[i];
>  		rxq_stats = &rxq->rxq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&rxq_stats->syncp);
> -			bytes += rxq_stats->bytes;
> -			packets += rxq_stats->packets;
> -			errors += rxq_stats->csum_errors +
> -				rxq_stats->other_errors;
> -			dropped += rxq_stats->dropped;
> +			bytes = rxq_stats->bytes;
> +			packets = rxq_stats->packets;
> +			errors = rxq_stats->other_errors;
> +			dropped = rxq_stats->dropped;
>  		} while (u64_stats_fetch_retry(&rxq_stats->syncp, start));
> +		stats->rx_packets += packets;
> +		stats->rx_bytes   += bytes;
> +		stats->rx_errors  += errors;
> +		stats->rx_dropped += dropped;
>  	}
> -	stats->rx_packets = packets;
> -	stats->rx_bytes   = bytes;
> -	stats->rx_errors  = errors;
> -	stats->rx_dropped = dropped;
>  }
>  
>  static void hinic3_nic_set_rx_mode(struct net_device *netdev)

[Severity: Low]
Now that the reads inside both retry loops use '=' instead of '+=',
the resets before the loops are dead stores. That is bytes, packets and
dropped before the tx loop, and bytes, packets, errors and dropped
before the rx loop.

Every iteration of the for loop overwrites those locals in the do/while
before anything reads them. The per-queue values are then summed into
the rtnl_link_stats64 fields, which dev_get_stats() has already cleared
before it calls ndo_get_stats64. Nothing reads the zero values.

This causes no functional problem. The leftover resets still make the
locals look like accumulators, which is the pattern this patch is
removing.

Could the resets be dropped? Alternatively, the locals could be declared
inside the loop bodies, so it's clear they only hold one queue's
snapshot.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com

Re: [PATCH net-next v01 3/4] Fixes: Ensure secure concurrent access to adaptive_rx_coal.

[netdev-bot+sashiko]

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

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

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 fix does not close the race its commit message describes.
- [Medium] The commit message describes a bug that cannot happen in this
  tree, and the code change does not touch that behaviour.
- [Low] The commit subject and body are malformed.

--

Fixes: Ensure secure concurrent access to adaptive_rx_coal.

This patch wraps the write of nic_dev->adaptive_rx_coal in
init_intr_coal_param() in WRITE_ONCE(), and the read in hinic3_net_dim()
in READ_ONCE(). The commit message says this fixes a race where an
already-scheduled DIM work item overwrites static coalescing settings
after adaptive RX coalescing is disabled via ethtool.

> Fixes: Ensure secure concurrent access to adaptive_rx_coal.

[Severity: Low]
This isn't a bug, but the subject starts with "Fixes:". Tools and
maintainers read that as a trailer pointing at an earlier commit. The
subject also lacks the "net: hinic3:" prefix and ends with a period.

> As noted in the subsequent review.

The body points to a review the reader can't see. It then copies a
reviewer's question word for word, but never says what the patch changes
or why. There is also no real Fixes: <sha> ("...") trailer.

Would something like "net: hinic3: annotate adaptive_rx_coal accesses
with READ_ONCE/WRITE_ONCE" describe the change more accurately?

> When disabling adaptive RX coalescing via ethtool, does the driver
> cancel any pending Dynamic Interrupt Moderation (DIM) work items?

[Severity: Medium]
Can this sequence happen in the current driver? hinic3_ethtool_ops has no
.set_coalesce or .get_coalesce callback:

static const struct ethtool_ops hinic3_ethtool_ops = {
	.supported_coalesce_params      = ETHTOOL_COALESCE_USECS |
					  ETHTOOL_COALESCE_PKT_RATE_RX_USECS,
	.get_link_ksettings             = hinic3_get_link_ksettings,
	.get_drvinfo                    = hinic3_get_drvinfo,
	.get_msglevel                   = hinic3_get_msglevel,
	.set_msglevel                   = hinic3_set_msglevel,
	.get_link                       = ethtool_op_get_link,
};

The only writer of adaptive_rx_coal runs once at probe time, before
register_netdev():

hinic3_nic_probe()
  hinic3_sw_init()
    hinic3_alloc_txrxqs()
      hinic3_init_intr_coalesce()
        init_intr_coal_param()
          WRITE_ONCE(nic_dev->adaptive_rx_coal, 1);

The only reader is hinic3_net_dim(). The value never changes after probe,
so the READ_ONCE()/WRITE_ONCE() annotations don't change behaviour.
Nothing later in the series adds set_coalesce support either.

Could the commit message describe this as an annotation-only change,
rather than a fix for a user-visible coalescing bug? As written, it may
mislead maintainers and stable backporters.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> index e7d6c2033b451..b4e58d4c8ba85 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> @@ -20,7 +20,7 @@ static void hinic3_net_dim(struct hinic3_nic_dev *nic_dev,
>  	struct dim_sample sample = {};
>  
>  	if (!test_bit(HINIC3_INTF_UP, &nic_dev->flags) ||
> -	    !nic_dev->adaptive_rx_coal)
> +	    !READ_ONCE(nic_dev->adaptive_rx_coal))
>  		return;
>  
>  	dim_update_sample(irq_cfg->total_events, rxq->rxq_stats.packets,

[Severity: High]
Does this close the race the commit message describes? Nothing cancels,
flushes or disables rxq->dim.work when the flag changes. The work handler
also never re-checks adaptive_rx_coal before it programs the hardware:

hinic3_rx_dim_work() {
	...
	cur_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);

	hinic3_update_queue_coal(rxq->netdev, rxq->q_id,
				 cur_moder.usec, cur_moder.pkts);
	...
}

That goes through
hinic3_update_queue_coal()->hinic3_set_interrupt_moder()->hinic3_set_interrupt_cfg().
It writes the DIM-derived coalesc_timer_cfg and pending_limit to the
hardware, then updates rxq->last_coalesc_timer_cfg and last_pending_limit.

The check here also happens separately from the schedule_work() inside
net_dim(), so this is a check-then-act sequence:

CPU0 (NAPI)                          CPU1 (disable path, if one existed)
hinic3_net_dim()
  READ_ONCE(adaptive_rx_coal) == 1
                                     clears adaptive_rx_coal
                                     programs static coalescing values
  net_dim()
    schedule_work(&dim->work)

hinic3_rx_dim_work()
  hinic3_update_queue_coal()   <- overwrites CPU1's static values

Work queued before the flag was cleared would still run as well.

READ_ONCE()/WRITE_ONCE() only stop the compiler from tearing or caching
the access. They don't order anything against the work item, and they
can't stop it from running.

Would the fix need one of these instead?

- The worker re-checks the flag under a lock (or with proper ordering)
  shared with the configuration path.
- The disable path clears the flag, quiesces NAPI, and cancels or
  disables each rxq->dim.work before it programs the static values.

Right now the only disable_work_sync() calls on dim.work are in the IRQ
teardown paths. Nothing later in the series adds this synchronization.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com

Re: [PATCH net-next v01 4/4] Fixes: Ensure max_qps is always greater than or equal to num_qps

[netdev-bot+sashiko]

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

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

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] In hinic3_try_to_enable_rss()
  (drivers/net/ethernet/huawei/hinic3/hinic3_rss.c:309-314), the patch…
- [Medium] The subject 'Ensure max_qps is always greater than or equal to
  num_qps' describes an invariant that already held in the baseline…
- [Medium] The commit message justifies the change with an out-of-bounds
  read through netdev_nl_stats_by_netdev() -> netdev_stat_queue_sum() ->…
- [Low] The subject uses 'Fixes:' as a prefix instead of a subsystem
  prefix such as 'net: hinic3:', and there is no `Fixes: <12+ char sha>…

--

Fixes: Ensure max_qps is always greater than or equal to num_qps

This patch changes hinic3_try_to_enable_rss() in two ways. First, max_qps
is now forced to 1 only when the function reports max_qps <= 1 or lacks
RSS support, and stays at hinic3_func_max_qnum() when RSS resource
allocation or hardware configuration fails. Second, every error path now
sets q_params.num_qps to 1. The patch also reflows the signatures of
hinic3_alloc_channel_resources() and hinic3_free_channel_resources() and
renames their trxq_params argument to q_params.

> Fixes: Ensure max_qps is always greater than or equal to num_qps

[Severity: Medium]
Is this subject accurate? Before this patch, hinic3_try_to_enable_rss()
already kept num_qps <= max_qps on every exit. The error label did:

err_reset_q_params:
	clear_bit(HINIC3_RSS_ENABLE, &nic_dev->flags);
	nic_dev->q_params.num_qps = nic_dev->max_qps;

The success path goes through decide_num_qps():

	nic_dev->q_params.num_qps = min(dev_cpus, nic_dev->max_qps);

At open, hinic3_config_num_qps() only lowers num_qps further.

The problem the body describes is a different one: the per-queue arrays
sized by max_qps can be smaller than netdev->real_num_*_queues.

The message also leaves out the behaviour changes this patch makes:

- On multi-queue hardware without HINIC3_NIC_F_RSS, max_qps changes from
  hinic3_func_max_qnum() to 1. That shrinks txqs, rxqs, intr_coalesce and
  qps_msix_entries, and lowers the hinic3_get_stats64() loop bounds.
- When alloc_rss_resource() or hinic3_set_hw_rss_parameters() fails,
  max_qps now stays at hinic3_func_max_qnum() instead of 1.
- err_reset_q_params now hard-codes q_params.num_qps = 1.

Could the message describe these changes, especially the one on the
non-RSS path?

> As noted in the subsequent review.
>
> After that, hinic3_sw_init()->hinic3_try_to_enable_rss() can shrink
> max_qps to 1 when alloc_rss_resource() or hinic3_set_hw_rss_parameters()
> fails:

[ ... ]

> Would this read past the end of the rxqs and txqs slab objects and return
> those values to user space? NETDEV_CMD_QSTATS_GET does not require
> GENL_ADMIN_PERM, so any local user can issue it.
> The same applies to nic_dev->txqs[i] in hinic3_get_queue_stats_tx().
> Before this patch hinic3 had no stat_ops, and hinic3_get_stats64() bounds
> its loops by max_qps.

[Severity: Medium]
Does the code path described here exist in this tree? There is no
hinic3_get_queue_stats_rx(), hinic3_get_queue_stats_tx() or stat_ops
anywhere under drivers/net/ethernet/huawei/hinic3/. The only per-queue
stats consumer is hinic3_get_stats64(), and it is bounded by max_qps:

	for (i = 0; i < nic_dev->max_qps; i++) {
		txq = &nic_dev->txqs[i];

"Before this patch hinic3 had no stat_ops" looks like it refers to a
different, unmerged patch. If so, should this change be folded into the
patch that adds stat_ops? As a standalone fix, its rationale depends on
code that isn't in the tree.

The message also opens with "As noted in the subsequent review." and
pastes the reviewer's question ("Would this read past the end...?")
verbatim. Could it say what was wrong and how this patch fixes it
instead?

> This patch also solve some defects reported by checkpatch.

[Severity: Low]
This isn't a bug, but the subject uses "Fixes:" as its prefix instead of
a subsystem prefix such as "net: hinic3:". There is also no
Fixes: <sha> ("subject") trailer, so backport tooling can't tell which
commit this fixes.

The checkpatch cleanup in hinic3_netdev_ops.c only reflows the signatures
of hinic3_alloc_channel_resources() and hinic3_free_channel_resources()
and renames trxq_params to q_params. It has nothing to do with max_qps.
Could it go in a separate patch?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> index 25db74d8c7ddb..4aa725339e604 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> @@ -308,14 +308,14 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)
>  
>  	nic_dev->max_qps = hinic3_func_max_qnum(hwdev);
>  	if (nic_dev->max_qps <= 1 ||
> -	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS))
> +	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS)) {
> +		nic_dev->max_qps = 1;

[Severity: High]
Doesn't this recreate the mismatch the commit message says it closes,
and now on the normal probe path? Take a function whose firmware reports
max_sqs > 1 but doesn't advertise HINIC3_NIC_F_RSS:

hinic3_nic_probe()
    max_qps = hinic3_func_max_qnum(hwdev);
    netdev = alloc_etherdev_mq(sizeof(*nic_dev), max_qps);
    -> real_num_{tx,rx}_queues start at func_max_qnum
hinic3_sw_init()
    hinic3_try_to_enable_rss()
        nic_dev->max_qps = 1;

hinic3_alloc_txqs() and hinic3_alloc_rxqs() then size nic_dev->txqs[]
and nic_dev->rxqs[] by max_qps:

    u16 q_id, num_txqs = nic_dev->max_qps;
    ...
    u16 num_rxqs = nic_dev->max_qps;

hinic3_init_intr_coalesce() and the qps_msix_entries allocation also use
max_qps, so each of these arrays ends up with one entry.

Only hinic3_vport_up() lowers real_num_*_queues, and only on a
successful open:

	err = netif_set_real_num_queues(netdev, nic_dev->q_params.num_qps,
					nic_dev->q_params.num_qps);

Before this patch, this branch left max_qps at func_max_qnum, so the
arrays matched the netdev queue count.

The stat_ops consumer the commit message describes walks
real_num_*_queues:

netdev_nl_stats_by_netdev()->netdev_stat_queue_sum()->
    hinic3_get_queue_stats_rx()/hinic3_get_queue_stats_tx()

Once that is added, wouldn't nic_dev->rxqs[i] and nic_dev->txqs[i] read
past the one-entry slab objects on every non-RSS multi-queue function?
Any local user could trigger this via NETDEV_CMD_QSTATS_GET, which
doesn't require GENL_ADMIN_PERM.

At this revision the read can't happen yet:

- hinic3_xmit_frame() is bounded by q_params.num_qps.
- hinic3_get_stats64() is bounded by max_qps.
- There are no per-queue ethtool or stat_ops callbacks.

The bug class the message claims to fix is still there, though, and it
now hits the common no-RSS probe path rather than rare allocation or
firmware failures.

Is the new assignment needed at all? hinic3_config_num_qps() already
forces num_qps to 1 at open when RSS is off:

	if (!test_bit(HINIC3_RSS_ENABLE, &nic_dev->flags))
		q_params->num_qps = 1;

On this path, the only effect of the assignment seems to be shrinking
the arrays.

Would it be more consistent to keep max_qps = hinic3_func_max_qnum() in
every branch and set only q_params.num_qps to 1? Other options would be
to size the netdev from the same value as the arrays, or to call
netif_set_real_num_queues() during probe.

> +		goto err_reset_q_params;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com