[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

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

[Long Li]

> 
> 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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  2 Sep 2026 10:51:53 -0700 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v3] net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()
    https://git.kernel.org/netdev/net/c/f6d61fe4c19c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html