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
---
net: mana: fix reset work race with device removal
The service work item is moved into struct gdma_context and its lifecycle
is guarded by a new serv_lock plus a waitqueue, so removal, shutdown and
the probe unwind can close admission and wait for an in-flight reset cycle
before gc is freed. The probe boundary is handled by latching reset events
that arrive while the probe is still running.
A few questions below about the new admission gates and the paths that were
not converted.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8e9bfc1d6a2ae..b91d2542cfe56 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
[ ... ]
> @@ -796,51 +899,94 @@ static void mana_recovery_delayed_func(struct
> work_struct *w)
[ ... ]
> +/* Serialize admission with retirement. Called in hard IRQ context, so
> + * serv_lock is taken with interrupts disabled and nothing that may
> + * sleep runs under it.
> + */
> int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
> {
> - struct mana_serv_work *mns_wk;
> + unsigned long flags;
> + bool busy;
[ ... ]
> dev_info(gc->dev, "Start MANA service type:%d\n", type);
> - mns_wk->pdev = to_pci_dev(gc->dev);
> - mns_wk->type = type;
> - pci_dev_get(mns_wk->pdev);
> - INIT_WORK(&mns_wk->serv_work, mana_serv_func);
> - schedule_work(&mns_wk->serv_work);
> +
> + gc->serv_type = type;
> + pci_dev_get(to_pci_dev(gc->dev));
> + queue_work(system_wq, &gc->serv_work);
> return 0;
> }
[Severity: Low]
Is system_wq the intended target here? include/linux/workqueue.h carries:
extern struct workqueue_struct *system_wq; /* use system_percpu_wq, this
will be removed */
and system_wq is created with __WQ_DEPRECATED, so __queue_work() emits:
workqueue: work func %ps enqueued on deprecated workqueue. Use
system_{percpu|dfl}_wq instead.
The schedule_work() that was removed expanded to queue_work(system_percpu_wq,
...), so this hunk seems to newly pick the deprecated alias and a warning on
every service cycle.
Given that mana_serv_reset()/mana_serv_fpga() do msleep(MANA_SERVICE_PERIOD *
1000) and PCI stop/rescan, would system_long_wq (or system_dfl_long_wq) be a
better fit than the per-CPU queue?
[ ... ]
> @@ -2558,22 +2697,26 @@ static int mana_gd_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>
> err = mana_rdma_probe(&gc->mana_ib);
> if (err)
> - goto cleanup_mana;
> + goto service_quiesce;
>
> /*
> * If a hardware reset event has occurred over HWC during probe,
> * rollback and perform hardware reset procedure.
> */
> - if (test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
> + rollback = mana_service_probe_complete(gc);
> + if (rollback) {
> err = -EPROTO;
> - goto cleanup_mana_rdma;
> + goto service_quiesce;
> }
>
> return 0;
>
> -cleanup_mana_rdma:
> +service_quiesce:
> + /* The stats work can admit service once mana_probe() has run:
> + * retire an in-flight cycle before any teardown.
> + */
> + mana_service_quiesce(gc);
[Severity: Low]
Does this comment still describe the code after the change in mana_en.c?
The stats work handler now only admits service when
mana_service_probe_done(gc) is true, and serv_probe_done is set solely by
mana_service_probe_complete(), which runs after mana_rdma_probe() has
succeeded. On the path that jumps here because mana_rdma_probe() failed,
serv_probe_done is still false, so the stats work cannot have admitted a
cycle at all, and the EQ producer is gated on the same flag.
Could the comment be reworded to match the gate, or is a producer intended
that can still admit during probe?
[ ... ]
> @@ -2687,6 +2837,9 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
>
> dev_info(&pdev->dev, "Shutdown was called\n");
>
> + /* Shutdown tears down the same HW paths as remove(). */
> + mana_service_quiesce(gc);
> +
> mana_rdma_remove(&gc->mana_ib);
> mana_remove(&gc->mana, true);
[Severity: High]
This isn't a bug introduced by this patch, since the old GC_IN_SERVICE bit
was ignored by the PM callbacks too, but should mana_gd_suspend() and
mana_gd_resume() (installed as mana_driver.suspend/.resume) get the same
treatment as remove() and shutdown()?
An admitted cycle runs:
mana_serv_reset()
mana_gd_suspend(pdev, PMSG_SUSPEND);
msleep(MANA_SERVICE_PERIOD * 1000);
ret = mana_gd_resume(pdev);
with serv_in_flight set for that whole 10+ second window, and the work sits
on system_wq, which is not WQ_FREEZABLE. If the guest enters system suspend
or hibernate during that window, pci_pm_suspend() -> pci_legacy_suspend()
calls mana_gd_suspend() a second time on the same gc, and neither PM callback
consults serv_in_flight/serv_removing or calls mana_service_quiesce().
Both threads then reach mana_hwc_destroy_channel(), whose guard is not atomic
with the clearing store:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
struct hw_channel_context *hwc = gc->hwc.driver_data;
if (!hwc)
return;
...
kfree(hwc);
gc->hwc.driver_data = NULL;
gc->hwc.gdma_context = NULL;
vfree(gc->cq_table);
gc->cq_table = NULL;
}
mana_gd_cleanup_device() has the same shape around gc->service_wq:
if (gc->service_wq) {
destroy_workqueue(gc->service_wq);
gc->service_wq = NULL;
}
Can this interleaving double-free hwc and gc->cq_table, or call
destroy_workqueue() twice on gc->service_wq? And can mana_gd_resume() from
the service cycle re-arm DMA/MMIO on a device the PM core has already
suspended?
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 591fb4191d90d..0eb23aea2e139 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -4060,10 +4060,13 @@ static void mana_gf_stats_work_handler(struct
> work_struct *work)
> memset(&ac->hc_stats, 0, sizeof(ac->hc_stats));
> dev_warn(gc->dev,
> "Gf stats wk handler: gf stats query timed out.\n");
> - /* As HWC timed out, indicating a faulty HW state and needs a
> - * reset.
> + /* As HWC timed out, indicating a faulty HW state and
> + * needs a reset. Never admit service work before the probe
> + * has completed: a probe that is failing unwinds netdevs and
> + * the HWC channel itself and cannot drain a cycle.
> */
> - mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
> + if (mana_service_probe_done(gc))
> + mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
> return;
> }
> schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);
[Severity: Medium]
When mana_service_probe_done(gc) is false here, is the reset request lost for
good?
mana_probe() arms this work with a MANA_GF_STATS_PERIOD delay, while
serv_probe_done is only set later, at the tail of mana_gd_probe():
err = mana_rdma_probe(&gc->mana_ib);
if (err)
goto service_quiesce;
...
rollback = mana_service_probe_complete(gc);
So the handler can run with serv_probe_done still false if mana_rdma_probe()
(register device plus the synchronous mana_ib adev probe and its HWC
requests) takes longer than that delay.
In that window an -ETIMEDOUT from mana_query_gf_stats() takes the new branch,
no cycle is admitted, serv_during_probe is not set so
mana_service_probe_complete() reports no rollback, and the return also skips
the schedule_delayed_work() re-arm. If the probe then succeeds, is there any
remaining path that retries? The recovery list in mana_gd_probe() is only
populated when the probe itself fails with -ETIMEDOUT/-EPROTO, and
hwc_timeout_occurred only feeds a warning, so hc_stats would stay zeroed for
the life of the binding and a wedged HWC would never be reset. Before this
patch the request was unconditional and the resulting reset cycle re-armed
the stats work through mana_probe(resuming=true).
The EQ producer solves the same race by latching under serv_lock:
admit = gc->serv_probe_done;
first = !gc->serv_during_probe;
if (!admit)
gc->serv_during_probe = true;
Would routing this producer through the same latch-or-admit helper work, or
alternatively keeping the delayed work re-armed so the request is retried
once serv_probe_done is set?
