Re: [PATCH v8 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[Eric Farman]

On 7/27/26 10:08 PM, Matthew Rosato wrote:
> On 7/27/26 9:35 PM, Eric Farman wrote:
> > The fsm_notoper() routine is called when the device has been
> > lost, and is (by definition) no longer operational. Since this
> > can happen asynchronously from the normal behavior of the
> > driver, the cleanup may happen when holding other locks
> > in the calling sequence (notably, the cio subchannel lock).
> >
> > Push the cleanup of the private->cp resources to a workqueue,
> > where it can be done out from under that lock sequence and
> > safely under a common locking mechanism.
>
> Nit: you mention 'safely under a common locking mechanism' but that
> doesn't actually happen until next patch.
>
> Maybe add something like 'so a subsequent patch can...'
>
> > Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c     | 9 +++++++++
> >   drivers/s390/cio/vfio_ccw_fsm.c     | 3 +--
> >   drivers/s390/cio/vfio_ccw_ops.c     | 6 +++++-
> >   drivers/s390/cio/vfio_ccw_private.h | 3 +++
> >   4 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> > index 1a095085bc72..c197ad5ab580 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
> >   		eventfd_signal(private->crw_trigger);
> >   }
> >   
> > +void vfio_ccw_notoper_todo(struct work_struct *work)
> > +{
> > +	struct vfio_ccw_private *private;
> > +
> > +	private = container_of(work, struct vfio_ccw_private, notoper_work);
> > +
> > +	cp_free(&private->cp);
> > +}
> > +
> >   /*
> >    * Css driver callbacks
> >    */
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index 4d7988ea47ef..4d47a3c7b9a0 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -170,8 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
> >   	css_sched_sch_todo(sch, SCH_TODO_UNREG);
> >   	private->state = VFIO_CCW_STATE_NOT_OPER;
> >   
> > -	/* This is usually handled during CLOSE event */
> > -	cp_free(&private->cp);
> > +	queue_work(vfio_ccw_work_q, &private->notoper_work);
> >   }
> >   
> >   /*
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> > index bc8eb485d03f..1cca3ecdae45 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -54,6 +54,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
> >   	INIT_LIST_HEAD(&private->crw);
> >   	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> >   	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> > +	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
> >   
> >   	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
> >   	if (!private->cp.guest_cp)
> > @@ -133,7 +134,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
> >   
> >   	/*
> >   	 * Ensure these work items are fully drained, so none can
> > -	 * fire after being released.
> > +	 * fire after being released. The notoper_work struct is
> > +	 * only meaningful if the device had been opened, which
> > +	 * means it would have been cleaned in an earlier close.
> >   	 */
> >   	cancel_work_sync(&private->io_work);
> >   	cancel_work_sync(&private->crw_work);
> > @@ -216,6 +219,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
> >   	 */
> >   	cancel_work_sync(&private->io_work);
> >   	cancel_work_sync(&private->crw_work);
> > +	flush_work(&private->notoper_work);
>
> Sashiko found a path where you might never open/close, so I guess we
> need to flush in both close and release after all?

Argh.

The distinction I overlooked is that while the device might not be 
opened, meaning there's no cp stuff to free, the fsm_notoper call will 
enqueue the workqueue itself anyway. It makes no distinction of whether 
there will be anything for the cp_free logic to actually do.

> I still think flush vs cancel to ensure that we process the cp_free()
> work if it's pending else we risk leaking the cp resources.

I have been going back and forth, and agree flush is probably better. 
Seems wrong to just blindly cancel it, even if there -shouldn't- be 
anything to release.

> I think this is the last issue of note, so this series is very close as
> far as I'm concerned.
>
> Thanks,
> Matt
>
> >   
> >   	vfio_ccw_unregister_dev_regions(private);
> >   }
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> > index 0501d4bbcdbd..e2256402b089 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -102,6 +102,7 @@ struct vfio_ccw_parent {
> >    * @req_trigger: eventfd ctx for signaling userspace to return device
> >    * @io_work: work for deferral process of I/O handling
> >    * @crw_work: work for deferral process of CRW handling
> > + * @notoper_work: work for deferred processing in not-operational state
> >    */
> >   struct vfio_ccw_private {
> >   	struct vfio_device vdev;
> > @@ -125,11 +126,13 @@ struct vfio_ccw_private {
> >   	struct eventfd_ctx	*req_trigger;
> >   	struct work_struct	io_work;
> >   	struct work_struct	crw_work;
> > +	struct work_struct	notoper_work;
> >   } __aligned(8);
> >   
> >   int vfio_ccw_sch_quiesce(struct subchannel *sch);
> >   void vfio_ccw_sch_io_todo(struct work_struct *work);
> >   void vfio_ccw_crw_todo(struct work_struct *work);
> > +void vfio_ccw_notoper_todo(struct work_struct *work);
> >   
> >   extern struct mdev_driver vfio_ccw_mdev_driver;
> >