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

[Long Li <longli@microsoft.com>]

> 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