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

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

[Long Li]

mana_rdma_remove() sets gd->rdma_teardown to stop
mana_rdma_service_handle() from acting on servicing events, but nothing
ever clears it. A hardware service reset (GDMA_EQE_HWC_RESET_REQUEST)
goes through mana_gd_suspend() -> mana_rdma_remove() and mana_gd_resume()
-> mana_rdma_probe(), so from the first reset onwards every
GDMA_EQE_HWC_SOC_SERVICE event returns early and RDMA suspend/resume
servicing is silently dropped for the life of the device.

gd->is_suspended has the same problem: it is set when servicing removes
the adev and is cleared only by a matching resume. A reset while RDMA is
suspended re-adds the adev but leaves is_suspended set, so a later resume
event calls add_adev() on top of a live gd->adev and leaks it. This is
currently masked by the rdma_teardown bug.

Clear both in mana_rdma_probe(). On the reset path mana_rdma_remove()
has closed the gate and drained the service workqueue, so clear
is_suspended first and re-open the gate with smp_store_release(), paired
with smp_load_acquire() in the handler, so the handler cannot observe an
open gate with a stale is_suspended. On the initial probe path the gate
was never closed and both flags are already clear.

This does not order gd->adev, which add_adev() publishes afterwards. A
servicing event arriving in that window is still dropped, as it is in
mainline today on the initial probe path; closing it needs probe and the
handler to be serialized and is left to a separate change.

Fixes: 505cc26bcae0 ("net: mana: Add support for auxiliary device servicing events")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v3:
- Reword the comment and commit message so they only claim what holds:
  the "gate is still closed" justification applies to the reset path,
  not to the initial probe path where gc is zeroed and the gate was
  never closed. Note that the release/acquire pair orders is_suspended
  but not gd->adev. Reported by the AI review of v2.
- No functional change.

Changes in v2:
- Target the net tree explicitly in the subject prefix; v1 omitted it and
  the netdev CI guessed net-next, where the Fixes: tag is not evaluated.
- CC Shiraz Saleem, the author of the commit named in the Fixes: tag, as
  reported by netdev/cc_maintainers.

v2: https://lore.kernel.org/all/20260828001202.2110938-1-longli@microsoft.com/
v1: https://lore.kernel.org/all/20260826235940.1869565-1-longli@microsoft.com/
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 7a1ac853e3abcd28c4a1e5c6987ec631a18ad840..d8e847df7a44b2758b428d49b2b060bfd94b0e58 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);
-- 
2.43.0

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

[netdev-bot+sashiko]

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