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

[Simon Horman <horms@kernel.org>]

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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828001202.2110938-1-longli@microsoft.com?part=1
---
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 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 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?

> +
> +	/* 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?  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.