[PATCH net v2 0/2] net: ngbe: fix error handling in resume and open paths

[PATCH net v2 0/2] net: ngbe: fix error handling in resume and open paths

[Zhang Yunfei]

Two error-handling fixes for the ngbe PM/open paths.

ngbe_resume() declared err as u32 and returned 0 unconditionally, so a
failed ngbe_reset_hw(), wx_init_interrupt_scheme() or ngbe_open() left
the device in netif_device_detach() state with a broken interrupt
scheme while the PM core was told the resume succeeded; the reset task
bails out on the missing netif_device_present() check, so the device
cannot self-heal. Patch 1 fixes the type and propagates all of these
errors, making the whole tail of the resume path consistent with the
pci_enable_device_mem() failure path at the top, which already reports
its error.

ngbe_open() sets the WX_CFG_PORT_CTL_DRV_LOAD bit to tell the
management firmware the host has taken over the port, but no error
path cleared it, leaving the firmware owning a port whose rings and
IRQs are gone. Patch 2 rolls the bit back on all open error paths,
matching ngbe_close() and ngbe_dev_shutdown().

---
Changes in v2:
- also propagate the ngbe_reset_hw() failure, so the whole tail of
  ngbe_resume() reports errors to the PM core (Sashiko review);
- drop the inaccurate "device can be re-probed" claim: the PM core
  records and logs the failure, there is no re-probe (Sashiko review);
- patch 2/2 unchanged.

Link: https://lore.kernel.org/netdev/20260917090050.1927999-1-zhangyunfei1@kylinos.cn/T/#u/


Zhang Yunfei (2):
  net: ngbe: propagate resume errors to the PM core
  net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)


base-commit: 93f51579e7df248780214094418f205253383cc5
-- 
2.25.1

[PATCH net v2 1/2] net: ngbe: propagate resume errors to the PM core

[Zhang Yunfei]

ngbe_resume() declares err as u32 and unconditionally returns 0, so
failures of wx_init_interrupt_scheme() or ngbe_open() are silently
swallowed, and the return value of ngbe_reset_hw() is ignored
entirely. The device stays in netif_device_detach() state with a
broken interrupt scheme, the PM core is told the resume succeeded,
and the netdev never appears in the networking stack again: the
reset task also bails out early on the missing
netif_device_present() check, so the device cannot self-heal.

Fix the type to int and propagate the errors instead, making the
whole tail of the resume path consistent with the
pci_enable_device_mem() failure path at the top, which already
propagates its error. If the hardware reset fails, the remaining
resume steps cannot succeed, so return early instead of continuing
with a broken device. A failed resume is then reported to the PM
core, which records and logs the failure, instead of being silently
swallowed.

Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
---

Changes in v2:
- also propagate the ngbe_reset_hw() failure, so the whole tail of
  ngbe_resume() reports errors to the PM core;
