Re: [PATCH net-next v1 4/5] net: wangxun: extract the close_suspend sequence

[Paolo Abeni <pabeni@redhat.com>]

On 4/28/26 4:11 AM, Jiawen Wu wrote:
> @@ -510,6 +511,16 @@ void ngbe_up(struct wx *wx)
>  	ngbe_up_complete(wx);
>  }
>  
> +void ngbe_close_suspend(struct wx *wx)
> +{
> +	wx_ptp_suspend(wx);

Sashiko says:

Does this introduce a redundant call to wx_ptp_suspend()?
Both ngbe_close() and txgbe_close() call wx_ptp_stop() immediately before
conditionally calling ngbe_close_suspend() or txgbe_close_suspend().
Looking at wx_ptp_stop(), it already executes wx_ptp_suspend() as its first
operation.
While test_and_clear_bit(WX_STATE_PTP_RUNNING, wx->state) makes the second
call safe, it seems to be redundant execution.

> +	ngbe_down(wx);
> +	wx_free_irq(wx);
> +	wx_free_isb_resources(wx);
> +	wx_free_resources(wx);
> +	phylink_disconnect_phy(wx->phylink);
> +}
> +
>  /**
>   * ngbe_close - Disables a network interface
>   * @netdev: network interface device structure
> @@ -526,11 +537,8 @@ static int ngbe_close(struct net_device *netdev)
>  	struct wx *wx = netdev_priv(netdev);
>  
>  	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);
> +	if (netif_device_present(netdev))
> +		ngbe_close_suspend(wx);

Sashiko says:

On the ngbe side, can this skip the teardown sequence entirely on the
suspend/shutdown paths?
ngbe_dev_shutdown() (shared by .suspend and .shutdown) does:
    static void ngbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake)
    {
            ...
            netif_device_detach(netdev);
            if (netif_running(netdev))
                    ngbe_close(netdev);
            wx_clear_interrupt_scheme(wx);
            ...
    }
netif_device_detach() clears __LINK_STATE_PRESENT, so by the time
ngbe_close() runs, netif_device_present(netdev) returns false and the new
guard
    if (netif_device_present(netdev))
            ngbe_close_suspend(wx);
skips ngbe_close_suspend() on the suspend and shutdown paths.  Before this
patch ngbe_close() unconditionally ran ngbe_down(), wx_free_irq(),
wx_free_isb_resources(), wx_free_resources() and
phylink_disconnect_phy() on those paths.
Does that mean after this patch, suspend and shutdown:
  - leave the requested MSI-X / legacy IRQ handlers registered while
    wx_clear_interrupt_scheme() subsequently frees the vector pool,
  - leak the coherent ISB DMA buffer (isb_mem / isb_dma) freed in
    wx_free_isb_resources(),
  - leak the TX/RX ring DMA allocations freed in wx_free_resources(),
  - leave phylink connected (no phylink_disconnect_phy()), so resume
    re-enters phylink_connect_phy on an already connected state, and
  - skip ngbe_down() / ngbe_reset(), so the MAC is not quiesced before
    pci_disable_device()?
The txgbe shutdown path avoids this because txgbe_dev_shutdown() calls
txgbe_close_suspend() directly rather than going through txgbe_close():
    static void txgbe_dev_shutdown(struct pci_dev *pdev)
    {
            ...
            netif_device_detach(netdev);
            rtnl_lock();
            if (netif_running(netdev))
                    txgbe_close_suspend(wx);
            rtnl_unlock();
            ...
    }
Would an equivalent change in ngbe_dev_shutdown() (call
ngbe_close_suspend() directly instead of ngbe_close()) be the intended
pairing for the new guard in ngbe_close()?

/P