mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths
@ 2026-09-30  9:47 Zhang Yunfei
  2026-09-30  9:47 ` [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core Zhang Yunfei
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Zhang Yunfei @ 2026-09-30  9:47 UTC (permalink / raw)
  To: netdev
  Cc: jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet, kuba,
	pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	zhangyunfei1, linux-kernel, stable, leitao

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 v3:
- patch 1: set WX_STATE_RES_FREED on every failing return of
  ngbe_resume() (the pci_enable_device_mem() failure, the hardware
  reset failure early return and the wx_init_interrupt_scheme()/
  ngbe_open() failures), so that a later ngbe_close() (ndo_stop or
  unregister_netdev()) skips re-running the teardown on the
  already-freed post-suspend state instead of freeing IRQs that are no
  longer requested (Sashiko review of v2);
- patch 2: correct the Fixes tag to a1cf597b99a7, the commit that
  introduced the bug (the first ngbe_open() failure path after the
  DRV_LOAD bit is set; v2 pointed at e7956139a6cf, which added more
  failing returns but not the first one); no code change.

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 of v1);
- drop the inaccurate "device can be re-probed" claim: the PM core
  records and logs the failure, there is no re-probe (Sashiko review
  of v1);
- patch 2/2 unchanged.

Link: https://lore.kernel.org/netdev/20260922100836.1147718-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 | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)


base-commit: 93f51579e7df248780214094418f205253383cc5
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core
  2026-09-30  9:47 [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Zhang Yunfei
@ 2026-09-30  9:47 ` Zhang Yunfei
  2026-09-30 20:00   ` Joe Damato
  2026-10-04 10:20   ` netdev-bot+sashiko
  2026-09-30  9:47 ` [PATCH net v3 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails Zhang Yunfei
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 8+ messages in thread
From: Zhang Yunfei @ 2026-09-30  9:47 UTC (permalink / raw)
  To: netdev
  Cc: jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet, kuba,
	pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	zhangyunfei1, linux-kernel, stable, leitao, Sashiko

After a failed resume from suspend, the device stays detached from
the networking stack: every attempt to bring the interface up fails
the netif_device_present() check in __dev_open() with -ENODEV,
while the PM core is told that resume succeeded.

ngbe_resume() declares err as u32 and unconditionally returns 0:
the return value of ngbe_reset_hw() is ignored entirely, and
failures of wx_init_interrupt_scheme() and ngbe_open() are silently
swallowed. The interrupt scheme torn down at suspend is never
rebuilt, so the device cannot self-heal.

Fix the type to int and propagate the errors, making the whole tail
of the resume path consistent with the pci_enable_device_mem()
failure path at the top. If the hardware reset fails, return early:
the remaining resume steps cannot succeed. The suspend path may
already have torn the interface down (ngbe_close() and
wx_clear_interrupt_scheme()), leaving freed rings and IRQs behind
while netif_running() still reports true, so set WX_STATE_RES_FREED
on every failing return, the same mark ngbe_down_suspend() uses for
the PCI error recovery path: a later ngbe_close() skips the
teardown of the already-freed state, and ngbe_up_complete() clears
the bit again so later opens are unaffected. This is safe on the
wx_init_interrupt_scheme() failure path too, as it cleans up after
itself.

Found by manual code inspection of the PM error paths; the missing
ngbe_reset_hw() propagation was reported by Sashiko in its review
of v1. The failure paths are unreachable without fault injection:
a loadable test module injects failures into ngbe_reset_hw(),
wx_init_interrupt_scheme() and ngbe_open(). In a QEMU VM the
unfixed driver hits the kernel "Trying to free already-free IRQ"
warning on the post-suspend close; with the fix, the error is
reported and no warning appears. No physical ngbe device is
involved.

Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917090050.1927999-1-zhangyunfei1%40kylinos.cn
Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
---
Changes in v3:
- set WX_STATE_RES_FREED on every failing return of ngbe_resume()
  (the pci_enable_device_mem() failure, the reset failure early return
  and the wx_init_interrupt_scheme()/ngbe_open() failures), so that a
  later ngbe_close() skips re-running the teardown on the already-freed
  post-suspend state (Sashiko review of v2);

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 | 14 +++++++++++---
 1 file changed, 11 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..8247f6c14be0 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -954,13 +954,14 @@ 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;
 
 	err = pci_enable_device_mem(pdev);
 	if (err) {
+		set_bit(WX_STATE_RES_FREED, wx->state);
 		wx_err(wx, "Cannot enable PCI device from suspend\n");
 		return err;
 	}
@@ -968,16 +969,23 @@ 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) {
+		set_bit(WX_STATE_RES_FREED, wx->state);
+		wx_err(wx, "Hardware reset failed: %d\n", err);
+		return err;
+	}
 	rtnl_lock();
 	err = wx_init_interrupt_scheme(wx);
 	if (!err && netif_running(netdev))
 		err = ngbe_open(netdev);
 	if (!err)
 		netif_device_attach(netdev);
+	else
+		set_bit(WX_STATE_RES_FREED, wx->state);
 	rtnl_unlock();
 
-	return 0;
+	return err;
 }
 
 static struct pci_driver ngbe_driver = {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net v3 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails
  2026-09-30  9:47 [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Zhang Yunfei
  2026-09-30  9:47 ` [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core Zhang Yunfei
@ 2026-09-30  9:47 ` Zhang Yunfei
  2026-09-30 19:58   ` Joe Damato
  2026-09-30 20:01 ` [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Joe Damato
  2026-10-06  0:50 ` patchwork-bot+netdevbpf
  3 siblings, 1 reply; 8+ messages in thread
From: Zhang Yunfei @ 2026-09-30  9:47 UTC (permalink / raw)
  To: netdev
  Cc: jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet, kuba,
	pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	zhangyunfei1, linux-kernel, stable, leitao

On NCSI-managed systems, a failed ifup - for example when IRQ or
ring allocation fails under memory pressure - leaves the port
without either host or firmware driving it: the firmware already
handed the port over on open, so out-of-band management of the NIC
through that port stops until the next successful ifup.

ngbe_open() sets the WX_CFG_PORT_CTL_DRV_LOAD bit to tell the
management firmware the host has taken over the port, but every
error path returns without clearing it, leaving rings, IRQs and the
PHY torn down while the firmware still believes the host owns the
port.

Roll the bit back on all open error paths, matching ngbe_close()
and ngbe_dev_shutdown(), so a failed ifup leaves the same
firmware-visible state as if the interface had never been opened.

Found by manual code inspection of the open error paths. The
deterministic reproduction uses a loadable test module injecting a
wx_setup_resources() failure: in a QEMU VM the unfixed driver
leaves DRV_LOAD set after a failed open, and with the fix the bit
is cleared. No physical ngbe device is involved.

Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
---
Changes in v3:
- correct the Fixes tag to a1cf597b99a7, the commit that introduced
  the bug (the first ngbe_open() failure path after the DRV_LOAD bit
  is set; v2 pointed at e7956139a6cf, which added more failing returns
  but not the first one); no code change.

 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 8247f6c14be0..0aea5a99a1e2 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v3 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails
  2026-09-30  9:47 ` [PATCH net v3 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails Zhang Yunfei
@ 2026-09-30 19:58   ` Joe Damato
  0 siblings, 0 replies; 8+ messages in thread
From: Joe Damato @ 2026-09-30 19:58 UTC (permalink / raw)
  To: Zhang Yunfei
  Cc: netdev, jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet,
	kuba, pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	linux-kernel, stable, leitao

On Wed, Sep 30, 2026 at 05:47:48PM +0800, Zhang Yunfei wrote:
> On NCSI-managed systems, a failed ifup - for example when IRQ or
> ring allocation fails under memory pressure - leaves the port
> without either host or firmware driving it: the firmware already
> handed the port over on open, so out-of-band management of the NIC
> through that port stops until the next successful ifup.
> 
> ngbe_open() sets the WX_CFG_PORT_CTL_DRV_LOAD bit to tell the
> management firmware the host has taken over the port, but every
> error path returns without clearing it, leaving rings, IRQs and the
> PHY torn down while the firmware still believes the host owns the
> port.
> 
> Roll the bit back on all open error paths, matching ngbe_close()
> and ngbe_dev_shutdown(), so a failed ifup leaves the same
> firmware-visible state as if the interface had never been opened.
> 
> Found by manual code inspection of the open error paths. The
> deterministic reproduction uses a loadable test module injecting a
> wx_setup_resources() failure: in a QEMU VM the unfixed driver
> leaves DRV_LOAD set after a failed open, and with the fix the bit
> is cleared. No physical ngbe device is involved.
> 
> Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
> ---
> Changes in v3:
> - correct the Fixes tag to a1cf597b99a7, the commit that introduced
>   the bug (the first ngbe_open() failure path after the DRV_LOAD bit
>   is set; v2 pointed at e7956139a6cf, which added more failing returns
>   but not the first one); no code change.
> 
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

idk much about these devices but the reasoning and code looks right to me
after reading it so:

Reviewed-by: Joe Damato <joe@dama.to>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core
  2026-09-30  9:47 ` [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core Zhang Yunfei
@ 2026-09-30 20:00   ` Joe Damato
  2026-10-04 10:20   ` netdev-bot+sashiko
  1 sibling, 0 replies; 8+ messages in thread
From: Joe Damato @ 2026-09-30 20:00 UTC (permalink / raw)
  To: Zhang Yunfei
  Cc: netdev, jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet,
	kuba, pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	linux-kernel, stable, leitao, Sashiko

On Wed, Sep 30, 2026 at 05:47:47PM +0800, Zhang Yunfei wrote:
> After a failed resume from suspend, the device stays detached from
> the networking stack: every attempt to bring the interface up fails
> the netif_device_present() check in __dev_open() with -ENODEV,
> while the PM core is told that resume succeeded.
> 
> ngbe_resume() declares err as u32 and unconditionally returns 0:
> the return value of ngbe_reset_hw() is ignored entirely, and
> failures of wx_init_interrupt_scheme() and ngbe_open() are silently
> swallowed. The interrupt scheme torn down at suspend is never
> rebuilt, so the device cannot self-heal.
> 
> Fix the type to int and propagate the errors, making the whole tail
> of the resume path consistent with the pci_enable_device_mem()
> failure path at the top. If the hardware reset fails, return early:
> the remaining resume steps cannot succeed. The suspend path may
> already have torn the interface down (ngbe_close() and
> wx_clear_interrupt_scheme()), leaving freed rings and IRQs behind
> while netif_running() still reports true, so set WX_STATE_RES_FREED
> on every failing return, the same mark ngbe_down_suspend() uses for
> the PCI error recovery path: a later ngbe_close() skips the
> teardown of the already-freed state, and ngbe_up_complete() clears
> the bit again so later opens are unaffected. This is safe on the
> wx_init_interrupt_scheme() failure path too, as it cleans up after
> itself.
> 
> Found by manual code inspection of the PM error paths; the missing
> ngbe_reset_hw() propagation was reported by Sashiko in its review
> of v1. The failure paths are unreachable without fault injection:
> a loadable test module injects failures into ngbe_reset_hw(),
> wx_init_interrupt_scheme() and ngbe_open(). In a QEMU VM the
> unfixed driver hits the kernel "Trying to free already-free IRQ"
> warning on the post-suspend close; with the fix, the error is
> reported and no warning appears. No physical ngbe device is
> involved.
> 
> Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917090050.1927999-1-zhangyunfei1%40kylinos.cn
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>
> ---
> Changes in v3:
> - set WX_STATE_RES_FREED on every failing return of ngbe_resume()
>   (the pci_enable_device_mem() failure, the reset failure early return
>   and the wx_init_interrupt_scheme()/ngbe_open() failures), so that a
>   later ngbe_close() skips re-running the teardown on the already-freed
>   post-suspend state (Sashiko review of v2);
> 
> 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 | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

propagating the error through seems sensible, so:

Reviewed-by: Joe Damato <joe@dama.to>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths
  2026-09-30  9:47 [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Zhang Yunfei
  2026-09-30  9:47 ` [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core Zhang Yunfei
  2026-09-30  9:47 ` [PATCH net v3 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails Zhang Yunfei
@ 2026-09-30 20:01 ` Joe Damato
  2026-10-06  0:50 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 8+ messages in thread
From: Joe Damato @ 2026-09-30 20:01 UTC (permalink / raw)
  To: Zhang Yunfei
  Cc: netdev, jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet,
	kuba, pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	linux-kernel, stable, leitao

On Wed, Sep 30, 2026 at 05:47:46PM +0800, Zhang Yunfei wrote:
> Two error-handling fixes for the ngbe PM/open paths.

[...]

whole series feels very AI-y but from my reading of the proposed patches they
seem reasonable, hence my tags.

idk if these failure modes are real enough to be fixes or not though.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core
  2026-09-30  9:47 ` [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core Zhang Yunfei
  2026-09-30 20:00   ` Joe Damato
@ 2026-10-04 10:20   ` netdev-bot+sashiko
  1 sibling, 0 replies; 8+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 10:20 UTC (permalink / raw)
  To: zhangyunfei1
  Cc: netdev, jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet,
	kuba, pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	linux-kernel, stable, leitao, sashiko-bot

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

Critical: 0 · High: 0 (2) · Medium: 2 · 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 is tagged `Cc: stable@vger.kernel.org` and `Fixes:
  6963e463256e` ("net: ngbe: add Wake on Lan support", July 2023).
- [Medium] The patch marks the post-suspend freed state with
  `WX_STATE_RES_FREED` only when `ngbe_resume()` fails.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the shared
  Wangxun VF resume path has the same defect this patch fixes for ngbe,…
- [High] This problem wasn't introduced by this patch, but the patch now
  relies on it.

--

net: ngbe: propagate resume errors to the PM core

This patch changes err in ngbe_resume() from u32 to int. It also passes
failures from ngbe_reset_hw(), wx_init_interrupt_scheme() and ngbe_open()
up to the PM core, instead of always returning 0. On every failing return
it sets WX_STATE_RES_FREED, so a later ngbe_close() skips the teardown of
state that was already freed at suspend.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917090050.1927999-1-zhangyunfei1%40kylinos.cn

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 855dc963c610c..8247f6c14be0f 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -954,13 +954,14 @@ static int ngbe_resume(struct pci_dev *pdev)

[ ... ]

>  	err = pci_enable_device_mem(pdev);
>  	if (err) {
> +		set_bit(WX_STATE_RES_FREED, wx->state);

[Severity: Medium]
This patch has Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
and Cc: stable, so it will be picked up for every stable tree from v6.6.y
on. Will it build in those trees?

WX_STATE_RES_FREED comes from commit e73e4d187a1f52 ("net: wangxun: add
pcie error handler"). So does the check in ngbe_close() that this fix
depends on:

    if (test_bit(WX_STATE_RES_FREED, wx->state))
        goto out;

e73e4d187a1f52 is a feature commit with no Fixes: or stable tag. The
commit message doesn't list it as a dependency.

In stable trees that have 6963e463256e but not e73e4d187a1f52,
WX_STATE_RES_FREED is undeclared. A backporter might drop the set_bit()
calls to get it to build. In that case ngbe_close() still has no
RES_FREED guard in those trees. The post-suspend "Trying to free
already-free IRQ" double teardown described in the commit message would
then still happen after a failed resume.

Should e73e4d187a1f52 be listed as a stable prerequisite?

>  		wx_err(wx, "Cannot enable PCI device from suspend\n");
>  		return err;
>  	}
> @@ -968,16 +969,23 @@ 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) {
> +		set_bit(WX_STATE_RES_FREED, wx->state);
> +		wx_err(wx, "Hardware reset failed: %d\n", err);
> +		return err;
> +	}

[Severity: Medium]
The state being marked here is freed at suspend time, not in
ngbe_resume(). ngbe_suspend()->ngbe_dev_shutdown() does:

    rtnl_lock();
    netif_device_detach(netdev);

    if (netif_running(netdev))
        ngbe_close(netdev);
    wx_clear_interrupt_scheme(wx);
    rtnl_unlock();

It never sets WX_STATE_RES_FREED, and __LINK_STATE_START stays set.

Can something else close the netdev between suspend and the point where
ngbe_resume() either succeeds or reaches one of these set_bit() calls?
dev_close() doesn't check netif_device_present(). Take any RTNL holder,
such as netns teardown on the non-freezable netns_wq:

default_device_exit_net()
  dev_change_net_namespace()
    dev_close()
      ngbe_close()

It would see RES_FREED clear and run ngbe_down() and wx_free_irq()
again. MSI-X is already gone at that point, so wx_free_irq() takes this
path:

    if (!(pdev->msix_enabled)) {
        if (!wx->misc_irq_domain)
            free_irq(pdev->irq, wx);
        return;
    }

That gives the same "Trying to free already-free IRQ" warning the commit
message describes.

The locking is also inconsistent. This set_bit() and the one after the
pci_enable_device_mem() failure run without rtnl_lock. ngbe_close() tests
the bit under RTNL, and the third set_bit() below is also under RTNL.

Would it work to set WX_STATE_RES_FREED in ngbe_dev_shutdown() under
RTNL, right after ngbe_close() and wx_clear_interrupt_scheme()? That is
what ngbe_down_suspend() does. A successful resume clears the bit again
through ngbe_open()->ngbe_up_complete(), and the three set_bit() calls in
ngbe_resume() would no longer be needed.

>  	rtnl_lock();
>  	err = wx_init_interrupt_scheme(wx);
>  	if (!err && netif_running(netdev))
>  		err = ngbe_open(netdev);
>  	if (!err)
>  		netif_device_attach(netdev);
> +	else
> +		set_bit(WX_STATE_RES_FREED, wx->state);

[Severity: High]
This is a pre-existing issue, but this patch now depends on it. When
ngbe_open() fails here, does anything stop the RX DMA that ngbe_open()
has already turned on?

Before any step in ngbe_open() can fail, wx_configure() has already armed
RX:

wx_configure()
  wx_configure_rx()
    wx_configure_rx_ring()  <- programs WX_PX_RR_BAL/BAH, sets RR_EN,
                               calls wx_enable_rx_queue() and
                               wx_alloc_rx_buffers()
    wx_enable_rx()          <- sets WX_MAC_RX_CFG_RE and
                               WX_RDB_PB_CTL_RXEN

If ngbe_request_irq(), phylink_connect_phy() or
netif_set_real_num_{tx,rx}_queues() fails afterwards, the unwind is:

err_dis_phy:
	phylink_disconnect_phy(wx->phylink);
err_free_irq:
	wx_free_irq(wx);
err_free_resources:
	wx_free_isb_resources(wx);
	wx_free_resources(wx);
err_control_hw:
	wx_control_hw(wx, false);
	return err;

wx_free_rx_resources() calls dma_free_coherent() on the descriptor ring
and destroys the page pool. Nothing writes the registers that disable the
RX queues or the MAC receiver.

On this resume path bus mastering is still on, because ngbe_reset_hw()
called pci_set_master(). With the new set_bit(), a later ngbe_close()
jumps straight to wx_control_hw(false) and skips
ngbe_down()->ngbe_disable_device(). That is the only code that stops the
queues.

Even without the RES_FREED mark, ngbe_disable_device() would return early.
wx_init_interrupt_scheme() set WX_STATE_DOWN, and nothing cleared it.

If the link is up, for example kept up for Wake-on-LAN, can incoming
frames be DMA-written into the freed descriptor memory and freed pages?
A plain ndo_open failure reaches the same unwind.

The patch in this series "net: ngbe: clear DRV_LOAD bit when ngbe_open()
fails" adds wx_control_hw(wx, false) to this unwind. That covers the
stale WX_CFG_PORT_CTL_DRV_LOAD bit. It still doesn't disable the RX rings
or the MAC receiver, so the DMA part is still open at the end of the
series.

>  	rtnl_unlock();
>  
> -	return 0;
> +	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the shared Wangxun VF
resume path has the same problem, and there it can crash.
wxvf_resume() in drivers/net/ethernet/wangxun/libwx/wx_vf_common.c is
used by both ngbevf and txgbevf:

	pci_set_master(pdev);
	wx_init_interrupt_scheme(wx);
	netif_device_attach(wx->netdev);

	return 0;

If wx_init_interrupt_scheme() fails, the PM core is told resume succeeded
and the netdev is attached with no q_vectors. wxvf_suspend() has already
freed them: wx_free_q_vector() sets the wx->tx_ring[] and wx->rx_ring[]
entries to NULL, and wx_reset_interrupt_capability() sets wx->msix_entry
to NULL.

A later "ip link set up" then passes the netif_device_present() check and
runs wxvf_open(). Does that dereference NULL?

If MSI-X allocation failed, the queue counts are left non-zero, so
wx_setup_resources() walks NULL tx_ring[] entries.

If q_vector allocation failed, wx_request_msix_irqs_vf() runs:

	err = request_irq(wx->msix_entry->vector, wx_msix_misc_vf,
			  0, netdev->name, wx);

with msix_entry set to NULL.

The commit message limits the fix to ngbe. Should wxvf_resume() get the
same error handling?

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths
  2026-09-30  9:47 [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Zhang Yunfei
                   ` (2 preceding siblings ...)
  2026-09-30 20:01 ` [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Joe Damato
@ 2026-10-06  0:50 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-10-06  0:50 UTC (permalink / raw)
  To: Zhang Yunfei
  Cc: netdev, jiawenwu, mengyuanlou, andrew+netdev, davem, edumazet,
	kuba, pabeni, aleksandr.loktionov, u.kleine-koenig, weirongguang,
	linux-kernel, stable, leitao

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 30 Sep 2026 17:47:46 +0800 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] net: ngbe: propagate resume errors to the PM core
    https://git.kernel.org/netdev/net-next/c/6986ff35f5bb
  - [net,v3,2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails
    https://git.kernel.org/netdev/net-next/c/7b36b48049f4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-10-06  0:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-30  9:47 [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Zhang Yunfei
2026-09-30  9:47 ` [PATCH net v3 1/2] net: ngbe: propagate resume errors to the PM core Zhang Yunfei
2026-09-30 20:00   ` Joe Damato
2026-10-04 10:20   ` netdev-bot+sashiko
2026-09-30  9:47 ` [PATCH net v3 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails Zhang Yunfei
2026-09-30 19:58   ` Joe Damato
2026-09-30 20:01 ` [PATCH net v3 0/2] net: ngbe: fix error handling in resume and open paths Joe Damato
2026-10-06  0:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®