RE: [EXTERNAL] Re: [PATCH net v2] 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:
- [High] Lockless gate reopening in `mana_rdma_probe()` exposes a race
window that can silently drop hardware service events.
--

commit 92507b04de784a341e9ea7d0275d9b3dbe00e839
Author: Long Li [off-list ref]

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

This commit clears the rdma_teardown and is_suspended flags during probe
to ensure hardware service events are handled correctly after a reset cycle.

Link:
https://lore.ke/
rnel.org%2Fall%2F20260826235940.1869565-1-
longli%40microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com%7Ce
4ad1794c21a42eae10608df0562428a%7C72f988bf86f141af91ab2d7cd011db4
7%7C1%7C0%7C639235591719595495%7CUnknown%7CTWFpbGZsb3d8eyJFb
XB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF
pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=DgiM6eURZJPqg1Kw%2F107
aSEqL3jkvp3JzFytQ6oXEJ4%3D&reserved=0

quoted

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

[ ... ]

quoted

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

quoted

+    * 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]
Does opening the rdma_teardown gate here create a race window where
hardware service events can be silently dropped?

Because mana_gd_register_device(gd) was called earlier in this function, the
hardware can now generate events. If an event like
GDMA_SERVICE_TYPE_RDMA_SUSPEND fires immediately after
smp_store_release() clears rdma_teardown, the ordered workqueue will run
mana_rdma_service_handle().

The handler will pass the rdma_teardown check but evaluate !gd->adev as
true because add_adev() hasn't completed yet:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_rdma_service_handl
e() {
      ...
      case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
              if (!gd->adev || gd->is_suspended)
                      break;
      ...
}

This causes the event to be dropped, which could create a permanent state
inconsistency between the OS and the hardware. Should the gate be opened
after add_adev() successfully assigns gd->adev?

quoted

    if (err)
            mana_gd_deregister_device(gd);

--
Sashiko AI
review * https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%25
2Fsashiko.dev%2F%23%2Fpatchset%2F20260828001202.2110938-1-
longli%40microsoft.com%3Fpart%3D1&data=05%7C02%7Clongli%40microsoft.
com%7Ce4ad1794c21a42eae10608df0562428a%7C72f988bf86f141af91ab2d7c
d011db47%7C1%7C0%7C639235591719616083%7CUnknown%7CTWFpbGZsb
3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
OIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=3TUhCla0LzwnJhxJ
NJ5xptY5YUbnLeiax%2FT4HpHRpFA%3D&reserved=0

This is not an issue introduced by this patch.

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