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