Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0
- [Medium] In mana_rdma_probe()
(drivers/net/ethernet/microsoft/mana/mana_en.c:4296) the servicing…
--
net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()
mana_rdma_remove() sets gd->rdma_teardown and nothing ever clears it, so
after the first hardware service reset every GDMA_EQE_HWC_SOC_SERVICE event
returns early. This patch clears gd->is_suspended and re-opens the gate
with smp_store_release() in mana_rdma_probe(), paired with a new
smp_load_acquire() in mana_rdma_service_handle().
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7a1ac853e3ab..d8e847df7a44 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -3983,7 +3983,8 @@ static void mana_rdma_service_handle(struct work_struct
> *work)
> struct device *dev = gd->gdma_context->dev;
> int ret;
>
> - if (READ_ONCE(gd->rdma_teardown))
> + /* Pairs with the smp_store_release() in mana_rdma_probe(). */
> + if (smp_load_acquire(&gd->rdma_teardown))
> goto out;
>
> switch (serv_work->event) {
[ ... ]
> @@ -4279,6 +4280,21 @@ int mana_rdma_probe(struct gdma_dev *gd)
> if (err)
> return err;
>
> + /* Clear the state left by a previous mana_rdma_remove() so servicing
> + * events are handled again after a reset cycle.
> + */
> + gd->is_suspended = false;
> +
> + /* Publish is_suspended before re-opening the gate, so the handler
> + * cannot observe an open gate with a stale is_suspended. Pairs
> + * with the smp_load_acquire() in mana_rdma_service_handle(). This
> + * matters on the reset path, where mana_rdma_remove() closed the
> + * gate and drained the workqueue; on the initial probe path the
> + * gate was never closed and both flags are already clear. It does
> + * not order gd->adev, which add_adev() publishes below.
> + */
> + smp_store_release(&gd->rdma_teardown, false);
> +
> err = add_adev(gd, "rdma");
> if (err)
> mana_gd_deregister_device(gd);
[Severity: Medium]
Would it be better to open the gate after add_adev() succeeds rather than
before it?
As written, the store that re-enables the handler happens while gd->adev is
still NULL from the remove half of the reset cycle. The publication happens
later, at the end of add_adev():
drivers/net/ethernet/microsoft/mana/mana_en.c:add_adev() {
...
ret = auxiliary_device_add(adev);
if (ret)
goto add_fail;
gd->adev = adev;
...
}
A GDMA_SERVICE_TYPE_RDMA_SUSPEND handled in that window passes the gate and
then hits the gd->adev test in mana_rdma_service_handle():
case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
if (!gd->adev || gd->is_suspended)
break;
remove_adev(gd);
gd->is_suspended = true;
break;
so the suspend request is discarded, probe finishes with a live aux device
and is_suspended == false, and the matching RESUME is then also swallowed by
the "if (!gd->is_suspended) break;" test. Is the resulting state (host asked
for a quiesce, RDMA aux device still live) recoverable without another reset?
Is the concurrency here real on the reset path? From what I can follow, the
probe runs from system_wq while the handler runs on gc->service_wq:
mana_schedule_serv_work()
schedule_work(&mns_wk->serv_work) /* system_wq */
mana_serv_reset()
mana_gd_suspend() -> mana_rdma_remove()
mana_gd_resume() -> mana_gd_setup()
-> mana_probe()
-> mana_rdma_probe()
and mana_gd_resume() calls mana_gd_setup() first, which re-creates
gc->service_wq, re-arms the HWC IRQs, re-creates the HW channel and
re-detects devices so gc->mana_ib.dev_id.type is valid again, all before
mana_rdma_probe() runs. So an event arriving during probe reaches
mana_hwc_init_event_handler() -> mana_rdma_service_event() ->
queue_work(gc->service_wq) and mana_rdma_service_handle() can execute on
another CPU while mana_rdma_probe() is still in add_adev(). Since the event
timing is driven by the host, is that window reachable in practice?
In the opposite interleaving, the handler reads gd->adev with a plain load
and immediately destroys the object through remove_adev()
(auxiliary_device_delete/uninit, mana_adev_idx_free) while probe is still
finishing. The new acquire load only orders is_suspended against the
release; the "gd->adev = adev;" store has no release pairing with the
handler's load. Would publishing gd->adev with release semantics, or moving
the smp_store_release() below a successful add_adev(), cover this too?
There is also the question of what rdma_teardown now is. Before this patch
it was a write-once teardown flag; with the clear in mana_rdma_probe() it
becomes a gate that mana_rdma_remove() closes on entry:
WRITE_ONCE(gd->rdma_teardown, true);
if (gc->service_wq)
flush_workqueue(gc->service_wq);
and that mana_rdma_probe() re-opens on the way back in. That is a flag
guarding a code region rather than expressing device state. Would a mutex
held across mana_rdma_probe(), mana_rdma_remove() and
mana_rdma_service_handle() be a better fit here, since it would also remove
the window the comment documents instead of describing it?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902175153.3410560-1-longli%40microsoft.com
