Re: [net PATCH 05/13] fbnic: Don't touch host filters from a service task pass after 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: 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 patch is incomplete.

--

fbnic: Don't touch host filters from a service task pass after ndo_stop

This commit moves fbnic_health_check() and fbnic_bmc_rpc_check() in
fbnic_service_task() under the netif_running() check that re-arms the
task. A pass that was blocked on RTNL during ndo_stop should then skip
the firmware recovery and BMC TCAM rebuild. The fix relies on
__dev_close_many() clearing __LINK_STATE_START before it calls ndo_stop.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index ad234bb0f75d0..e7744915d001c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -229,18 +229,23 @@ static void fbnic_service_task(struct work_struct *work)

[ ... ]

> -	if (netif_running(netdev))
> +	/* 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);
> +
>  		schedule_delayed_work(&fbd->service_task, HZ);
> +	}

[Severity: High]
Does this gate cover the ndo_stop call in fbnic_pm_suspend()?

That path calls ndo_stop directly and does not go through
__dev_close_many(). Before calling it, it only does
netif_device_detach():

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

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

netif_device_detach() clears __LINK_STATE_PRESENT and leaves
__LINK_STATE_START set. So netif_running() is still true after
fbnic_pm_suspend() drops RTNL.

fbnic_pm_suspend() is reached from system suspend, fbnic_shutdown() and
fbnic_err_error_detected(). On those paths, can the same sequence the
commit message describes still happen?

CPU1 (fbnic_pm_suspend)              CPU2 (fbnic_service_task)
rtnl_lock()
                                     rtnl_lock()  <- blocks
ndo_stop()
  fbnic_stop()
    fbnic_down()
      fbnic_service_task_stop()
        cancel_delayed_work()
    fbnic_fw_xmit_ownership_msg(fbd, false)
      fw_heartbeat_enabled = false
rtnl_unlock()
                                     netif_running() is still true
                                     fbnic_health_check()
                                       fbnic_fw_config_after_crash()
                                     fbnic_bmc_rpc_check()
                                     schedule_delayed_work()

In that pass, fbnic_health_check() sees fw_heartbeat_enabled == false
with an empty Tx mailbox. It reports a firmware crash through devlink
that did not happen. fbnic_fw_config_after_crash() then reclaims
ownership and writes the host rules back with __fbnic_set_rx_mode()
while the host queues are disabled.

If FBNIC_FW_CAP_F_BMC_TCAM_REINIT is set, fbnic_bmc_rpc_check() also
runs fbnic_bmc_rpc_init(), fbnic_rss_reinit() and __fbnic_set_rx_mode().

The pass also re-arms itself through schedule_delayed_work(). After
fbnic_pm_suspend() drops RTNL, it goes on to call fbnic_fw_free_mbx()
and fbnic_free_irqs() and to set uc_addr0/uc_addr4 to NULL. It never
does a synchronous cancel of service_task. Could the task keep running
every HZ after that teardown, until reboot on shutdown or until
__fbnic_open() on resume?

Would it be more robust to use a driver-owned stop indication? It could
be set under RTNL in fbnic_down() or fbnic_stop() and checked by the
service task, or the work could be drained synchronously, instead of
keying on netif_running().

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