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

[PATCH] 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>
---
 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: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless access to `gd->adev` during probe causes hardware SUSPEND events to be silently dropped, leading to a state mismatch between the host and hardware.
--

commit a413863cad4d3b91f96dce2e01b3fa3e47e6054e
Author: Long Li <longli@microsoft.com>

net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()

This commit clears gd->rdma_teardown and gd->is_suspended during
mana_rdma_probe() to allow servicing events to be handled again after a reset
cycle. It uses smp_store_release() to publish the state before adding the
auxiliary device.

> 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

[ ... ]

> @@ -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");

[Severity: High]
Could clearing the teardown flag before the auxiliary device is fully
initialized cause hardware suspend events to be silently dropped?

If a GDMA_SERVICE_TYPE_RDMA_SUSPEND event arrives from hardware immediately
after smp_store_release() clears gd->rdma_teardown, the handler in
mana_rdma_service_handle() will be able to process it.

However, because add_adev() has not yet completed and published gd->adev,
the lockless check in mana_rdma_service_handle() will see a NULL pointer:

mana_rdma_service_handle() {
        ...
        switch (serv_work->event) {
        case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
                if (!gd->adev || gd->is_suspended)
                        break;
        ...
}

Will this result in the handler breaking out early and dropping the suspend
event locklessly, leading to a state mismatch between the host and hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826235940.1869565-1-longli@microsoft.com?part=1

RE: [EXTERNAL] Re: [PATCH] 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:
> - [High] Lockless access to `gd->adev` during probe causes hardware SUSPEND
> events to be silently dropped, leading to a state mismatch between the host
> and hardware.
> --
> 
> commit a413863cad4d3b91f96dce2e01b3fa3e47e6054e
> Author: Long Li <longli@microsoft.com>
> 
> net: mana: Clear RDMA teardown and suspend state in mana_rdma_probe()
> 
> This commit clears gd->rdma_teardown and gd->is_suspended during
> mana_rdma_probe() to allow servicing events to be handled again after a
> reset cycle. It uses smp_store_release() to publish the state before adding
> the auxiliary device.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -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");
> 
> [Severity: High]
> Could clearing the teardown flag before the auxiliary device is fully initialized
> cause hardware suspend events to be silently dropped?
> 
> If a GDMA_SERVICE_TYPE_RDMA_SUSPEND event arrives from hardware
> immediately after smp_store_release() clears gd->rdma_teardown, the
> handler in
> mana_rdma_service_handle() will be able to process it.
> 
> However, because add_adev() has not yet completed and published gd-
> >adev, the lockless check in mana_rdma_service_handle() will see a NULL
> pointer:
> 
> mana_rdma_service_handle() {
>         ...
>         switch (serv_work->event) {
>         case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
>                 if (!gd->adev || gd->is_suspended)
>                         break;
>         ...
> }
> 
> Will this result in the handler breaking out early and dropping the suspend
> event locklessly, leading to a state mismatch between the host and
> hardware?

Thanks for looking at this. Some background on what the patch fixes
first, because it is the same code path.

There are two bugs, both in state that outlives a reset cycle.

gd is &gc->mana_ib, embedded in the gdma_context. A hardware service
reset runs mana_serv_reset(), which calls mana_gd_suspend() and then
mana_gd_resume() on the same pci_get_drvdata() pointer -- gc is not
freed and reallocated, so everything in gd survives the cycle.

1) rdma_teardown is set by mana_rdma_remove() and never cleared.

   mana_gd_suspend() -> mana_rdma_remove()  sets rdma_teardown = true
   mana_gd_resume()  -> mana_rdma_probe()   left it set

   From the first reset onward, mana_rdma_service_handle() returns at

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

   so every GDMA_EQE_HWC_SOC_SERVICE event is discarded for the
   remaining life of the device. RDMA suspend/resume servicing stops
   working entirely, and the host and hardware disagree permanently.

2) is_suspended has the same lifetime problem, and it is the only
   guard on the resume path -- note that the RDMA_RESUME case does not
   also test gd->adev:

        case GDMA_SERVICE_TYPE_RDMA_RESUME:
                if (!gd->is_suspended)
                        break;
                ret = add_adev(gd, "rdma");

   If a reset happens while RDMA is suspended, mana_rdma_remove() finds
   gd->adev already NULL and does not touch is_suspended, then
   mana_rdma_probe() re-adds the adev. is_suspended is still true, so a
   later resume event calls add_adev() on top of a live gd->adev.
   add_adev() assigns gd->adev last, so the pointer is overwritten and
   the previous auxiliary device stays registered but untracked.
   mana_rdma_remove() then only removes the tracked one.

   Today this is masked by bug 1: the handler never gets that far.
   Clearing rdma_teardown alone would unmask it, which is why both
   flags are cleared together.

On the window you point at:

The !gd->adev check is not modified by this patch, and the race is not
introduced by it. It already exists on the initial probe path in
mainline: gc is vzalloc()ed, so rdma_teardown starts clear, and
mana_gd_setup() creates the service workqueue and enables the HWC
interrupts (gdma_main.c) before mana_gd_probe() calls
mana_rdma_probe() -> add_adev(). A SUSPEND arriving there passes the
gate, sees gd->adev == NULL and is dropped, with no patch applied.

Before this patch the reset path did not reach that check, but only
because the gate was shut forever and the event was already being
dropped unconditionally -- along with every event after it. So there is
no input for which this patch loses an event that mainline would have
delivered; the set of dropped events strictly shrinks.

Closing the remaining add_adev() window needs probe to be serialized
against the service workqueue, or the pending request to be latched and
replayed once gd->adev is published. That is a behaviour change to the
servicing protocol rather than part of this fix, so I would prefer to
do it as a follow-up patch.

Thanks,
Long