- drop the inaccurate "device can be re-probed" claim: the PM core
  records and logs the failure, there is no re-probe.

 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index 855dc963c610..e8cabcc84a41 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -954,7 +954,7 @@ static int ngbe_resume(struct pci_dev *pdev)
 {
 	struct net_device *netdev;
 	struct wx *wx;
-	u32 err;
+	int err;
 
 	wx = pci_get_drvdata(pdev);
 	netdev = wx->netdev;
@@ -968,7 +968,9 @@ static int ngbe_resume(struct pci_dev *pdev)
 	pci_set_master(pdev);
 	device_wakeup_disable(&pdev->dev);
 
-	ngbe_reset_hw(wx);
+	err = ngbe_reset_hw(wx);
+	if (err)
+		return err;
 	rtnl_lock();
 	err = wx_init_interrupt_scheme(wx);
 	if (!err && netif_running(netdev))
@@ -977,7 +979,7 @@ static int ngbe_resume(struct pci_dev *pdev)
 		netif_device_attach(netdev);
 	rtnl_unlock();
 
-	return 0;
+	return err;
 }
 
 static struct pci_driver ngbe_driver = {
-- 
2.25.1

RE: [PATCH net v2 1/2] net: ngbe: propagate resume errors to the PM core

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Zhang Yunfei <zhangyunfei1@kylinos.cn>
> Sent: Tuesday, September 22, 2026 12:09 PM
> To: netdev@vger.kernel.org
> Cc: jiawenwu@trustnetic.com; mengyuanlou@net-swift.com;
> andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; leitao@debian.org;
> weirongguang@kylinos.cn; u.kleine-koenig@baylibre.com; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: [PATCH net v2 1/2] net: ngbe: propagate resume errors to the
> PM core
> 
> ngbe_resume() declares err as u32 and unconditionally returns 0, so
> failures of wx_init_interrupt_scheme() or ngbe_open() are silently
> swallowed, and the return value of ngbe_reset_hw() is ignored
> entirely. The device stays in netif_device_detach() state with a
> broken interrupt scheme, the PM core is told the resume succeeded, and
> the netdev never appears in the networking stack again: the reset task
> also bails out early on the missing
> netif_device_present() check, so the device cannot self-heal.
> 
> Fix the type to int and propagate the errors instead, making the whole
> tail of the resume path consistent with the
> pci_enable_device_mem() failure path at the top, which already
> propagates its error. If the hardware reset fails, the remaining
> resume steps cannot succeed, so return early instead of continuing
> with a broken device. A failed resume is then reported to the PM core,
> which records and logs the failure, instead of being silently
> swallowed.
> 
> Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
> ---
> 
> Changes in v2:
> - also propagate the ngbe_reset_hw() failure, so the whole tail of
>   ngbe_resume() reports errors to the PM core;
> - drop the inaccurate "device can be re-probed" claim: the PM core
>   records and logs the failure, there is no re-probe.
> 
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 855dc963c610..e8cabcc84a41 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -954,7 +954,7 @@ static int ngbe_resume(struct pci_dev *pdev)  {
>  	struct net_device *netdev;
>  	struct wx *wx;
> -	u32 err;
> +	int err;
> 
>  	wx = pci_get_drvdata(pdev);
>  	netdev = wx->netdev;
> @@ -968,7 +968,9 @@ static int ngbe_resume(struct pci_dev *pdev)
>  	pci_set_master(pdev);
>  	device_wakeup_disable(&pdev->dev);
> 
> -	ngbe_reset_hw(wx);
> +	err = ngbe_reset_hw(wx);
> +	if (err)
> +		return err;
>  	rtnl_lock();
>  	err = wx_init_interrupt_scheme(wx);
>  	if (!err && netif_running(netdev))
> @@ -977,7 +979,7 @@ static int ngbe_resume(struct pci_dev *pdev)
>  		netif_device_attach(netdev);
>  	rtnl_unlock();
> 
> -	return 0;
> +	return err;
>  }
> 
>  static struct pci_driver ngbe_driver = {
> --
> 2.25.1


Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH net v2 1/2] net: ngbe: propagate resume errors to the PM core

[Breno Leitao]

On Tue, Sep 22, 2026 at 06:08:35PM +0800, Zhang Yunfei wrote:
> ngbe_resume() declares err as u32 and unconditionally returns 0, so
> failures of wx_init_interrupt_scheme() or ngbe_open() are silently
> swallowed, and the return value of ngbe_reset_hw() is ignored
> entirely. The device stays in netif_device_detach() state with a
> broken interrupt scheme, the PM core is told the resume succeeded,
> and the netdev never appears in the networking stack again: the
> reset task also bails out early on the missing
> netif_device_present() check, so the device cannot self-heal.
> 
> Fix the type to int and propagate the errors instead, making the
> whole tail of the resume path consistent with the
> pci_enable_device_mem() failure path at the top, which already
> propagates its error. If the hardware reset fails, the remaining
> resume steps cannot succeed, so return early instead of continuing
> with a broken device. A failed resume is then reported to the PM
> core, which records and logs the failure, instead of being silently
> swallowed.
> 
> Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net v2 1/2] net: ngbe: propagate resume errors to the PM core

[netdev-bot+sashiko]

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

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

- [High] The newly added early return `err = ngbe_reset_hw(wx); if (err)
  return err;` in ngbe_resume()…

--

net: ngbe: propagate resume errors to the PM core

ngbe_resume() used a u32 err and always returned 0, so resume failures
were never reported.  The patch changes err to int, propagates the
wx_init_interrupt_scheme()/ngbe_open() error, and adds a new early
return when ngbe_reset_hw() fails.

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 855dc963c610c..e8cabcc84a414 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -954,7 +954,7 @@ static int ngbe_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *netdev;
>  	struct wx *wx;
> -	u32 err;
> +	int err;
>  
>  	wx = pci_get_drvdata(pdev);
>  	netdev = wx->netdev;
> @@ -968,7 +968,9 @@ static int ngbe_resume(struct pci_dev *pdev)
>  	pci_set_master(pdev);
>  	device_wakeup_disable(&pdev->dev);
>  
> -	ngbe_reset_hw(wx);
> +	err = ngbe_reset_hw(wx);
> +	if (err)
> +		return err;

