> 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 <[email protected]>
>
> 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
>
> > 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]
> 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?
>
> > 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
