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

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

[Fan Gong]

Testing:
  These patch-set have been tested on hinic3 hardware.

V02:
* Rewrite the commit message and provide an explanation for each
  patch.(AI Review)
* Split checkpatchs in to a separate patch.(AI Review)
* Fix the failure of napi_alloc_skb() in recv_one_pkt().(AI Review)
* 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. So, remove READ_ONCE() and WRITE_ONCE() on
  nic_dev->adaptive_rx_coal.(AI Review)

V01: https://lore.kernel.org/netdev/cover.1793946318.git.gongfan1@huawei.com/

Fan Gong(5)
  hinic3: Solve an stall issue in hinic3_rx_poll()
  hinic3: Avoid concurrent updates on 32-bit systems.
  hinic3: fix some checkpatchs.
  hinic3: Ensure the value of max_qps is greater than 1
  hinic3: Ensure that the value of max_qps is not changed

Fan Gong (5):
  hinic3: Solve an stall issue in hinic3_rx_poll()
  hinic3: Avoid concurrent updates on 32-bit systems.
  hinic3: fix some checkpatchs.
  hinic3: Ensure the value of max_qps is greater than 1
  hinic3: Ensure that the value of max_qps is not changed

 .../ethernet/huawei/hinic3/hinic3_hw_cfg.c    |  4 +-
 .../huawei/hinic3/hinic3_netdev_ops.c         | 66 ++++++++-----------
 .../net/ethernet/huawei/hinic3/hinic3_rss.c   |  9 +--
 .../net/ethernet/huawei/hinic3/hinic3_rx.c    | 11 +++-
 4 files changed, 41 insertions(+), 49 deletions(-)


base-commit: 73022f605c447f31610b94ed4a983d2b4bf2c721
-- 
2.43.0

[PATCH net-next v02 1/5] hinic3: Solve an stall issue in hinic3_rx_poll()

[Fan Gong]

When Large Receive Offload (LRO) packets are large, such as 32KB, each
packet consumes 16 Work Queue Elements (WQEs). If multiple large LRO
packets occur at this time, it will cause rapid consumption of WQEs in
the RQ and WQEs cannot be replenished in time. The chip will drop
packets due to lack of WQEs, causing the RQ to be unable to receive
packets. Furthermore, the NAPI instance will not be polled again,
resulting in residual packets in the RQ.

When napi_alloc_skb() fails inside recv_one_pkt(), hinic3_rx_poll()
breaks out of the loop without advancing rxq->cons_idx or clearing
rx_cqe->status, and returns nr_pkts (< budget).  hinic3_poll() then
calls napi_complete_done() and re-enables the edge-triggered MSI-X
interrupt while the unprocessed CQE remains at the head of the ring,
stalling the receive queue until a new packet arrives to trigger
another interrupt.

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")
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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
index 309ab5901379..f2262cdd6628 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;
@@ -571,8 +572,10 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
 
 		vlan_len = le32_to_cpu(rx_cqe->vlan_len);
 		pkt_len = RQ_CQE_SGE_GET(vlan_len, LEN);
-		if (recv_one_pkt(rxq, rx_cqe, pkt_len, vlan_len, status))
+		if (recv_one_pkt(rxq, rx_cqe, pkt_len, vlan_len, status)) {
+			poll_again = true;
 			break;
+		}
 
 		nr_pkts++;
 		num_lro = RQ_CQE_STATUS_GET(status, NUM_LRO);
@@ -581,12 +584,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

Re: [PATCH net-next v02 1/5] hinic3: Solve an stall issue in hinic3_rx_poll()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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 patch does not close the WQE-starvation stall that its
  commit message describes.
