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