[Severity: High]
Can this new bail-out leave the driver state claiming resources it no
longer owns?

The suspend side calls ngbe_close() directly rather than dev_close():

drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:ngbe_dev_shutdown() {
	...
	if (netif_running(netdev))
		ngbe_close(netdev);
	wx_clear_interrupt_scheme(wx);
	...
}

So __LINK_STATE_START is never cleared and netif_running(netdev) stays
true, while wx_clear_interrupt_scheme() zeroes wx->num_tx_queues /
num_rx_queues / num_q_vectors, NULLs wx->tx_ring[]/rx_ring[] in
wx_free_q_vector(), and calls pci_free_irq_vectors() so
pdev->msix_enabled and msi_enabled become false.

Returning here skips wx_init_interrupt_scheme(), so none of that is
rebuilt.  ngbe_close()'s only idempotency guard is WX_STATE_RES_FREED,
and that bit is set only by ngbe_down_suspend() (the PCIe error
recovery path), not by ngbe_dev_shutdown():

drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:ngbe_close() {
	if (test_bit(WX_STATE_RES_FREED, wx->state))
		goto out;

	wx_ptp_stop(wx);
	ngbe_down(wx);
	wx_free_irq(wx);
	...
}

Does that mean the next ndo_stop - "ip link set <dev> down", a second
ngbe_dev_shutdown() (netif_running() is still true), or
unregister_netdev() from ngbe_remove() on rmmod - re-runs the full
teardown on already-freed state and hits the unbalanced free_irq()
here?

drivers/net/ethernet/wangxun/libwx/wx_lib.c:wx_free_irq() {
	...
	if (!(pdev->msix_enabled)) {
		if (!wx->misc_irq_domain)
			free_irq(pdev->irq, wx);
		return;
	}
	...
}

With msix_enabled false and wx->misc_irq_domain never set on ngbe (only
txgbe sets it), free_irq(pdev->irq, wx) is issued for an IRQ that has no
registered action, which gives the "Trying to free already-free IRQ"
WARN, or a panic with panic_on_warn.

Separately, ngbe_set_ringparam() is gated on netif_running() only, not
netif_device_present():

drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c:ngbe_set_ringparam() {
	...
	if (!netif_running(wx->netdev)) {
	...
}

After the failed resume it takes the "running" branch and drives
ngbe_down()/ngbe_up() with zero q_vectors and no IRQ vectors, and
ngbe_up_complete() clears WX_STATE_DOWN and restarts the tx queues of a
detached netdev whose tx_ring[] entries are NULL and whose
wx->num_tx_queues is 0, which wx_xmit_frame() then uses in
r_idx % wx->num_tx_queues.

Would it be better to leave a consistent representation before
returning, for example mirroring ngbe_down_suspend() by setting
WX_STATE_RES_FREED, or keeping the interrupt scheme initialised, or
marking the interface down, so the second close is a no-op?

ngbe_reset_hw() can fail through wx_stop_adapter() ->
wx_disable_pcie_master() returning -ETIMEDOUT, or through the
read_poll_timeout() on 0x10028, so this is reachable on a hardware or
timing fault and the follow-up ifdown/rmmod is an ordinary operation
once the NIC looks dead.

The same double-teardown hazard already exists on the pre-existing
wx_init_interrupt_scheme()/ngbe_open() failure paths below, but this
patch adds a new route into it.  The only follow-up patch found, "net:
ngbe: clear DRV_LOAD bit when ngbe_open() fails", touches ngbe_open()
only and leaves ngbe_resume() unchanged.

>  	rtnl_lock();
>  	err = wx_init_interrupt_scheme(wx);
>  	if (!err && netif_running(netdev))

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922100836.1147718-1-zhangyunfei1%40kylinos.cn

[PATCH net v2 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

[Zhang Yunfei]

ngbe_open() sets the WX_CFG_PORT_CTL_DRV_LOAD bit via
wx_control_hw(wx, true) to tell the management firmware that the
host driver has taken over the port (NCSI/OOB firmware stops using
its management channel). Every error path of ngbe_open() returns
without clearing it, leaving rings, IRQs and the PHY torn down
while the firmware still believes the host owns the port, an
inconsistent driver-firmware handshake state that persists until
the next successful ifup.

Roll the bit back on all open error paths, matching ngbe_close()
and ngbe_dev_shutdown(), which already clear it.

Fixes: e7956139a6cf ("net: ngbe: Add irqs request flow")
Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
---
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index e8cabcc84a41..7e2cc69fe8f8 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -494,7 +494,7 @@ static int ngbe_open(struct net_device *netdev)
 
 	err = wx_setup_resources(wx);
 	if (err)
-		return err;
+		goto err_control_hw;
 
 	wx_configure(wx);
 
@@ -526,6 +526,8 @@ static int ngbe_open(struct net_device *netdev)
 err_free_resources:
 	wx_free_isb_resources(wx);
 	wx_free_resources(wx);
+err_control_hw:
+	wx_control_hw(wx, false);
 	return err;
 }
 
-- 
2.25.1

Re: [PATCH net v2 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

[Breno Leitao]

On Tue, Sep 22, 2026 at 06:08:36PM +0800, Zhang Yunfei wrote:
> ngbe_open() sets the WX_CFG_PORT_CTL_DRV_LOAD bit via
> wx_control_hw(wx, true) to tell the management firmware that the
> host driver has taken over the port (NCSI/OOB firmware stops using
> its management channel). Every error path of ngbe_open() returns
> without clearing it, leaving rings, IRQs and the PHY torn down
> while the firmware still believes the host owns the port, an
> inconsistent driver-firmware handshake state that persists until
> the next successful ifup.
> 
> Roll the bit back on all open error paths, matching ngbe_close()
> and ngbe_dev_shutdown(), which already clear it.

silly question, why unwind, rather than not taking hw control until the
port is actually up, as the last thing in ngbe_open()?

Looking at txgbe, it does something like:

	static int txgbe_open(struct net_device *netdev) {
	.....
		txgbe_up_complete(wx);

		return 0;
	}

Would ngbe_open() be able to follow a similar approach?

--breno

RE: [PATCH net v2 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Zhang Yunfei <zhangyunfei1@kylinos.cn>
> Sent: Tuesday, September 22, 2026 12:09 PM
> To: netdev@vger.kernel.org
> Cc: jiawenwu@trustnetic.com; mengyuanlou@net-swift.com;
> andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; leitao@debian.org;
> weirongguang@kylinos.cn; u.kleine-koenig@baylibre.com; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: [PATCH net v2 2/2] net: ngbe: clear DRV_LOAD bit when
> ngbe_open() fails
> 
> ngbe_open() sets the WX_CFG_PORT_CTL_DRV_LOAD bit via
> wx_control_hw(wx, true) to tell the management firmware that the host
> driver has taken over the port (NCSI/OOB firmware stops using its
> management channel). Every error path of ngbe_open() returns without
> clearing it, leaving rings, IRQs and the PHY torn down while the
> firmware still believes the host owns the port, an inconsistent
> driver-firmware handshake state that persists until the next
> successful ifup.
> 
> Roll the bit back on all open error paths, matching ngbe_close() and
> ngbe_dev_shutdown(), which already clear it.
> 
> Fixes: e7956139a6cf ("net: ngbe: Add irqs request flow")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
> ---
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index e8cabcc84a41..7e2cc69fe8f8 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -494,7 +494,7 @@ static int ngbe_open(struct net_device *netdev)
> 
>  	err = wx_setup_resources(wx);
>  	if (err)
> -		return err;
> +		goto err_control_hw;
> 
>  	wx_configure(wx);
> 
> @@ -526,6 +526,8 @@ static int ngbe_open(struct net_device *netdev)
>  err_free_resources:
>  	wx_free_isb_resources(wx);
>  	wx_free_resources(wx);
> +err_control_hw:
> +	wx_control_hw(wx, false);
>  	return err;
>  }
> 
> --
> 2.25.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH net v2 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

[Zhang Yunfei]

Resend with the full Cc list; the previous reply inadvertently dropped
the Cc recipients.

Hi Breno,

Not silly at all -- txgbe's approach is indeed cleaner, and you read it
right: wx_control_hw(wx, true) lives at the top of txgbe_up_complete()
(txgbe_main.c:160), so txgbe's open error paths never have to unwind
the bit.

For ngbe I kept the claim where the vendor put it (top of ngbe_open())
and added the missing rollback, because moving the claim into
ngbe_up_complete() changes the driver-firmware handshake timing: the
whole setup/configure window would then run while the NCSI/OOB
firmware still believes it owns the port. Whether ngbe's management
firmware tolerates that window is a firmware-contract question I
cannot answer from the code alone (txgbe is a different product line),
so Jiawen/Mengyuan are better placed to judge it. Deferring the claim
into ngbe_up_complete() would look like the cleaner long-term shape;
this patch only closes the existing inconsistency.

Thanks for the suggestion!

Zhang Yunfei