[PATCH net 0/2] bnxt_en: Fix PM resume on VFs

[PATCH net 0/2] bnxt_en: Fix PM resume on VFs

[Michael Chan]

The 2 patches fix Power Management resume on VFs when the PF is running
in the same kernel.  The first patch maps the specific PF_DOWN FW error
code to a unique errno.  The second patch adds pre-polling on a VF
during PM resume when the error warrants further polling.  This will
allow the PF to finish resume before the VF continues.

This issue was found in the lab when testing system hibernate on a
BCM57608 NIC with 64 VFs created.  The FW version is 238.1.59.0.

Keegan Freyhof (2):
  bnxt_en: Map HWRM_ERR_CODE_PF_UNAVAILABLE to -ECOMM
  bnxt_en: Wait for the PF in bnxt_resume()

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 51 +++++++++++++++----
 .../net/ethernet/broadcom/bnxt/bnxt_hwrm.c    |  4 +-
 2 files changed, 43 insertions(+), 12 deletions(-)

-- 
2.51.0

[PATCH net 1/2] bnxt_en: Map HWRM_ERR_CODE_PF_UNAVAILABLE to -ECOMM

[Michael Chan]

From: Keegan Freyhof <keegan.freyhof@broadcom.com>

The current use of -ENODEV for both PCI being disabled and FW's
HWRM_ERR_CODE_PF_UNAVAILABLE errors makes it impossible for the
caller to distinguish between the 2 errors.  As a result, a disabled
VF device will cause the driver to pointlessly retry setting L2
filters believing that the PF is down.

Fix it by using -ECOMM instead for HWRM_ERR_CODE_PF_UNAVAILABLE
which means that the FW is failing the command on the VF because the
PF is down.  This new -ECOMM error code which is more accurate will
now propagate to userspace instead of -ENODEV on some VF failure
paths when the PF is down.

