Re: [PATCH net v3] net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()

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().

[ ... ]

[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