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

[Eric Farman <farman@linux.ibm.com>]

On 7/27/26 5:54 PM, Matthew Rosato wrote:
> On 7/27/26 3:22 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
>> (soon) under its own serialization mechanism.
> 
> This part of the commit message needs updating now that you are not
> introducing a cp_mutex.
> 
>>
>> Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c     | 9 +++++++++
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 3 +--
>>   drivers/s390/cio/vfio_ccw_ops.c     | 3 +++
>>   drivers/s390/cio/vfio_ccw_private.h | 3 +++
>>   4 files changed, 16 insertions(+), 2 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 bd488e40e153..8ec6b175d991 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)
>> @@ -139,6 +140,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>>   	/* Should be empty, but just in case */
>>   	cancel_work_sync(&private->io_work);
>>   	cancel_work_sync(&private->crw_work);
>> +	cancel_work_sync(&private->notoper_work);
> 
> Sashiko mentions (and I agree) that you can accidentally leak the cp
> resources if you manage to cancel the pending notoper_work before it runs.
> 
> Since you are not running on a workqueue thread, it should be OK to
> flush instead to make sure that the pending item is executed.
> 
> A subsequent cancel should not be necessary, since we know that only 1
> instance of vfio_ccw_notoper_todo() will ever be queued because we queue
> it on the first time the device goes NOT_OPER.  Based on the FSM, once
> the device goes NOT_OPER, subsequent transitions into NOT_OPER are
> treated as a NOP and there is no way to transition to a different state
> other than NOT_OPER until the device is released, so you can't go from
> NOT_OPER->x->NOT_OPER.

Agreed, this should be flush not cancel.

Though, the piece where that's important is the call in close_dev, which 
is the counterpart to the open_device path which allows a cp to be 
queued in the first place. A close will have preceded a release, which 
means there will no longer be any cp's outstanding, and userspace will 
have been unable to submit new ones without performing another open.

The other two elements (io_work and crw_work) would be queued in 
response to a hardware event (an interrupt and a CRW, respectively), 
which could come asynchronously from all of this, so needs to be in both 
close and release.

> 
> Would probably be good to include a (less verbose) comment to that
> effect along with the flush.
> 

Agreed; thanks.

>>   
>>   	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
>>   	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
>> @@ -209,6 +211,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>>   
>>   	cancel_work_sync(&private->io_work);
>>   	cancel_work_sync(&private->crw_work);
>> +	cancel_work_sync(&private->notoper_work);
>>   
>>   	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;
>>   
>