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

[netdev-bot+sashiko@kernel.org]

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