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

[Leon Romanovsky <leon@kernel.org>]

On Thu, Apr 24, 2025 at 02:33:24AM +0000, Shiraz Saleem wrote:
> > 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.

Yes, I also think that it solves, so let's post v2 please.

Just remember that WRITE_ONCE/READ_ONCE is not a replacement for locks.

Thanks

> 
> Shiraz
>