- [Low] When napi_alloc_skb() fails, hinic3_rx_poll() now returns
  `budget` even if zero packets were processed (the poll_again path at…

--

hinic3: Solve an stall issue in hinic3_rx_poll()

This patch makes hinic3_rx_poll() return the full budget in two cases:
when recv_one_pkt() fails, and when the LRO replenish threshold is hit.
In both cases hinic3_poll() then keeps NAPI scheduled instead of calling
napi_complete_done() while an unprocessed CQE sits at the head of the
ring.

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

[ ... ]

> @@ -571,8 +572,10 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
>  
>  		vlan_len = le32_to_cpu(rx_cqe->vlan_len);
>  		pkt_len = RQ_CQE_SGE_GET(vlan_len, LEN);
> -		if (recv_one_pkt(rxq, rx_cqe, pkt_len, vlan_len, status))
> +		if (recv_one_pkt(rxq, rx_cqe, pkt_len, vlan_len, status)) {
> +			poll_again = true;
>  			break;
> +		}

[Severity: Low]
This isn't a bug, but a note on this path. When napi_alloc_skb() fails,
the CQE stays in place and hinic3_rx_poll() now returns budget even when
nr_pkts is 0. hinic3_poll() then repolls the same head CQE until an
atomic skb allocation succeeds.

This matches the "return failure ? budget : total_rx_packets" idiom in
i40e/iavf/ice. net_rx_action() also bounds it, so it isn't a livelock.

Should this failure be counted somewhere? rxq_stats has an alloc_skb_err
field, but hinic3_rxq_stats_init() only ever sets it to zero, so these
retries can't be seen from userspace.

[ ... ]

> @@ -581,12 +584,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: High]
Can the RQ still stall for good if hinic3_rx_fill_buffers() can't post
any buffers here?

The commit message describes this case: "WQEs cannot be replenished in
time. The chip will drop packets due to lack of WQEs ... Furthermore,
the NAPI instance will not be polled again". However, the return value
of hinic3_rx_fill_buffers() is ignored, and poll_again is never set
when the refill falls short.

hinic3_rx_fill_buffers() stops at the first page_pool failure and rings
the doorbell only for the buffers it actually posted:

drivers/net/ethernet/huawei/hinic3/hinic3_rx.c:hinic3_rx_fill_buffers() {
    ...
		err = rx_alloc_mapped_page(rxq->page_pool, rx_info,
					   rxq->buf_len);
		if (unlikely(err))
			break;
    ...
	if (likely(i)) {
		hinic3_write_db(...);
		rxq->delta -= i;
    ...
}

Suppose the device has used up every posted WQE, all CQEs have been
processed, and page_pool_dev_alloc_frag() returns NULL. The loop then
exits on !RXDONE, poll_again stays false, and nr_pkts < budget is
returned. hinic3_poll() then completes NAPI and re-arms the vector:

drivers/net/ethernet/huawei/hinic3/hinic3_irq.c:hinic3_poll() {
    ...
	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);
	}
    ...
}

With no WQEs posted, the device writes no CQE and raises no interrupt,
so NAPI never runs again.

hinic3_rx_fill_buffers() is only called from hinic3_configure_rxqs() and
hinic3_rx_poll(). hinic3_periodic_work_handler() only logs TX timeout
events. Nothing else refills the ring once memory recovers, and the
queue stays dead until the interface is reset. rx_buf_empty and
alloc_rx_buf_err are also never incremented, so the stall is silent.

i40e/iavf/ice handle this by folding the refill failure into the return
value ("failure = failure || iavf_alloc_rx_buffers(...)"). Could
poll_again also be set when hinic3_rx_fill_buffers() posts fewer than
rxq->delta - 1 buffers, or could a deferred refill be scheduled?

>  }

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

[PATCH net-next v02 2/5] hinic3: Avoid concurrent updates on 32-bit systems.

[Fan Gong]

Within the while loop of hinic3_get_stats64(), the counters between
u64_stats_fetch_retry and u64_stats_fetch_begin are incremented by +=.
This operation will cause repeated counting of the counters on a 32-bit
system. Therefore, change += to + and the counters are incremented
out side the wile loop.

csum_errors and other_errors are not the same things. csum_errors will
go to csum_bad in netdev netlink qstats.

Remove unnecessary checks for the nic_dev->txqs and nic_dev->rxqs.

Remove dead stores before the loop.

Fixes: f47872bed40f ("hinic3: Add .ndo_tx_timeout and .ndo_get_stats64")
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         | 42 +++++++------------
 1 file changed, 14 insertions(+), 28 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..d1fd91d97ccf 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
