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