Re: [PATCH net] net: mana: fix reset work race with device removal

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]?part=1
---
--- Patch [1]: [PATCH net] net: mana: fix reset work race with device removal ---
commit 789cdee15635f51361a37bb4e10ca06127d69f71
Author: Fan Wu <[email protected]>

net: mana: fix reset work race with device removal

The patch makes mana_serv_reset() take the driver-core device lock around
the pci_get_drvdata() read and the whole reset sequence, drops the lock
before calling mana_serv_rescan(), and has mana_gd_remove() clear the PCI
drvdata before vfree()ing the gdma_context.  It also clears GC_IN_SERVICE
on the resume-failure exit before rescanning.

A couple of questions about the new locking and the new clear_bit().

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index e8b7ffb47eb98..ae03d7a53348e 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -591,15 +591,16 @@ static void mana_serv_fpga(struct pci_dev *pdev)
>  
>  static void mana_serv_reset(struct pci_dev *pdev)
>  {
> -	struct gdma_context *gc = pci_get_drvdata(pdev);
> +	struct gdma_context *gc;
>  	struct hw_channel_context *hwc;
>  	int ret;
>  
> +	device_lock(&pdev->dev);
> +	gc = pci_get_drvdata(pdev);

[Severity: High]
[Finding: fable-5-5-8]
[Sources: fable-5, opus-5]
Is it intended that &pdev->dev's mutex is now held across the entire reset
cycle rather than just the drvdata read?  With the new labels the lock is
only dropped at out: and rescan:, so it covers this part of
mana_serv_reset():

	mana_gd_suspend(pdev, PMSG_SUSPEND);

	msleep(MANA_SERVICE_PERIOD * 1000);

	ret = mana_gd_resume(pdev);

That is an unconditional 10 second msleep(), plus mana_gd_resume(), which
re-runs mana_gd_setup()/mana_probe()/mana_rdma_probe(); the first HWC
command there can wait up to HW_CHANNEL_WAIT_RESOURCE_TIMEOUT_MS (30000
ms) when the adapter is unresponsive.

This runs from a system_wq work item (mana_serv_func() -> mana_do_service()
-> mana_serv_reset()), so everything that serializes on the same device
mutex blocks for that whole time:

  device_release_driver_internal() -> __device_driver_lock()  (sysfs
    unbind, driver removal)
  device_shutdown()  (reboot/shutdown, takes device_lock(parent) plus
    device_lock(dev))
  drivers/base/power/main.c device_suspend()/device_resume(), which arm
    the DPM watchdog before taking device_lock(dev)
  pci_stop_and_remove_bus_device() -> pci_stop_dev() ->
    device_release_driver(), which blocks on this mutex while holding the
    global pci_rescan_remove_lock

Can this stall system suspend/resume, reboot and all PCI hotplug/rescan on
the machine for tens of seconds?  And with CONFIG_DPM_WATCHDOG set below
the hold time, would the PM core's watchdog fire because it is armed before
device_lock()?

The trigger is not driver-internal: mana_schedule_serv_work() is called
from mana_gd_process_eqe() for a device-supplied
GDMA_EQE_HWC_RESET_REQUEST, and GC_IN_SERVICE only prevents overlap, not
back-to-back requests.

Would narrowing the lock to the pci_get_drvdata() read, and getting the
lifetime guarantee from a driver-private mutex, a refcounted
gdma_context, or cancelling the service work in mana_gd_remove(), be
enough for the stated goal?

There is also a structural concern with the wide hold: under this lock the
code reaches mana_rdma_remove() (flush_workqueue(gc->service_wq)),
mana_gd_cleanup_device() (destroy_workqueue(gc->service_wq)) and
mana_remove() (disable_work_sync(&apc->queue_reset_work),
cancel_delayed_work_sync(&ac->gf_stats_work), rtnl_lock()).  Would a later
flush or cancel of the service work from the remove path then deadlock
against this lock?

>  	if (!gc) {
>  		/* Perform PCI rescan on device if GC is not set up */
>  		dev_err(&pdev->dev, "MANA service: GC not setup, rescanning\n");
> -		mana_serv_rescan(pdev);
> -		return;
> +		goto rescan;
>  	}
>  
>  	hwc = gc->hwc.driver_data;
> @@ -621,8 +622,8 @@ static void mana_serv_reset(struct pci_dev *pdev)
>  	if (ret == -ETIMEDOUT || ret == -EPROTO) {
>  		/* Perform PCI rescan on device if we failed on HWC */
>  		dev_err(&pdev->dev, "MANA service: resume failed, rescanning\n");
> -		mana_serv_rescan(pdev);
> -		return;
> +		clear_bit(GC_IN_SERVICE, &gc->flags);
> +		goto rescan;
>  	}

[Severity: Medium]
[Finding: fable-5-2-2]
[Sources: fable-5, opus-5]
Does clearing GC_IN_SERVICE before the rescan change the service gating
semantics on this exit?  Previously this path returned with the bit still
set, and it only stopped mattering because mana_serv_rescan() removed the
device and vfree()d gc (a freshly probed device starts with flags == 0).

GC_IN_SERVICE looks like the only "a service cycle is in flight" gate:

  mana_schedule_serv_work():
	if (test_and_set_bit(GC_IN_SERVICE, &gc->flags)) {
		dev_info(gc->dev, "Already in service\n");
		return -EBUSY;
	}

  mana_tx_timeout() in mana_en.c:
	/* Already in service, hence tx queue reset is not required.*/
	if (test_bit(GC_IN_SERVICE, &gc->flags))
		return;

With the bit cleared first, the flag reads "not in service" for the whole
teardown window: device_unlock() happens, then mana_serv_rescan() can block
for a long time on pci_lock_rescan_remove() and again on device_lock inside
pci_stop_and_remove_bus_device() -> pci_stop_dev() ->
device_release_driver().

Is that window reachable with the HWC still alive?  mana_gd_resume()
returns -ETIMEDOUT when mana_rdma_probe() -> mana_gd_register_device()
times out on HWC, and in that case only mana_rdma_remove() has been undone
- the HWC channel, its EQ and the MSI-X IRQs are still up and the netdevs
are still registered.  In that state two triggers can queue a second
service work:

  mana_gd_process_eqe()          (incoming GDMA_EQE_HWC_RESET_REQUEST)
      -> mana_schedule_serv_work()

  mana_gf_stats_work_handler()   (re-arms every 2 s, on -ETIMEDOUT)
      -> mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST)

Can that second work then win the unfair device_lock ahead of the in-flight
removal, observe a still non-NULL gc, and run another full
mana_gd_suspend() + msleep(10 s) + mana_gd_resume() cycle while the first
work is stuck inside pci_stop_and_remove_bus_device() holding
pci_rescan_remove_lock?

Separately, with the bit cleared, is mana_tx_timeout() still suppressed
during teardown, or can it now queue apc->queue_reset_work and drive HWC
and queue programming against an adapter that just requested a reset and is
being removed?

The commit message describes this hunk only as:

    Also clear GC_IN_SERVICE before rescanning after a failed resume, so
    this exit follows the same service-state cleanup as the other reset
    exits.

Could the changelog also mention the change in service-gating behaviour,
given this is tagged for stable?

>  
>  	if (ret)
> @@ -632,6 +633,12 @@ static void mana_serv_reset(struct pci_dev *pdev)
>  
>  out:
>  	clear_bit(GC_IN_SERVICE, &gc->flags);
> +	device_unlock(&pdev->dev);
> +	return;
> +
> +rescan:
> +	device_unlock(&pdev->dev);
> +	mana_serv_rescan(pdev);
>  }

[ ... ]