@@ -791,49 +791,35 @@ static void hinic3_get_stats64(struct net_device *netdev,
 	unsigned int start;
 	int i;
 
-	bytes = 0;
-	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

Re: [PATCH net-next v02 2/5] hinic3: Avoid concurrent updates on 32-bit systems.

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch is titled "Avoid concurrent updates on 32-bit systems",
  says the `+=` inside the u64_stats retry loop of hinic3_get_stats64()…
- [Low] After this patch, struct hinic3_rxq_stats::csum_errors
  (hinic3_rx.h:36) has no consumer anywhere in the tree.

--

hinic3: Avoid concurrent updates on 32-bit systems.

This commit changes hinic3_get_stats64() to read the per-queue counters
by assignment inside the u64_stats retry loop and add them into
rtnl_link_stats64 after the loop. It also removes csum_errors from
rx_errors, removes the NULL checks on nic_dev->txqs and nic_dev->rxqs,
and removes the dead stores before the loops.

> hinic3: Avoid concurrent updates on 32-bit systems.
>
> Within the while loop of hinic3_get_stats64(), the counters between
> u64_stats_fetch_retry and u64_stats_fetch_begin are incremented by +=.
> This operation will cause repeated counting of the counters on a 32-bit
> system. Therefore, change += to + and the counters are incremented
> out side the wile loop.

[ ... ]

> Fixes: f47872bed40f ("hinic3: Add .ndo_tx_timeout and .ndo_get_stats64")

[Severity: Low]
Can the double counting described here happen in this driver?

The hinic3 Kconfig entry only allows 64-bit builds, and COMPILE_TEST
does not remove that limit:

drivers/net/ethernet/huawei/hinic3/Kconfig:
	depends on X86 || ARM64 || COMPILE_TEST
	depends on PCI_MSI && 64BIT

When BITS_PER_LONG == 64, the retry helper always returns false:

include/linux/u64_stats_sync.h:
static inline bool __u64_stats_fetch_retry(const struct u64_stats_sync *syncp,
					   unsigned int start)
{
	return false;
}

So each do/while body in hinic3_get_stats64() runs exactly once, and the
old += loop already gave correct totals.

Even on a hypothetical 32-bit build, nothing would force a retry. The
only u64_stats_update_begin()/u64_stats_update_end() sections in hinic3
seem to be the init-time zeroing helpers hinic3_txq_clean_stats() and
hinic3_rxq_clean_stats().

The new reader pattern is fine as a cleanup. Would it be better to
describe the patch as a cleanup, drop the Fixes: tag and target net-next?

With the tag, a change that has no runtime effect goes to stable. It
would also bring along the NULL check removal, the dead store removal
and the change to what rx_errors reports.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9d..d1fd91d97ccfc 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> @@ -791,49 +791,35 @@ 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: Low]
The commit message says:

    csum_errors and other_errors are not the same things. csum_errors will
    go to csum_bad in netdev netlink qstats.

Does hinic3 have a qstats path for this? I couldn't find netdev_stat_ops,
get_queue_stats_rx or csum_bad anywhere under
drivers/net/ethernet/huawei/hinic3/.

That means this line removes the last reader of csum_errors in struct
hinic3_rxq_stats.

The field has no writer either. hinic3_rx_csum() detects the checksum
error but returns without counting it:

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

The only other reference is the zeroing in hinic3_rxq_clean_stats().

Removing checksum errors from rx_errors does match the rx-csum-bad
definition. Could the commit message be corrected, or the qstats
reporting be added along with an increment in hinic3_rx_csum()?

As things stand, csum_errors is declared and zeroed but never counted
or reported. If a writer is added later, checksum errors still won't
show up in any user-visible interface until qstats support exists.

> +		} 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;

[ ... ]

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

[PATCH net-next v02 3/5] hinic3: fix some checkpatchs.

[Fan Gong]

Fixes: 73f37a7e1993 ("hinic3: Queue pair resource initialization")
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 ++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
index d1fd91d97ccf..1c6cb1149028 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);
 }
 
-- 
2.43.0

