Re: [PATCH v7 08/10] s390/vfio_ccw: move cp cleanup out of not operational
From: Eric Farman
Date: Mon Jul 27 2026 - 20:56:42 EST
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@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 | 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;