Re: [net PATCH 11/13] 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 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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 PM suspend path does not protect against a service-task
  pass that runs late.

--

fbnic: Suspend phylink after taking the datapath down in ndo_stop

This patch reorders fbnic_stop() so that fbnic_down() runs before
fbnic_mac_free_irq() and phylink_suspend(). Tx then drains while the link
is still up, and the shutdown order becomes the reverse of the open path.
The commit message says the PM suspend path picks up the same order
because it goes through ndo_stop.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index a40566029a863..36ef17dec0f4a 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);
>  	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
>  
> -	fbnic_down(fbn);
> -
>  	fbnic_time_stop(fbn);
>  	fbnic_fw_xmit_ownership_msg(fbn->fbd, false);

[Severity: Medium]

The commit message says:

  The PM suspend path goes through ndo_stop and picks up the same order.

Does the PM path also get the service task protection that this ordering
relies on after ndo_stop?

fbnic_service_task() uses netif_running() to decide whether it may touch
host state:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task() {
    ...
	/* fbnic_service_task_stop() does not wait for a pass that is already
	 * blocked on RTNL, so one can run after ndo_stop. Leave host state
	 * alone in that case; __fbnic_open() reclaims and rebuilds it.
	 */
	if (netif_running(netdev)) {
		fbnic_health_check(fbd);

		fbnic_bmc_rpc_check(fbd);
    ...
}

That works for __dev_close_many(), which clears __LINK_STATE_START before
it calls ndo_stop. fbnic_pm_suspend() calls ndo_stop directly instead:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_pm_suspend() {
    ...
	netif_device_detach(netdev);

	if (netif_running(netdev))
		netdev->netdev_ops->ndo_stop(netdev);
    ...
}

netif_device_detach() only clears __LINK_STATE_PRESENT, so netif_running()
is still true after this. Also, fbnic_stop()->fbnic_down()->
fbnic_down_noidle()->fbnic_service_task_stop() uses cancel_delayed_work().
That call does not wait for a pass that is already blocked in rtnl_lock().

Suppose a pass is waiting on RTNL during the PM ndo_stop. When
fbnic_pm_suspend() drops RTNL, that pass sees netif_running() == true.

At that point fbnic_health_check() runs right after
fbnic_fw_xmit_ownership_msg(fbd, false) has cleared fw_heartbeat_enabled.
If the Tx mailbox has drained, can it report "Firmware crash detected!"?
After that, fbnic_fw_config_after_crash() would reclaim ownership and write
the host MACDA/action rules through __fbnic_set_rx_mode() while the queues
are disabled.

fbnic_fw_present() only checks uc_addr4, and uc_addr4 stays set until the
end of fbnic_pm_suspend(). The work can also run at the same time as
fbnic_fw_free_mbx(), which is serialized by devl_lock and not RTNL.

fbnic_bmc_rpc_check() can reprogram the BMC/host rules in the same way.

The pass then calls schedule_delayed_work(&fbd->service_task, HZ). Would
the service task then keep re-arming every HZ across suspend, after the
mailbox and IRQs are freed? If so, it would repeat the false crash report
and the ownership reclaim until resume calls ndo_open.

The window is not trivially small, because ndo_stop includes
fbnic_wait_all_queues_idle(). The effect looks transient: on resume,
__fbnic_open() reclaims ownership and rebuilds the rules.

The netif_running() gate was added by the earlier series commit "fbnic:
Don't touch host filters from a service task pass after ndo_stop". It only
covers the __dev_close_many() caller, and the code is unchanged at the end
of the series.

Should fbnic_pm_suspend() keep a late service task pass out in a way that
does not depend on __LINK_STATE_START?

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