Re: [PATCH net-next v02 3/5] hinic3: fix some checkpatchs.

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit "hinic3: fix some checkpatchs." changes only style,
  but it carries `Fixes: 73f37a7e1993 ("hinic3: Queue pair resource…
- [Low] The commit message is only the subject "hinic3: fix some
  checkpatchs." plus the `Fixes:` tag and sign-offs.

--

hinic3: fix some checkpatchs.

This commit renames the trxq_params parameter to q_params in
hinic3_alloc_channel_resources() and hinic3_free_channel_resources().
It also moves the return type of both functions onto its own line.
Nothing else in the code changes.

> hinic3: fix some checkpatchs.

[Severity: Low]
This isn't a bug, but the commit message has nothing besides the subject
and the tags.

It doesn't say which checkpatch warning is being fixed. The old
continuation lines were about 87 columns wide. Default checkpatch
(max_line_length=100) doesn't flag that, so only an 80 column run would
report them.

It also doesn't mention the trxq_params -> q_params rename, or that the
change has no functional effect.

Could a short description be added? checkpatch itself warns when the
commit description is missing.

> Fixes: 73f37a7e1993 ("hinic3: Queue pair resource initialization")

[Severity: Low]
Is the Fixes: tag appropriate here?

In drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c, the diff
renames trxq_params to q_params. It also re-wraps the signatures of
hinic3_alloc_channel_resources() and hinic3_free_channel_resources().

No expressions, calls, ordering or error paths change. The callers are
untouched, and the generated code should be the same.

Commit 73f37a7e1993 did add these functions, but this patch doesn't fix
a bug in them.

The other patches in this series are fixes aimed at net. Examples are
"hinic3: Solve an stall issue in hinic3_rx_poll()" and "hinic3: Avoid
concurrent updates on 32-bit systems.".

With the tag, could this cosmetic change be taken into net? Could stable
or AUTOSEL then pick it up for every tree that contains 73f37a7e1993?

Would it be better to drop this patch from the fix series, or to resend
it to net-next without the Fixes: tag?

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

[PATCH net-next v02 4/5] hinic3: Ensure the value of max_qps is greater than 1

[Fan Gong]

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")
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_hw_cfg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c
index c5dd53a8fa96..65d311454825 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c
@@ -267,7 +267,9 @@ bool hinic3_support_nic(struct hinic3_hwdev *hwdev)
 
 u16 hinic3_func_max_qnum(struct hinic3_hwdev *hwdev)
 {
-	return hwdev->cfg_mgmt->cap.nic_svc_cap.max_sqs;
+	u16 max_qps = hwdev->cfg_mgmt->cap.nic_svc_cap.max_sqs;
+
+	return max_qps <= 1 ? 1 : max_qps;
 }
 
 u8 hinic3_physical_port_id(struct hinic3_hwdev *hwdev)
-- 
2.43.0

Re: [PATCH net-next v02 4/5] hinic3: Ensure the value of max_qps is greater than 1

[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 patch has a Fixes: tag but changes no runtime behaviour.
- [Low] The subject "hinic3: Ensure the value of max_qps is greater than
  1" does not match the code.

--

hinic3: Ensure the value of max_qps is greater than 1

This commit changes hinic3_func_max_qnum() so that the queue count it
returns is never below 1. It has a Fixes: tag for 17fcb3dc12bb and no
commit body.

> hinic3: Ensure the value of max_qps is greater than 1

[Severity: Low]
The subject doesn't match the code. The new expression in
hinic3_func_max_qnum() can still return 1:

	return max_qps <= 1 ? 1 : max_qps;

So the guarantee is max_qps >= 1, not "greater than 1".

The rest of the driver also handles 1 as a valid value.
hinic3_try_to_enable_rss() in hinic3_rss.c skips RSS when max_qps is 1:

	if (nic_dev->max_qps <= 1 ||
	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS))
		goto err_reset_q_params;

The same function also sets nic_dev->max_qps = 1 on its own error paths.

Would "at least 1" or "non-zero" describe the change more accurately?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c
> index c5dd53a8fa96c..65d3114548257 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_cfg.c
> @@ -267,7 +267,9 @@ bool hinic3_support_nic(struct hinic3_hwdev *hwdev)
>  
>  u16 hinic3_func_max_qnum(struct hinic3_hwdev *hwdev)
>  {
> -	return hwdev->cfg_mgmt->cap.nic_svc_cap.max_sqs;
> +	u16 max_qps = hwdev->cfg_mgmt->cap.nic_svc_cap.max_sqs;
> +
> +	return max_qps <= 1 ? 1 : max_qps;

[Severity: Medium]
Can max_sqs actually be 0 when this function is called? The only place
that writes it seems to be hinic3_parse_l2nic_res_cap():

	nic_svc_cap->max_sqs = min(dev_cap->nic_max_sq_id + 1,
				   HINIC3_CFG_MAX_QP);

nic_max_sq_id is a u16, so the + 1 is evaluated as an int and cannot
wrap. HINIC3_CFG_MAX_QP is 256, so max_sqs always ends up between 1 and
256.

max_sqs keeps its zeroed value only when hinic3_support_nic() is false,
because hinic3_parse_dev_cap() then skips the L2NIC parse. In that case
hinic3_nic_probe() returns before it calls hinic3_func_max_qnum():

	if (!hinic3_support_nic(hwdev)) {
		dev_dbg(&adev->dev, "HW doesn't support nic\n");
		return 0;
	}

The other callers, hinic3_init_nicio_res() and hinic3_try_to_enable_rss(),
are only reached later in the same probe. hinic3_init_capability() also
runs only once.

If that's right, the clamp doesn't change any value that can reach it.

Even if 0 did reach this function, the old code would fail cleanly
instead of crashing. alloc_etherdev_mq(sizeof(*nic_dev), 0) returns NULL
and probe fails with -ENOMEM, and hinic3_alloc_qps() rejects
!qp_params->num_qps.

With this change, a 0 from firmware would instead give a device that
silently runs with one queue.

The patch has a Fixes: tag but no commit body. Could the commit message
describe the symptom or firmware condition this fixes, so stable
reviewers can check what bug it closes?

The next patch in the series, "hinic3: Ensure that the value of max_qps is
not changed", changes the RSS check to max_qps == 1 and relies on this
guarantee. The guarantee already seems to hold before this patch, though.

>  }
>  

[ ... ]

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

[PATCH net-next v02 5/5] hinic3: Ensure that the value of max_qps is not changed

[Fan Gong]

nic_dev->max_qps is used to allocate hardware resources. Therefore,
it is assigned the value 1 only when its value is abnormal. In other
scenarios, the value cannot be changed. However, the value of
nic_dev->q_params.num_qps can be dynamically changed.

Fixes: 1f3838b84a63 ("hinic3: Add Rss function")
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_rss.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
index 25db74d8c7dd..75dc0c044071 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
@@ -307,15 +307,13 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)
 	int err;
 
 	nic_dev->max_qps = hinic3_func_max_qnum(hwdev);
-	if (nic_dev->max_qps <= 1 ||
+	if (nic_dev->max_qps == 1 ||
 	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS))
 		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 +322,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 +329,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 v02 5/5] hinic3: Ensure that the value of max_qps is not changed

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch has a Fixes: tag, so it will be treated as a stable
  bug fix.

--

hinic3: Ensure that the value of max_qps is not changed

This patch stops hinic3_try_to_enable_rss() from setting nic_dev->max_qps
to 1 when RSS setup fails. On that path it now sets only
nic_dev->q_params.num_qps to 1. It also changes the max_qps <= 1 check to
max_qps == 1.

> nic_dev->max_qps is used to allocate hardware resources. Therefore,
> it is assigned the value 1 only when its value is abnormal. In other
> scenarios, the value cannot be changed. However, the value of
> nic_dev->q_params.num_qps can be dynamically changed.
>
> Fixes: 1f3838b84a63 ("hinic3: Add Rss function")

[Severity: Medium]
Is this description accurate? The users of nic_dev->max_qps appear to
size only software arrays:

hinic3_alloc_txqs()          -> nic_dev->txqs
hinic3_alloc_rxqs()          -> nic_dev->rxqs
hinic3_init_intr_coalesce()  -> nic_dev->intr_coalesce
hinic3_setup_num_qps()       -> nic_dev->qps_msix_entries

The hardware queue capacity comes from nic_io->max_qps, which is set on
its own:

drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.c:hinic3_init_nic_io() {
    ...
	nic_io->max_qps = hinic3_func_max_qnum(hwdev);
    ...
}

The removed nic_dev->max_qps = 1 assignments never change that value.

What failure does the Fixes: tag refer to? hinic3_try_to_enable_rss() has
one caller, hinic3_sw_init(). It runs before hinic3_alloc_txrxqs(), and
before hinic3_setup_num_qps() at open time. So with the old code, every
array sized from max_qps was sized at 1 after an RSS failure, and the
sizes matched.

At open, hinic3_config_num_qps() also forces num_qps to 1 whenever RSS is
disabled:

drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c:hinic3_config_num_qps() {
    ...
	if (!test_bit(HINIC3_RSS_ENABLE, &nic_dev->flags))
		q_params->num_qps = 1;
    ...
}

So after an RSS failure the device runs one queue, both before and after
this patch.

The only visible change seems to be on the RSS failure path: the
per-queue software arrays are now sized for the hardware maximum instead
of 1.

Could the commit message name the symptom being fixed, or the later change
that needs max_qps to stay at the hardware capacity? If neither exists,
would this fit better in net-next as a cleanup without the Fixes: tag?

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

Re: [PATCH net-next v02 0/5] 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:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

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 v02 0/5] net: hinic3: Bugfix for later patch-set.

[Jakub Kicinski]

On Wed, 11 Nov 2026 04:22:26 +0000 netdev-bot+sinfo@kernel.org wrote:
> If the series needs another revision for other reasons, please
> include the information in the commit messages then.

You're supposed to add the info to the cover letter when you post
new revision. Please read the responses in full :/