>
> 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?
This is a pre-existing issue, not introduced by this patch.
The concurrency is possible: reset/probe and RDMA servicing run on different
workqueues, and GC_IN_SERVICE does not serialize them.
The underlying probe-window issue already exists before this patch. On initial
probe, gc is zero-initialized, so rdma_teardown is clear. mana_gd_setup()
enables event processing and detects the RDMA device before mana_rdma_probe()
publishes gd->adev . A SUSPEND processed then already encounters:
if (!gd->adev || gd->is_suspended)
break;
This patch fixes a different problem: after reset, rdma_teardown previously
remained set permanently, discarding all subsequent servicing events. It also
clears stale is_suspended to prevent a later RESUME from adding a duplicate
auxiliary device. Reopening the gate makes the existing probe-window race
reachable after reset too; the patch does not resolve that race.
Moving the release store after successful add_adev() would improve
publication ordering on the reset path. However, it would not preserve the
SUSPEND request: a handler observing the closed gate executes goto out and
frees the work item without retrying. Publishing gd->adev with
release/acquire semantics alone likewise would not prevent a handler from
observing NULL before publication.
There is no automatic replay of the missed SUSPEND. A later SUSPEND can still
be processed, but the matching RESUME cannot retroactively provide the missed
quiescence.
I agree that proper serialization, or retaining pending events, is needed to
close this existing window. A mutex implementation must cover initialization
before servicing can run and must not hold the handler's mutex across
flush_workqueue() . That is separate from restoring servicing after reset.
>
> --
> Sashiko AI
> review · https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fnetdev-
> ai.bots.linux.dev%2Fsashiko%2F%23%2Fpatchset%2F20260902175153.341056
> 0-1-
> longli%2540microsoft.com&data=05%7C02%7Clongli%40microsoft.com%7Cf8
> 5ef293a80341ee9ed308df0b8adad7%7C72f988bf86f141af91ab2d7cd011db47
> %7C1%7C0%7C639242363166629979%7CUnknown%7CTWFpbGZsb3d8eyJFbX
> B0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFp
> bCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=nn4%2FxPu1A7bHbISZYnIuG
> xaoXjh%2FkS1prtwKZ4RSKao%3D&reserved=0
