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

[PATCH net v2] 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(). is_suspended is otherwise only touched
by mana_rdma_service_handle() on the ordered service workqueue, so clear
it while rdma_teardown still gates that handler and re-open the gate with
smp_store_release(), paired with smp_load_acquire() in the handler.

Fixes: 505cc26bcae0 ("net: mana: Add support for auxiliary device servicing events")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v2:
- No functional change; the diff is identical to v1, rebased on net/main.
- 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.

v1: https://lore.kernel.org/all/20260826235940.1869565-1-longli@microsoft.com/
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 14 +++++++++++++-
 1 file changed, 13 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..56f426da728647e39f4c4ffa9c66123436297ac1 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,17 @@ 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 act on a stale value.  Pairs with the smp_load_acquire()
+	 * in mana_rdma_service_handle().
+	 */
+	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 v2] net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828001202.2110938-1-longli@microsoft.com?part=1
---
net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()

This clears gd->rdma_teardown and gd->is_suspended in mana_rdma_probe() so
that RDMA servicing events are handled again after a hardware service reset
cycle. The gate is re-opened with smp_store_release(), 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 7a1ac853e3abc..56f426da72864 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,17 @@ 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;

[Severity: Low]

Does the invariant described in the commit message hold on the initial probe
path?  The commit message says:

    is_suspended is otherwise only touched by mana_rdma_service_handle() on
    the ordered service workqueue, so clear it while rdma_teardown still
    gates that handler

That is true for the reset path, where mana_rdma_remove() set rdma_teardown
and flushed gc->service_wq first.  On mana_gd_probe() -> mana_probe() ->
mana_rdma_probe() the gate was never closed: gc comes from vzalloc(), so both
gd->rdma_teardown and gd->is_suspended are already false, and mana_gd_setup()
has already created gc->service_wq and brought up the HWC and EQs before
mana_rdma_probe() runs.

So on that path the smp_store_release() is a false -> false store and the
handler's smp_load_acquire() has no state change to order against, while the
plain write of gd->is_suspended here (and add_adev()'s plain write of
gd->adev) can run concurrently with the handler's plain accesses to the same
two fields with no lock on either side.

The outcome looks benign today, since is_suspended is already false and the
handler is a no-op in that window, but would it be worth wording the comment
and commit message so they only claim the property that actually holds?

> +
> +	/* Publish is_suspended before re-opening the gate, so the handler
> +	 * cannot act on a stale value.  Pairs with the smp_load_acquire()
> +	 * in mana_rdma_service_handle().
> +	 */
> +	smp_store_release(&gd->rdma_teardown, false);
> +
>  	err = add_adev(gd, "rdma");
>  	if (err)
>  		mana_gd_deregister_device(gd);

[Severity: Medium]

Is the gate re-opened too early here, i.e. before add_adev() has published
gd->adev?  add_adev() stores it as its last step, after
auxiliary_device_add() has already synchronously probed mana_ib:

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 work item that runs inside that window
passes the acquire load, but sees gd->adev == NULL because remove_adev()
cleared it during the preceding mana_rdma_remove():

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 event is consumed without setting gd->is_suspended, and the
matching GDMA_SERVICE_TYPE_RDMA_RESUME then also breaks on
!gd->is_suspended.  Doesn't that leave the freshly created RDMA auxiliary
device live across a servicing operation that explicitly asked for it to be
quiesced, which is the same silently-dropped-servicing behaviour this change
sets out to fix, now confined to the probe window?

Can the two contexts really overlap?  mana_serv_reset() runs on the system
workqueue via mana_schedule_serv_work()/schedule_work() (or from
mana_recovery_delayed_func()) and calls mana_gd_suspend() ->
mana_rdma_remove() then mana_gd_resume() -> mana_gd_setup() ->
mana_rdma_probe(), while the handler runs on gc->service_wq, which
mana_gd_setup() re-creates before mana_rdma_probe() is called:

