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);
> }
[ ... ]
