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

[Matthew Rosato]

On 7/27/26 11:30 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
> a future patch can safely manage the locking requirements.
> 
> Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>

...

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index d361d1fde3a0..1df6d649565b 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)
> @@ -134,9 +135,16 @@ 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.
> +	 *
> +	 * notoper_work should have nothing to do here, because only
> +	 * open devices could have channel_program resources in use
> +	 * and those would be released during close. Nevertheless,
> +	 * call flush here as well to be certain anything that was
> +	 * allocated is freed.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	flush_work(&private->notoper_work);

Sashiko still seems grumpy about this, but it largely seems fine to me.

We know that notoper_work will have either 0 or 1 task queued to it
(never more than 1) and that for the life a device a task would only be
queued to it exactly once (on entering NOT_OPER the first time, which it
a terminal state e.g. the FSM does not allow transferring to any other
state).  It is impossible to "re-enter" NOT_OPER and queue a second
notoper_work.  Therefore once not_oper has been flushed once (either
here or in close_dev()), nothing will add another.

The only scneario I'm trying to think about is whether it is still
theoretically possible to go into the NOT_OPER state after this
flush_work().  Which wouldn't be solved by a cancel_work_sync here
either btw.

This window would be very small and highly unlikely + this otherwise
closes the more obvious issues so with that:

Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>