Re: [PATCH rdma-next 4/4] net: mana: Add support for auxiliary device servicing events

Shiraz Saleem
2025-04-24T02:33:26Z

> Subject: [EXTERNAL] Re: [PATCH rdma-next 4/4] net: mana: Add support for
> auxiliary device servicing events
>
> On Mon, Apr 14, 2025 at 11:28:49AM -0700, Konstantin Taranov wrote:
> > From: Shiraz Saleem <shirazsaleem@microsoft.com>
> >
> > Handle soc servcing events which require the rdma auxiliary device
> > resources to be cleaned up during a suspend, and re-initialized during a
> resume.
> >
> > Signed-off-by: Shiraz Saleem <shirazsaleem@microsoft.com>
> > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > ---
> > .../net/ethernet/microsoft/mana/gdma_main.c | 11 +++-
> > .../net/ethernet/microsoft/mana/hw_channel.c | 19 ++++++
> > drivers/net/ethernet/microsoft/mana/mana_en.c | 60
> +++++++++++++++++++
> > include/net/mana/gdma.h | 18 ++++++
> > include/net/mana/hw_channel.h | 9 +++
> > 5 files changed, 116 insertions(+), 1 deletion(-)
>
> <...>
>
> > @@ -1474,6 +1481,8 @@ static void mana_gd_cleanup(struct pci_dev
> *pdev)
> > mana_hwc_destroy_channel(gc);
> >
> > mana_gd_remove_irqs(pdev);
> > +
> > + destroy_workqueue(gc->service_wq);
> > }
>
> <...>
>
> > +static void mana_handle_rdma_servicing(struct work_struct *work) {
> > + struct mana_service_work *serv_work =
> > + container_of(work, struct mana_service_work, work);
> > + struct gdma_dev *gd = serv_work->gdma_dev;
> > + struct device *dev = gd->gdma_context->dev;
> > + int ret;
> > +
> > + switch (serv_work->event) {
> > + case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
> > + if (!gd->adev || gd->is_suspended)
> > + break;
> > +
> > + remove_adev(gd);
> > + gd->is_suspended = true;
> > + break;
> > +
> > + case GDMA_SERVICE_TYPE_RDMA_RESUME:
> > + if (!gd->is_suspended)
> > + break;
> > +
> > + ret = add_adev(gd, "rdma");
> > + if (ret)
> > + dev_err(dev, "Failed to add adev on resume: %d\n",
> ret);
> > + else
> > + gd->is_suspended = false;
> > + break;
> > +
> > + default:
> > + dev_warn(dev, "unknown adev service event %u\n",
> > + serv_work->event);
> > + break;
> > + }
> > +
> > + kfree(serv_work);
>
> The series looks ok to me, except one question. Are you sure that it is safe to
> have not-connected and not-locked general work while
> add_adev/remove_adev can be called in parallel from different thread? For
> example getting event GDMA_SERVICE_TYPE_RDMA_SUSPEND while
> mana_gd_probe() fails or some other intervention with PCI
> (GDMA_SERVICE_TYPE_RDMA_SUSPEND and PCI shutdown).
>
> What type of protection do you have here?
>
Hi Leon,

Thanks for spotting this.

There are two cases.

-Probe / Resume
add_adev() stores gd->adev only after auxiliary_device_add() succeeds.
While gd->adev is still NULL the worker drops any GDMA_SERVICE_TYPE_RDMA_SUSPEND event, so an early suspend that arrives during probe is harmless and cannot race with the later add_adev().

-Remove / Suspend / Shutdown
During teardown the worker may still be inside add_adev()/remove_adev() while the PCI thread starts its own remove_adev().

In v2 I ll serialize them with flag + flush pattern.

void mana_rdma_remove(struct gdma_dev *gd)
{
[....]
WRITE_ONCE(gd->rdma_teardown, true); /* block new events */
flush_workqueue(gc->service_wq); /* wait running worker */

if (gd->adev)
remove_adev(gd);

[....]
}
i.e. during teardown, we stop the producer and drain the queue

and,

static void mana_handle_rdma_servicing(struct work_struct *work)
{
[....]
if (READ_ONCE(gd->rdma_teardown))
goto out;
[.....]
}
The flag blocks any new work, and flush_workqueue() waits for anything already running. This serialises the two paths and removes
the race you pointed out.

Shiraz