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

[PATCH net 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 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 the error, making the 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().

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


base-commit: 587858367581b9c55c3690f4e63382ad622719d4
-- 
2.25.1

[PATCH net 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. 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 return err, making the tail of the resume
path consistent with the pci_enable_device_mem() failure path at
the top, which already propagates its error. A failed resume then
becomes visible to the PM core and the device can be re-probed.

Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
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, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index 855dc963c610..6d8289e2532d 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;
@@ -977,7 +977,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

[PATCH net 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 6d8289e2532d..88f00b72a512 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 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

[mengyuanlou]

> 2026年9月17日 17:00，Zhang Yunfei <zhangyunfei1@kylinos.cn> 写道：
> 
> 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>

Reviewed-by: Mengyuan Lou <mengyuanlou@net-swift.com>

> ---
> 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 6d8289e2532d..88f00b72a512 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 1/2] net: ngbe: propagate resume errors to the PM core

[mengyuanlou]

> 2026年9月17日 17:00，Zhang Yunfei <zhangyunfei1@kylinos.cn> 写道：
> 
> ngbe_resume() declares err as u32 and unconditionally returns 0, so
> failures of wx_init_interrupt_scheme() or ngbe_open() are silently
> swallowed. 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 return err, making the tail of the resume
> path consistent with the pci_enable_device_mem() failure path at
> the top, which already propagates its error. A failed resume then
> becomes visible to the PM core and the device can be re-probed.
> 
> Fixes: 6963e463256e ("net: ngbe: add Wake on Lan support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhang Yunfei <zhangyunfei1@kylinos.cn>

Reviewed-by: Mengyuan Lou <mengyuanlou@net-swift.com>

> ---
> drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 855dc963c610..6d8289e2532d 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;
> @@ -977,7 +977,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 1/2] net: ngbe: propagate resume errors to the PM core

[netdev-bot+sashiko]

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

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

- [High] ngbe_resume() still discards the return value of ngbe_reset_hw()
  (drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:971:…
- [Medium] Commit message overstates what the patch achieves: "A failed
  resume then becomes visible to the PM core and the device can be re-…

Pre-existing issues:
- [High] After a failed resume (either wx_init_interrupt_scheme() or
  ngbe_open() failing in ngbe_resume(), ngbe_main.c:973-976) the netdev…
- [Medium] The same 'swallowed resume error' defect exists in the shared
  libwx VF resume helper wxvf_resume()…
- [Medium] ngbe_open()'s error path — the path whose failure this patch
  now surfaces from ngbe_resume() — frees the Rx/Tx descriptor rings,…

--

net: ngbe: propagate resume errors to the PM core

This changes err in ngbe_resume() from u32 to int and returns err instead
of a hardcoded 0, so that failures from wx_init_interrupt_scheme() or
ngbe_open() reach the PM core. The pci_enable_device_mem() failure path at
the top of the function already propagated its error.

A few questions about the changelog and about what happens on the newly
reachable failure return.

The commit message says:

> Fix the type to int and return err, making the tail of the resume
> path consistent with the pci_enable_device_mem() failure path at
> the top, which already propagates its error. A failed resume then
> becomes visible to the PM core and the device can be re-probed.

[Severity: Medium]
Is the "the device can be re-probed" part accurate?  For the legacy PCI
.resume hook, the return value ends up in device_resume():

drivers/base/power/main.c:device_resume() {
	...
	if (error) {
		WRITE_ONCE(async_error, error);
		dpm_save_failed_dev(dev_name(dev));
		pm_dev_err(dev, state, async ? " async" : "", error);
	}
	...
}

That records and logs the failure, but there is no unbind, no re-probe and
no retry.  ngbe_resume() also does no rollback on the error return: no
pci_disable_device(), no wx_clear_interrupt_scheme(), and the netdev stays
detached while still IFF_UP.

So the user-visible state described earlier in the changelog ("the netdev
never appears in the networking stack again ... the device cannot
self-heal") looks unchanged by this patch, with one extra dmesg line as the
only difference.  Could the last sentence be reworded to claim only what
the code does?

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 855dc963c610c..6d8289e2532d0 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;

[Severity: High]
Does the same "silently swallowed" case still exist a few lines further
down in this function?  ngbe_resume() calls ngbe_reset_hw() without
capturing the result, and the next statement overwrites err:

	ngbe_reset_hw(wx);
	rtnl_lock();
	err = wx_init_interrupt_scheme(wx);

ngbe_reset_hw() in ngbe_hw.c has two real error exits:

	ret = wx_stop_adapter(wx);
	if (ret != 0)
		return ret;
	...
		if (ret) {
			wx_err(wx, "Lan reset exceed s maximum times.\n");
			return ret;
		}

On those exits ngbe_reset_misc(), wx_clear_hw_cntrs(), wx_get_mac_addr(),
wx_init_rx_addrs() and pci_set_master() are all skipped.  Execution in
ngbe_resume() continues anyway, wx_init_interrupt_scheme() and ngbe_open()
usually succeed (or ngbe_open() is skipped when the interface is
administratively down), so err is 0, netif_device_attach() runs, and the
new return err reports success even though the MAC never acknowledged the
LAN reset and the MAC/Rx address state was not restored.

ngbe_probe() treats the same call as fatal:

	err = ngbe_reset_hw(wx);
	if (err) {
		dev_err(&pdev->dev, "HW Init failed: %d\n", err);
		goto err_free_mac_table;
	}

Should the resume path check it too, so the bug class this patch targets is
actually closed for the whole function?

> @@ -977,7 +977,7 @@ static int ngbe_resume(struct pci_dev *pdev)
>  		netif_device_attach(netdev);
>  	rtnl_unlock();
>  
> -	return 0;
> +	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch (the pre-patch code reached the
same state, just with err discarded), but what state is the device left in
after this new failure return, and can a later ndo_stop oops?

After wx_init_interrupt_scheme() or ngbe_open() fails here, ngbe_open() was
called directly rather than through dev_open(), so __LINK_STATE_START stays
set and netif_running() remains true, while the datapath resources are
already released.  WX_STATE_RES_FREED, the only guard ngbe_close() has for
"resources already released", is clear: it is set only by
ngbe_down_suspend() and cleared by ngbe_up_complete().

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

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

So the next ip link set dev X down, further suspend via
ngbe_dev_shutdown(), or ngbe_remove() -> unregister_netdev() runs the full
teardown a second time over released objects:

ngbe_close() -> ngbe_down() -> wx_clean_all_rx_rings() -> wx_clean_rx_ring()
	/* Zero out the descriptor ring */
	memset(rx_ring->desc, 0, rx_ring->size);

rx_ring->desc was set to NULL by wx_free_rx_resources() while
rx_ring->size stays non-zero, so this is a write through NULL.

When wx_init_interrupt_scheme() was the failing call, the ring loops also
dereference NULL, since wx_set_num_queues() restored num_tx_queues while
the suspend-time wx_clear_interrupt_scheme() left every ring pointer NULL:

wx_clean_all_tx_rings() {
	for (i = 0; i < wx->num_tx_queues; i++)
		wx_clean_tx_ring(wx->tx_ring[i]);
}

And wx_free_irq() would call free_irq() for an IRQ that is not registered
("Trying to free already-free IRQ" warning).  Would it make sense to unwind
the resume failure here, or to set WX_STATE_RES_FREED on this path?

[Severity: Medium]
This is a pre-existing issue, but the failure that this patch now surfaces
comes from ngbe_open(), whose error path frees the Rx/Tx descriptor rings,
the page-pool pages and the ISB coherent buffer without stopping the
hardware that wx_configure() just armed:

ngbe_open() {
	...
	wx_configure(wx);

	err = ngbe_request_irq(wx);
	if (err)
		goto err_free_resources;
	...
err_free_resources:
	wx_free_isb_resources(wx);
	wx_free_resources(wx);
	...
}

wx_configure() -> wx_configure_rx_ring() programs the ring base addresses,
enables the queue and posts buffers:

	wr32(wx, WX_PX_RR_BAL(reg_idx), rdba & DMA_BIT_MASK(32));
	wr32(wx, WX_PX_RR_BAH(reg_idx), upper_32_bits(rdba));
	...
	wr32m(wx, WX_PX_RR_CFG(reg_idx), WX_PX_RR_CFG_RR_EN, WX_PX_RR_CFG_RR_EN);
	wx_enable_rx_queue(wx, ring);
	wx_alloc_rx_buffers(ring, wx_desc_unused(ring));

and wx_configure_isb() writes WX_PX_ISB_ADDR_L/H with wx->isb_dma.  The
error labels only free memory: there is no ngbe_down(), wx_stop_adapter(),
wx_disable_rx() or wx_disable_rx_queue(), and no re-reset, so RXEN, the
per-queue enables and the stale descriptor/ISB base addresses stay
programmed with bus mastering on while the memory is released.

Can the Rx engine write into those released page-pool pages afterwards?  I
could not confirm that frames arrive in this window, since phylink_start()
is only reached from ngbe_up_complete(), so the link may still be down.
Would a stop/reset in the ngbe_open() error path be worth adding while this
path is being made reachable through a reported resume failure?

>  
>  static struct pci_driver ngbe_driver = {

[Severity: Medium]
This isn't a bug introduced by this patch, but was the sibling VF resume
helper considered?  wxvf_resume() in libwx/wx_vf_common.c has the same
pattern this patch removes from ngbe_resume():

int wxvf_resume(struct device *dev_d)
{
	...
	pci_set_master(pdev);
	wx_init_interrupt_scheme(wx);
	netif_device_attach(wx->netdev);

	return 0;
}

The wx_init_interrupt_scheme() return value is dropped, the netdev is
attached regardless, and 0 is returned.  That helper is the system-sleep
resume callback for both ngbevf and txgbevf:

	static DEFINE_SIMPLE_DEV_PM_OPS(ngbevf_pm_ops, wxvf_suspend, wxvf_resume);
	static DEFINE_SIMPLE_DEV_PM_OPS(txgbevf_pm_ops, wxvf_suspend, wxvf_resume);

The VF case looks worse than the PF one, because wxvf_suspend() calls
wx_clear_interrupt_scheme() (rings NULLed, queue counts zeroed) and
wxvf_resume() attaches the netdev even when the interrupt scheme was not
re-established.  Should the two implementations of the same resume contract
be fixed together, or is there a reason the VF path is left as is?

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0 (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] For all ngbe_open() failures that occur after wx_configure()
  (i.e. ngbe_request_irq(), phylink_connect_phy(),…

Pre-existing issues:
- [High] ngbe_open()'s error unwind
  (drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:522-531) frees the…
- [High] ngbe_setup_tc()
  (drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:630-647) discards the…
- [Low] The new wx_control_hw(wx, false) at
  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:530 performs an unlocked…

--

net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

The patch makes every error path of ngbe_open() fall through a new
err_control_hw label that calls wx_control_hw(wx, false), so the
WX_CFG_PORT_CTL_DRV_LOAD handshake bit is rolled back when the open
fails instead of being left asserted until the next successful ifup.

A few questions about the resulting error unwind below.

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 6d8289e2532d0..88f00b72a5123 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;
>  }

[Severity: Medium]

For the failures that happen after wx_configure() has run
(ngbe_request_irq(), phylink_connect_phy(),
netif_set_real_num_tx_queues(), netif_set_real_num_rx_queues()), does
this unwind free DMA memory that the hardware is still programmed to
use?

wx_configure() arms the datapath before any of those calls can fail:

  wx_configure()
    wx_configure_tx()   /* enables Tx queues, TDM, MAC transmitter */
    wx_configure_rx()
      wx_configure_rx_ring()
        wr32(wx, WX_PX_RR_BAL(reg_idx), rdba & DMA_BIT_MASK(32));
        wr32(wx, WX_PX_RR_BAH(reg_idx), upper_32_bits(rdba));
        wr32m(wx, WX_PX_RR_CFG(reg_idx), WX_PX_RR_CFG_RR_EN, WX_PX_RR_CFG_RR_EN);
        wx_enable_rx_queue(wx, ring);
        wx_alloc_rx_buffers(ring, wx_desc_unused(ring));
      wx_enable_rx(wx);
    wx_configure_isb()
      wr32(wx, WX_PX_ISB_ADDR_L, wx->isb_dma & DMA_BIT_MASK(32));

The error labels then release exactly that memory:

  wx_free_isb_resources()  /* dma_free_coherent() of the ISB buffer */
  wx_free_resources()
    wx_free_all_rx_resources()
      wx_free_rx_resources()
        wx_clean_rx_ring(rx_ring);
        dma_free_coherent(rx_ring->dev, rx_ring->size, rx_ring->desc,
                          rx_ring->dma);
        page_pool_destroy(rx_ring->page_pool);

None of the quiesce steps that ngbe_disable_device() performs run here:

  for (i = 0; i < wx->num_rx_queues; i++)
          wx_disable_rx_queue(wx, wx->rx_ring[i]);
  wx_disable_rx(wx);
  ...
  wx_irq_disable(wx);
  for (i = 0; i < wx->num_tx_queues; i++)
          wr32(wx, WX_PX_TR_CFG(reg_idx), WX_PX_TR_CFG_SWFLSH);

Can a descriptor or buffer writeback in that window land in freed
coherent memory or an unmapped IOVA?

Related to that, the commit message says the rollback is "matching
ngbe_close() and ngbe_dev_shutdown(), which already clear it".  Is the
ordering the same though?  ngbe_close() quiesces first and only then
releases the handshake:

  wx_ptp_stop(wx);
  ngbe_down(wx);
  wx_free_irq(wx);
  wx_free_isb_resources(wx);
  wx_free_resources(wx);
  phylink_disconnect_phy(wx->phylink);
out:
  wx_control_hw(wx, false);

Here the new wx_control_hw(wx, false) hands the port back to the
NCSI/OOB firmware while the Rx unit and RXEN are still enabled and the
rings have already been freed.  Would routing the error paths through
ngbe_down() (or at least the wx_disable_rx_queue()/wx_disable_rx()/
wx_irq_disable() sequence) before freeing resources and clearing
DRV_LOAD be closer to what ngbe_close() does?

[Severity: High]

This isn't a bug introduced by this patch, but while the unwind is
being reworked: should this path also set WX_STATE_RES_FREED?

ngbe_close() uses that bit as its only record of "resources already
released":

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

  wx_ptp_stop(wx);
  ngbe_down(wx);
  wx_free_irq(wx);
  wx_free_isb_resources(wx);
  wx_free_resources(wx);

ngbe_open() is also called on netdevs whose __LINK_STATE_START is
already set (ngbe_setup_tc(), ngbe_resume(), and wx_io_resume() in
libwx/wx_err.c), so a failed open leaves netif_running() true with the
IRQs, the ISB buffer and the rings freed while the bit is clear.

The next ndo_stop then repeats the teardown.  wx_free_irq() calls
free_irq() on vectors that were never requested, and
ngbe_down() -> wx_clean_all_rx_rings() -> wx_clean_rx_ring() reaches:

  /* Zero out the descriptor ring */
  memset(rx_ring->desc, 0, rx_ring->size);

with rx_ring->desc already NULL and rx_ring->size still non-zero, since
wx_free_rx_resources() clears desc but not size.  Can that oops?

[Severity: High]

This is a pre-existing issue too, but this patch makes the error return
of ngbe_open() more meaningful, so it seems worth asking: ngbe_setup_tc()
throws it away.

  wx_init_interrupt_scheme(wx);

  if (netif_running(dev))
          ngbe_open(dev);

  return 0;

wx_init_interrupt_scheme() can fail with -ENOMEM from
wx_set_interrupt_capability()/wx_alloc_q_vectors(), and every failure
exit of ngbe_open() now funnels through err_control_hw and returns a
negative errno.  Both are dropped and 0 is returned unconditionally, so
wx_set_channels() in libwx/wx_ethtool.c:

  return wx->setup_tc(dev, netdev_get_num_tc(dev));

reports success to user space while the interface stays administratively
running with rings and the ISB buffer freed, no IRQs requested, the PHY
disconnected, and now DRV_LOAD cleared with nothing re-asserting it
until a later successful ndo_open.  Should ngbe_setup_tc() propagate
these errors?

[Severity: Low]

This is a pre-existing pattern rather than something the patch
introduces, noted only for completeness: wx_control_hw() does an
unlocked read-modify-write of WX_CFG_PORT_CTL through wr32m():

  val = rd32(wx, reg);
  val = ((val & ~mask) | (field & mask));
  wr32(wx, reg, val);

__wx_enable_sriov() in libwx/wx_sriov.c updates the same register:

  wr32m(wx, WX_CFG_PORT_CTL,
        WX_CFG_PORT_CTL_NUM_VT_MASK,
        value);

and it runs from wx_pci_sriov_enable() under the PCI device_lock but
before wx_sriov_reinit() takes RTNL, so RTNL does not serialize the two.
The identical unlocked update already exists on every ifup via
wx_control_hw(wx, true), in ngbe_close() and in ngbe_dev_shutdown(), so
this line adds no new race class; serializing the WX_CFG_PORT_CTL
updates would be a separate libwx cleanup.

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