mana_rdma_service_event() {
	...
	INIT_WORK(&serv_work->work, mana_rdma_service_handle);
	queue_work(gc->service_wq, &serv_work->work);

Different workqueues, and GC_IN_SERVICE only serializes further reset
requests.

Related to the new comment: the handler's read of gd->adev is a plain read
racing with add_adev()'s plain write, so "the handler cannot act on a stale
value" holds for is_suspended but not for gd->adev.

mana_rdma_remove() uses the opposite ordering, closing the gate and draining
the workqueue before touching gd->adev:

mana_rdma_remove() {
	WRITE_ONCE(gd->rdma_teardown, true);

	if (gc->service_wq)
		flush_workqueue(gc->service_wq);

	if (gd->adev)
		remove_adev(gd);

Would doing the mirror image in probe work here, i.e. keeping the
gd->is_suspended = false store where it is but issuing
smp_store_release(&gd->rdma_teardown, false) only after add_adev() has
succeeded?  Note that ordering alone still drops such an event, just via the
gate instead; serializing mana_rdma_probe(), mana_rdma_remove() and the
handler body with a mutex rather than extending the bool gate would close the
window entirely.

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

[Long Li]

> This is an AI-generated review of your patch. The human sending this email
> has considered the AI review valid, or at least plausible.
> Full review at:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fnetdev
> -
> ai.bots.linux.dev%2Fsashiko%2F%23%2Fpatchset%2F20260828001202.211093
> 8-1-
> longli%40microsoft.com%3Fpart%3D1&data=05%7C02%7Clongli%40microsoft.
> com%7C588623f27cf64c19cb1008df0911b29d%7C72f988bf86f141af91ab2d7cd
> 011db47%7C1%7C0%7C639239643786367503%7CUnknown%7CTWFpbGZsb3
> d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> joiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=fPON6yyxZubRe8V9
> 4xCzc9ZMoSxXSR0cU2Zug1r8Lck%3D&reserved=0
> ---
> net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()
> 
> This clears gd->rdma_teardown and gd->is_suspended in
> mana_rdma_probe() so that RDMA servicing events are handled again after a
> hardware service reset cycle. The gate is re-opened with smp_store_release(),
> 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 7a1ac853e3abc..56f426da72864 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,17 @@ 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;
> 
> [Severity: Low]
> 
> Does the invariant described in the commit message hold on the initial probe
> path?  The commit message says:
> 
>     is_suspended is otherwise only touched by mana_rdma_service_handle()
> on
>     the ordered service workqueue, so clear it while rdma_teardown still
>     gates that handler
> 
> That is true for the reset path, where mana_rdma_remove() set
> rdma_teardown and flushed gc->service_wq first.  On mana_gd_probe() ->
> mana_probe() ->
> mana_rdma_probe() the gate was never closed: gc comes from vzalloc(), so
> both
> gd->rdma_teardown and gd->is_suspended are already false, and
> gd->mana_gd_setup()
> has already created gc->service_wq and brought up the HWC and EQs before
> mana_rdma_probe() runs.
> 
> So on that path the smp_store_release() is a false -> false store and the
> handler's smp_load_acquire() has no state change to order against, while the
> plain write of gd->is_suspended here (and add_adev()'s plain write of
> gd->adev) can run concurrently with the handler's plain accesses to the
> gd->same
> two fields with no lock on either side.
> 
> The outcome looks benign today, since is_suspended is already false and the
> handler is a no-op in that window, but would it be worth wording the
> comment and commit message so they only claim the property that actually
> holds?

Thank you, Simon.

I will submit v3, with the commit message changed to be more accurate.

The following issue is pre-existing. The race window exists for all service events; this patch doesn't enlarge that race window or make it more visible. I'll explain more later in this email.

This patch only fixes the issue with the 2nd service event not reaching to RDMA probe.

> 
> > +
> > +	/* Publish is_suspended before re-opening the gate, so the handler
> > +	 * cannot act on a stale value.  Pairs with the smp_load_acquire()
> > +	 * in mana_rdma_service_handle().
> > +	 */
> > +	smp_store_release(&gd->rdma_teardown, false);
> > +
> >  	err = add_adev(gd, "rdma");
> >  	if (err)
> >  		mana_gd_deregister_device(gd);
> 
> [Severity: Medium]
> 
> Is the gate re-opened too early here, i.e. before add_adev() has published
> gd->adev?  add_adev() stores it as its last step, after
> auxiliary_device_add() has already synchronously probed mana_ib:
> 
> 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 work item that runs inside that
> window passes the acquire load, but sees gd->adev == NULL because
> remove_adev() cleared it during the preceding mana_rdma_remove():
> 
> 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 event is consumed without setting gd->is_suspended, and
> the matching GDMA_SERVICE_TYPE_RDMA_RESUME then also breaks on !gd-
> >is_suspended.  Doesn't that leave the freshly created RDMA auxiliary device
> live across a servicing operation that explicitly asked for it to be quiesced,
> which is the same silently-dropped-servicing behaviour this change sets out
> to fix, now confined to the probe window?
> 
> Can the two contexts really overlap?  mana_serv_reset() runs on the system
> workqueue via mana_schedule_serv_work()/schedule_work() (or from
> mana_recovery_delayed_func()) and calls mana_gd_suspend() ->
> mana_rdma_remove() then mana_gd_resume() -> mana_gd_setup() ->
> mana_rdma_probe(), while the handler runs on gc->service_wq, which
> mana_gd_setup() re-creates before mana_rdma_probe() is called:
> 
> mana_rdma_service_event() {
> 	...
> 	INIT_WORK(&serv_work->work, mana_rdma_service_handle);
> 	queue_work(gc->service_wq, &serv_work->work);
> 
> Different workqueues, and GC_IN_SERVICE only serializes further reset
> requests.
> 
> Related to the new comment: the handler's read of gd->adev is a plain read
> racing with add_adev()'s plain write, so "the handler cannot act on a stale
> value" holds for is_suspended but not for gd->adev.
> 
> mana_rdma_remove() uses the opposite ordering, closing the gate and
> draining the workqueue before touching gd->adev:
> 
> mana_rdma_remove() {
> 	WRITE_ONCE(gd->rdma_teardown, true);
> 
> 	if (gc->service_wq)
> 		flush_workqueue(gc->service_wq);
> 
> 	if (gd->adev)
> 		remove_adev(gd);
> 
> Would doing the mirror image in probe work here, i.e. keeping the
> gd->is_suspended = false store where it is but issuing
> smp_store_release(&gd->rdma_teardown, false) only after add_adev() has
> succeeded?  

No, and I'd rather not make that change.

First, this window is pre-existing and this patch does not change it.
The code that drops the event is:

        case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
                if (!gd->adev || gd->is_suspended)
                        break;

That test is untouched by this patch -- it does not appear in the diff.
It is reachable in mainline today on the initial probe path:

        gc = vzalloc()                  rdma_teardown = false, adev = NULL
        mana_gd_setup()
          alloc_ordered_workqueue()     service_wq exists
          mana_gd_setup_hwc_irqs()      HWC events are flowing
          mana_gd_detect_devices()      mana_ib.dev_id.type is set
        mana_rdma_probe()
          add_adev()                    gd->adev published last

gc is zeroed, so the gate starts clear, and mana_rdma_service_event()
already passes its dev_id.type test by then.  A SUSPEND arriving while
add_adev() is still running is dropped on a stock kernel, with or
without this patch.

What this patch changes is only that the reset path now reaches that
same pre-existing test.  Before, it stopped earlier:

        if (READ_ONCE(gd->rdma_teardown))
                goto out;

because mana_rdma_remove() set rdma_teardown and nothing ever cleared
it, so that event and every event after it were dropped for the life of
the device.  There is no input where this patch loses an event that
mainline would have delivered.

Second, on the reordering itself: the gate is a discard, not a defer.
A work item that sees it closed falls through to "out: kfree(serv_work)"
and is never re-queued, so moving the store just relocates the loss from
!gd->adev to the gate, as you noted.  It also leaves rdma_teardown set
when add_adev() fails, reinstating the stuck gate this patch fixes.

> Note that ordering alone still drops such an event, just via the
> gate instead; serializing mana_rdma_probe(), mana_rdma_remove() and the
> handler body with a mutex rather than extending the bool gate would close
> the window entirely.

Agreed, because the handler would wait rather than discard.  One trap
for whoever writes it: mana_rdma_remove() closes the gate and then
calls flush_workqueue(gc->service_wq).  If that flush ends up inside
the mutex while the handler body also takes it, it deadlocks.  The
mutex would also be held across auxiliary_device_add(), which probes
mana_ib synchronously, so it needs a lockdep run.

Since the window predates this patch, I'd rather keep this one as the
minimal fix for the stuck gate and the is_suspended leak, which is what
the Fixes: tag refers to, and do the mutex separately against net-next.


Thanks,
Long