Re: [net PATCH 4/5] fbnic: Suspend phylink after taking the datapath down in ndo_stop

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The Fixes tag points at the wrong commit.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  for fbnic_mac_free_irq()…

--

fbnic: Suspend phylink after taking the datapath down in ndo_stop

This commit reorders fbnic_stop() so that fbnic_down() runs before
fbnic_mac_free_irq() and phylink_suspend(). Tx can then drain while the
link is still up, instead of draining into a MAC that is disabled and held
in reset. The PM suspend path goes through ndo_stop, so it gets the same
ordering.

> Fixes: 6ede14a2c636 ("fbnic: Move phylink resume out of service_task and into open/close")

[Severity: Low]
Should this tag point at an earlier commit?

Before 6ede14a2c636, phylink_suspend() was already the first thing that
ran on the down path. fbnic_down_noidle() first called
fbnic_service_task_stop(), and that function started with:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task_stop() {
	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbd));
	cancel_delayed_work(&fbd->service_task);
    ...
}

fbnic_napi_disable(), netif_tx_disable(), the RPC/rx-mode clearing,
fbnic_disable() and the fbnic_wait_all_queues_idle() call in fbnic_down()
all came after it.

At 69684376eed5, fbnic_mac_link_down_asic() already disabled the MAC and
held it in reset:

	mac_ctrl |= FBNIC_SIG_MAC_IN0_RESET_FF_TX_CLK |
		    FBNIC_SIG_MAC_IN0_RESET_TX_CLK |
		    FBNIC_SIG_MAC_IN0_RESET_FF_RX_CLK |
		    FBNIC_SIG_MAC_IN0_RESET_RX_CLK;

6ede14a2c636 moved phylink_suspend() from the top of fbnic_down() to just
before fbnic_down() in fbnic_stop(). It was still ahead of the queue
teardown in both places.

It looks like the ordering problem described here dates from
69684376eed5 ("eth: fbnic: Add link detection"). Would this be more
accurate?

Fixes: 69684376eed5 ("eth: fbnic: Add link detection")

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index 8a6703afca384..52244219d1668 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> @@ -86,11 +86,11 @@ static int fbnic_stop(struct net_device *netdev)
>  {
>  	struct fbnic_net *fbn = netdev_priv(netdev);
>  
> +	fbnic_down(fbn);
> +
>  	fbnic_mac_free_irq(fbn->fbd);

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc for
fbnic_mac_free_irq() in fbnic_irq.c says:

 * This function undoes the work done in fbnic_mac_request_irq and prepares
 * the device to no longer receive traffic on the host interface.

The function only disables the PCS MSI-X control entry, calls
synchronize_irq(), masks the vector, frees the IRQ and clears
fbd->mac_msix_vector.

Host Rx is actually stopped in fbnic_down_noidle(), through
fbnic_clear_rx_mode(), fbnic_clear_rules(), fbnic_rss_disable_hw() and
fbnic_disable(). With this reordering, fbnic_down() has already stopped
host traffic before fbnic_mac_free_irq() is called.

Could the comment be updated to say that this only tears down the MAC/PCS
link interrupt?

>  	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
>  
> -	fbnic_down(fbn);
> -
>  	fbnic_time_stop(fbn);
>  	fbnic_fw_xmit_ownership_msg(fbn->fbd, false);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179381496288.9644.9775319757787890210.stgit%40ahduyck-xeon-server.home.arpa