Re: [PATCH net 2/2] bnxt_en: Wait for the PF in bnxt_resume()

[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 uses two things as the PF-ready barrier for a VF
  resuming in the same kernel: a successful HWRM_VER_GET poll, then…
- [Low] The failure that the commit message and the new comment in
  bnxt_resume() describe is not the failure shown in the logs.

--

bnxt_en: Wait for the PF in bnxt_resume()

This patch changes bnxt_resume() on a VF. Before taking rtnl_lock() and
netdev_lock(), the VF now polls firmware with bnxt_hwrm_poll() up to 10
times, so that a PF in the same kernel can finish initializing first.
The retry delay depends on the error code: 250 ms for -EAGAIN, 750 ms
for -ECOMM, and no delay for -EBUSY.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 9d232d8357587..03682d9182c6d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> +	if (BNXT_VF(bp)) {
> +		for (i = 0; i < BNXT_RESUME_MAX_RETRY; i++) {
> +			rc = bnxt_hwrm_poll(bp);
> +			if (rc != -EAGAIN && rc != -EBUSY && rc != -ECOMM)
> +				break;
> +
> +			/* Wait briefly for -EAGAIN (FW busy), a bit longer for
> +			 * -ECOMM (PF down) to allow the PF to fully initialize.
> +			 * For -EBUSY, the message typically has timed out, so
> +			 * try again immediately.
> +			 */
> +			if (i < BNXT_RESUME_MAX_RETRY - 1) {
> +				if (rc == -EAGAIN)
> +					msleep(250);
> +				else if (rc == -ECOMM)
> +					msleep(750);
> +			}
> +		}

[Severity: Low]
Do the commit message and this comment describe the failure that the logs
actually show?

The commit message says:

  the initial FW message may timeout if the PF has not been initialized.

But every quoted log line is:

  hwrm req_type 0x0 seq id 0xb0 error 0xa

That message comes from the firmware error response branch of
__hwrm_send(). It is printed only after firmware returned a valid response
with a non-zero error_code. A timeout would print "Error (timeout: ...)"
and return -EBUSY instead.

Error 0xa is HWRM_ERR_CODE_HOT_RESET_PROGRESS, and __hwrm_to_stderr() maps
it to -EAGAIN:

drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c:__hwrm_to_stderr() {
    ...
	case HWRM_ERR_CODE_HOT_RESET_PROGRESS:
	case HWRM_ERR_CODE_BUSY:
		return -EAGAIN;
    ...
	case HWRM_ERR_CODE_PF_UNAVAILABLE:
		return -ECOMM;
    ...
}

The kernel-doc for hwrm_req_send() also describes -EAGAIN as "HWRM busy or
reset in progress".

The new comment labels -EAGAIN as only "FW busy" and gives it the shortest
backoff. The longer "allow the PF to fully initialize" wait goes to -ECOMM.
So the observed failure goes through the 250 ms -EAGAIN branch, with a
total budget of about 9 x 250 ms.

Could the commit message and the comment be updated so they name the
hot-reset-in-progress (-EAGAIN) case as the condition being fixed?

> +		if (rc) {
> +			netdev_err(dev,
> +				   "Firmware not responding during resume, rc = %d\n",
> +				   rc);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	rtnl_lock();
> +	netdev_lock(dev);
> +
>  	if (bnxt_hwrm_ver_get(bp)) {
>  		rc = -ENODEV;
>  		goto resume_exit;

[Severity: Medium]
Is rtnl_lock() enough to make the VF wait until the PF has finished
initializing?

On the PF side, the end of bnxt_resume() drops both locks before it
reconfigures SR-IOV:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_resume() {
    ...
resume_exit:
	netdev_unlock(bp->dev);
	rtnl_unlock();
	if (!rc) {
		bnxt_ulp_start(bp);
		bnxt_reenable_sriov(bp);
	}
	return rc;
}

bnxt_reenable_sriov() calls bnxt_cfg_hw_sriov(bp, &n, true) with neither
rtnl nor netdev_lock held. That path does this:

bnxt_cfg_hw_sriov()
    bnxt_hwrm_func_buf_rgtr()
    bnxt_func_cfg()
        bnxt_hwrm_func_vf_resc_cfg(..., reset=true)

It registers the VF request forwarding buffers and reserves per-VF
resources with HWRM_FUNC_VF_RESOURCE_CFG.

A VF that has passed the poll and is blocked in rtnl_lock() gets the lock
as soon as the PF drops it. It can then run bnxt_hwrm_ver_get(),
bnxt_hwrm_func_reset(), bnxt_hwrm_func_qcaps(),
bnxt_hwrm_func_drv_rgtr() and bnxt_open(). All of these can run before or
during the PF's bnxt_cfg_hw_sriov().

In that window, could the VF query its resource limits and reserve rings
before the PF has restored the VF min/max resources and forwarding buffers
after the hibernation reset? If so, bnxt_open() might fail ring reservation
or see stale limits, so the "PF not initialized" failure could still happen
in later commands.

Would it need explicit ordering to close this window? For example, the PF
could reconfigure SR-IOV before releasing the lock, or the VF could wait
for a PF-side SR-IOV ready condition.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110174943.2106347-1-michael.chan%40broadcom.com