Fixes: 662c9b22f5b5 ("bnxt_en: improve VF error messages when PF is unavailable")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Keegan Freyhof <keegan.freyhof@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c      | 14 +++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c |  4 +++-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 33c4cacc8ffe..9d232d835758 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11319,7 +11319,7 @@ static int bnxt_init_chip(struct bnxt *bp, bool irq_re_init)
 	/* Filter for default vnic 0 */
 	rc = bnxt_hwrm_set_vnic_filter(bp, 0, 0, bp->dev->dev_addr);
 	if (rc) {
-		if (BNXT_VF(bp) && rc == -ENODEV)
+		if (rc == -ECOMM)
 			netdev_err(bp->dev, "Cannot configure L2 filter while PF is unavailable\n");
 		else
 			netdev_err(bp->dev, "HWRM vnic filter failure rc: %x\n", rc);
@@ -12477,7 +12477,7 @@ int bnxt_update_link(struct bnxt *bp, bool chng_link_state)
 	rc = hwrm_req_send(bp, req);
 	if (rc) {
 		hwrm_req_drop(bp, req);
-		if (BNXT_VF(bp) && rc == -ENODEV) {
+		if (rc == -ECOMM) {
 			netdev_warn(bp->dev, "Cannot obtain link state while PF unavailable.\n");
 			rc = 0;
 		}
@@ -13954,7 +13954,7 @@ static int bnxt_cfg_rx_mode(struct bnxt *bp, struct netdev_hw_addr_list *uc,
 	for (i = 1, off = 0; i < vnic->uc_filter_count; i++, off += ETH_ALEN) {
 		rc = bnxt_hwrm_set_vnic_filter(bp, 0, i, vnic->uc_list + off);
 		if (rc) {
-			if (BNXT_VF(bp) && rc == -ENODEV) {
+			if (rc == -ECOMM) {
 				netdev_warn(bp->dev, "Cannot configure L2 filters while PF is unavailable, will retry\n");
 				rc = -EAGAIN;
 			} else if (rc == -EAGAIN) {
@@ -16997,7 +16997,7 @@ static int bnxt_set_dflt_rings(struct bnxt *bp, bool sh)
 	}
 
 	rc = __bnxt_reserve_rings(bp);
-	if (rc && rc != -ENODEV)
+	if (rc && rc != -ECOMM)
 		netdev_warn(bp->dev, "Unable to reserve tx rings\n");
 
 	bnxt_adj_tx_rings(bp);
@@ -17007,7 +17007,7 @@ static int bnxt_set_dflt_rings(struct bnxt *bp, bool sh)
 	/* Rings may have been trimmed, re-reserve the trimmed rings. */
 	if (bnxt_need_reserve_rings(bp)) {
 		rc = __bnxt_reserve_rings(bp);
-		if (rc && rc != -ENODEV)
+		if (rc && rc != -ECOMM)
 			netdev_warn(bp->dev, "2nd rings reservation failed.\n");
 		bnxt_adj_tx_rings(bp);
 	}
@@ -17033,7 +17033,7 @@ static int bnxt_init_dflt_ring_mode(struct bnxt *bp)
 	bnxt_clear_int_mode(bp);
 	rc = bnxt_set_dflt_rings(bp, true);
 	if (rc) {
-		if (BNXT_VF(bp) && rc == -ENODEV)
+		if (rc == -ECOMM)
 			netdev_err(bp->dev, "Cannot configure VF rings while PF is unavailable.\n");
 		else
 			netdev_err(bp->dev, "Not enough rings available.\n");
@@ -17411,7 +17411,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		bnxt_aux_devices_init(bp);
 	rc = bnxt_set_dflt_rings(bp, true);
 	if (rc) {
-		if (BNXT_VF(bp) && rc == -ENODEV) {
+		if (rc == -ECOMM) {
 			netdev_err(bp->dev, "Cannot configure VF rings while PF is unavailable.\n");
 		} else {
 			netdev_err(bp->dev, "Not enough rings available.\n");
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
index 5bfabdca7d0e..de02d1baa751 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
@@ -360,7 +360,7 @@ static int __hwrm_to_stderr(u32 hwrm_err)
 	case HWRM_ERR_CODE_CMD_NOT_SUPPORTED:
 		return -EOPNOTSUPP;
 	case HWRM_ERR_CODE_PF_UNAVAILABLE:
-		return -ENODEV;
+		return -ECOMM;
 	default:
 		return -EIO;
 	}
@@ -716,6 +716,8 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
  *	ENOMEM: HWRM has no buffers.
  *	EAGAIN: HWRM busy or reset in progress.
  *	EOPNOTSUPP: Invalid request type.
+ *	ENODEV: PCI device is disabled.
+ *	ECOMM: The parent PF is unavailable when issued on VFs.
  *	EIO: Any other error.
  * Error handling is orthogonal to request ownership. An unowned request will
  * still be consumed on error. If the caller owns the request, then the caller
-- 
2.51.0

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

[Michael Chan]

From: Keegan Freyhof <keegan.freyhof@broadcom.com>

When bnxt_resume() is called on a VF with the PF running in the
same kernel, the initial FW message may timeout if the PF has not
been initialized.  Use the more appropriate bnxt_hwrm_poll() with
the silent flag to suppress warning messages and loop up to 10 times
if the error code warrants additional polling.  This initial polling
is done without the rtnl_lock() to allow the PF running in the same
kernel (which will also require the rtnl_lock) to finish its
initialization.

This fixes a failure found in the lab when running system hibernate
on a BCM57608 NIC with 64 VFs created.  The FW version is 238.1.59.0.

bnxt_en 0000:23:08.5 eno4v5: hwrm req_type 0x0 seq id 0xb0 error 0xa
bnxt_en 0000:23:08.5: PM: dpm_run_callback(): pci_pm_restore returns -19
bnxt_en 0000:23:08.5: PM: failed to restore async: error -19
bnxt_en 0000:23:08.3 eno4v3: hwrm req_type 0x0 seq id 0xb0 error 0xa
bnxt_en 0000:23:08.4 eno4v4: hwrm req_type 0x0 seq id 0xb0 error 0xa
bnxt_en 0000:23:08.7 eno4v7: hwrm req_type 0x0 seq id 0xb0 error 0xa
bnxt_en 0000:23:08.3: PM: dpm_run_callback(): pci_pm_restore returns -19
bnxt_en 0000:23:08.4: PM: dpm_run_callback(): pci_pm_restore returns -19

Tested on dual-port BCM57608 with 64 VFs on each port.  The polling
loop provides adequate margins.

Fixes: f65a2044a8c9 ("bnxt_en: Add suspend/resume callbacks.")
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Keegan Freyhof <keegan.freyhof@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 37 ++++++++++++++++++++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 9d232d835758..03682d9182c6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -17567,21 +17567,50 @@ static int bnxt_suspend(struct device *device)
 	return rc;
 }
 
+#define BNXT_RESUME_MAX_RETRY	10
+
 static int bnxt_resume(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct bnxt *bp = netdev_priv(dev);
-	int rc = 0;
+	int i, rc;
 
-	rtnl_lock();
-	netdev_lock(dev);
 	rc = pci_enable_device(bp->pdev);
 	if (rc) {
 		netdev_err(dev, "Cannot re-enable PCI device during resume, err = %d\n",
 			   rc);
-		goto resume_exit;
+		return rc;
 	}
 	pci_set_master(bp->pdev);
+	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);
+			}
+		}
+		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;
-- 
2.51.0

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

[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 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