[PATCH v2 00/10] s390/vfio-ccw rework

[PATCH v2 00/10] s390/vfio-ccw rework

[Eric Farman]

Last autumn, Jason Gunthorpe proposed some rework of vfio-ccw [1],
to better fit with the new mdev API (thank you!). Part of that
series was pulled for kernel 5.16 [2], but the complexities of
the remaining patches got them hung up behind other work.

I dusted off that work a couple weeks ago (see v1), but am going
to split that further with the goal of this part to clean up the
existing device lifecycle and FSM used by vfio-ccw. The remaining
work runs into conflicts with other work (notably [2]), so I'd
like to propose this first group without that hangup. This makes
the behavior/usage of the FSM state a lot more consistent than
it is today.

This is all internal to vfio-ccw, with the exception of removal
of mdev_uuid from include/linux/mdev.h in patch 1.
@Kirti, I can drop that hunk if that's a concern for you.

One potential conflict still exists on patch 6, with [3].

v1->v2:
 - Rebased to v5.19-rc2
   - Patch 1: Remove mdev.h from vfio_ccw_fsm.c
   - Patch 4: Earlier patches removed meaningful users of private->mdev,
     leaving this as a very simple cleanup
 - [JG,MR] Added r-b's (Thank You!)
 - [MR] Patch 1: Update commit message
 - [MR] Patch 2: Add comment regarding interrupt types changing FSM state
 - [MR] Patch 5: Drop Fixes tag
 - [JG] Drop patch for clearing drvdata on mdev remove
 - [EF] Defer items to later series
   - The "if !private" patch, and JG's associated comment on patch 7;
     entire process requires further investigation
   - The vfio-mdev rework patches (to [2])
   - The "tie vfio_ccw_private to mdev lifecycle" patches
v1: https://lore.kernel.org/r/20220602171948.2790690-1-farman@linux.ibm.com/

Footnotes:
[1] https://lore.kernel.org/r/0-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
[2] https://lore.kernel.org/r/20220603063328.3715-1-hch@lst.de/
[3] https://lore.kernel.org/r/165471414407.203056.474032786990662279.stgit@omen/

Cc: Kirti Wankhede <kwankhede@nvidia.com>

Eric Farman (9):
  vfio/ccw: Fix FSM state if mdev probe fails
  vfio/ccw: Do not change FSM state in subchannel event
  vfio/ccw: Remove private->mdev
  vfio/ccw: Pass enum to FSM event jumptable
  vfio/ccw: Flatten MDEV device (un)register
  vfio/ccw: Create an OPEN FSM Event
  vfio/ccw: Create a CLOSE FSM event
  vfio/ccw: Refactor vfio_ccw_mdev_reset
  vfio/ccw: Move FSM open/close to MDEV open/close

Michael Kawano (1):
  vfio/ccw: Remove UUID from s390 debug log

 drivers/s390/cio/vfio_ccw_async.c   |  1 -
 drivers/s390/cio/vfio_ccw_drv.c     | 59 ++++++-------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 89 ++++++++++++++++++++++++-----
 drivers/s390/cio/vfio_ccw_ops.c     | 77 ++++++++-----------------
 drivers/s390/cio/vfio_ccw_private.h |  9 +--
 include/linux/mdev.h                |  5 --
 6 files changed, 117 insertions(+), 123 deletions(-)

-- 
2.32.0

[PATCH v2 01/10] vfio/ccw: Remove UUID from s390 debug log

[Eric Farman]

From: Michael Kawano <mkawano@linux.ibm.com>

As vfio-ccw devices are created/destroyed, the uuid of the associated
mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
This is because a pointer to the UUID is stored instead of the UUID
itself, and that memory may have been repurposed if/when the logs are
examined. The result is usually garbage UUID data in the logs, though
there is an outside chance of an oops happening here.

Simply remove the UUID from the traces, as the subchannel number will
provide useful configuration information for problem determination,
and is stored directly into the log instead of a pointer.

As we were the only consumer of mdev_uuid(), remove that too.

Cc: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
Fixes: 60e05d1cf0875 ("vfio-ccw: add some logging")
Fixes: b7701dfbf9832 ("vfio-ccw: Register a chp_event callback for vfio-ccw")
[farman: reworded commit message, added Fixes: tags]
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c |  5 ++---
 drivers/s390/cio/vfio_ccw_fsm.c | 26 ++++++++++++--------------
 drivers/s390/cio/vfio_ccw_ops.c |  8 ++++----
 include/linux/mdev.h            |  5 -----
 4 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ee182cfb467d..35055eb94115 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -14,7 +14,6 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/slab.h>
-#include <linux/uuid.h>
 #include <linux/mdev.h>
 
 #include <asm/isc.h>
@@ -358,8 +357,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 		return 0;
 
 	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
-	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
-			   mdev_uuid(private->mdev), sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: mask=0x%x event=%d\n",
+			   sch->schid.cssid,
 			   sch->schid.ssid, sch->schid.sch_no,
 			   mask, event);
 
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 8483a266051c..bbcc5b486749 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -10,7 +10,6 @@
  */
 
 #include <linux/vfio.h>
-#include <linux/mdev.h>
 
 #include "ioasm.h"
 #include "vfio_ccw_private.h"
@@ -242,7 +241,6 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	union orb *orb;
 	union scsw *scsw = &private->scsw;
 	struct ccw_io_region *io_region = private->io_region;
-	struct mdev_device *mdev = private->mdev;
 	char *errstr = "request";
 	struct subchannel_id schid = get_schid(private);
 
@@ -256,8 +254,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		if (orb->tm.b) {
 			io_region->ret_code = -EOPNOTSUPP;
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): transport mode\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: transport mode\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no);
 			errstr = "transport mode";
 			goto err_out;
@@ -265,8 +263,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		io_region->ret_code = cp_init(&private->cp, orb);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_init=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: cp_init=%d\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
 			errstr = "cp init";
@@ -276,8 +274,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		io_region->ret_code = cp_prefetch(&private->cp);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_prefetch=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: cp_prefetch=%d\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
 			errstr = "cp prefetch";
@@ -289,8 +287,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		io_region->ret_code = fsm_io_helper(private);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): fsm_io_helper=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: fsm_io_helper=%d\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
 			errstr = "cp fsm_io_helper";
@@ -300,16 +298,16 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		return;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
 		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): halt on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+				   "sch %x.%x.%04x: halt on io_region\n",
+				   schid.cssid,
 				   schid.ssid, schid.sch_no);
 		/* halt is handled via the async cmd region */
 		io_region->ret_code = -EOPNOTSUPP;
 		goto err_out;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
 		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): clear on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+				   "sch %x.%x.%04x: clear on io_region\n",
+				   schid.cssid,
 				   schid.ssid, schid.sch_no);
 		/* clear is handled via the async cmd region */
 		io_region->ret_code = -EOPNOTSUPP;
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index b49e2e9db2dc..0e05bff78b8e 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -131,8 +131,8 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	private->mdev = mdev;
 	private->state = VFIO_CCW_STATE_IDLE;
 
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: create\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
+			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
@@ -154,8 +154,8 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
 
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: remove\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
+			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index bb539794f54a..47ad3b104d9e 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -65,11 +65,6 @@ struct mdev_driver {
 	struct device_driver driver;
 };
 
-static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
-
 extern struct bus_type mdev_bus_type;
 
 int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver);
-- 
2.32.0

[PATCH v2 02/10] vfio/ccw: Fix FSM state if mdev probe fails

[Eric Farman]

The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
and this routine converts it to IDLE as part of its processing.
The error exit sets it to IDLE (again) but clears the private->mdev
pointer.

The FSM should of course be managing the state itself, but the
correct thing for vfio_ccw_mdev_probe() to do would be to put
the state back the way it found it.

The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
can be removed, since the distinction is unnecessary at this point.

Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 5 +++--
 drivers/s390/cio/vfio_ccw_ops.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 35055eb94115..179eb614fa5b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -106,9 +106,10 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 	/*
 	 * Reset to IDLE only if processing of a channel program
 	 * has finished. Do not overwrite a possible processing
-	 * state if the final interrupt was for HSCH or CSCH.
+	 * state if the interrupt was unsolicited, or if the final
+	 * interrupt was for HSCH or CSCH.
 	 */
-	if (private->mdev && cp_is_finished)
+	if (cp_is_finished)
 		private->state = VFIO_CCW_STATE_IDLE;
 
 	if (private->io_trigger)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 0e05bff78b8e..9a05dadcbb75 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
 	private->mdev = NULL;
-	private->state = VFIO_CCW_STATE_IDLE;
+	private->state = VFIO_CCW_STATE_STANDBY;
 	return ret;
 }
 
-- 
2.32.0

[PATCH v2 03/10] vfio/ccw: Do not change FSM state in subchannel event

[Eric Farman]

The routine vfio_ccw_sch_event() is tasked with handling subchannel events,
specifically machine checks, on behalf of vfio-ccw. It correctly calls
cio_update_schib(), and if that fails (meaning the subchannel is gone)
it makes an FSM event call to mark the subchannel Not Operational.

If that worked, however, then it decides that if the FSM state was already
Not Operational (implying the subchannel just came back), then it should
simply change the FSM to partially- or fully-open.

Remove this trickery, since a subchannel returning will require more
probing than simply "oh all is well again" to ensure it works correctly.

Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state machine")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 179eb614fa5b..279ad2161f17 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -301,19 +301,11 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	if (work_pending(&sch->todo_work))
 		goto out_unlock;
 
-	if (cio_update_schib(sch)) {
-		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
-		rc = 0;
-		goto out_unlock;
-	}
-
-	private = dev_get_drvdata(&sch->dev);
-	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
-		private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
-				 VFIO_CCW_STATE_STANDBY;
-	}
 	rc = 0;
 
+	if (cio_update_schib(sch))
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
+
 out_unlock:
 	spin_unlock_irqrestore(sch->lock, flags);
 
-- 
2.32.0

[PATCH v2 04/10] vfio/ccw: Remove private->mdev

[Eric Farman]

There are no remaining users of private->mdev. Remove it.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_async.c   | 1 -
 drivers/s390/cio/vfio_ccw_ops.c     | 3 ---
 drivers/s390/cio/vfio_ccw_private.h | 2 --
 3 files changed, 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 7a838e3d7c0f..420d89ba7f83 100644
--- a/drivers/s390/cio/vfio_ccw_async.c
+++ b/drivers/s390/cio/vfio_ccw_async.c
@@ -8,7 +8,6 @@
  */
 
 #include <linux/vfio.h>
-#include <linux/mdev.h>
 
 #include "vfio_ccw_private.h"
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 9a05dadcbb75..81377270d4a7 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -128,7 +128,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_init_group_dev(&private->vdev, &mdev->dev,
 			    &vfio_ccw_dev_ops);
 
-	private->mdev = mdev;
 	private->state = VFIO_CCW_STATE_IDLE;
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
@@ -145,7 +144,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 err_atomic:
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
-	private->mdev = NULL;
 	private->state = VFIO_CCW_STATE_STANDBY;
 	return ret;
 }
@@ -170,7 +168,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	vfio_uninit_group_dev(&private->vdev);
 	cp_free(&private->cp);
-	private->mdev = NULL;
 	atomic_inc(&private->avail);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 7272eb788612..12b5537d478f 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -73,7 +73,6 @@ struct vfio_ccw_crw {
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
  * @avail: available for creating a mediated device
- * @mdev: pointer to the mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
  * @io_mutex: protect against concurrent update of I/O regions
@@ -97,7 +96,6 @@ struct vfio_ccw_private {
 	int			state;
 	struct completion	*completion;
 	atomic_t		avail;
-	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
 	struct mutex		io_mutex;
-- 
2.32.0

[PATCH v2 05/10] vfio/ccw: Pass enum to FSM event jumptable

[Eric Farman]

The FSM has an enumerated list of events defined.
Use that as the argument passed to the jump table,
instead of a regular int.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_private.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 12b5537d478f..5c128eec596b 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -156,7 +156,7 @@ typedef void (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
 extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
 
 static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
-				     int event)
+				      enum vfio_ccw_event event)
 {
 	trace_vfio_ccw_fsm_event(private->sch->schid, private->state, event);
 	vfio_ccw_jumptable[private->state][event](private, event);
-- 
2.32.0

[PATCH v2 06/10] vfio/ccw: Flatten MDEV device (un)register

[Eric Farman]

The vfio_ccw_mdev_(un)reg routines are merely vfio-ccw routines that
pass control to mdev_(un)register_device. Since there's only one
caller of each, let's just call the mdev routines directly.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     |  4 ++--
 drivers/s390/cio/vfio_ccw_ops.c     | 10 ----------
 drivers/s390/cio/vfio_ccw_private.h |  3 ---
 3 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 279ad2161f17..fe87a2652a22 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -240,7 +240,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	private->state = VFIO_CCW_STATE_STANDBY;
 
-	ret = vfio_ccw_mdev_reg(sch);
+	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver);
 	if (ret)
 		goto out_disable;
 
@@ -262,7 +262,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
 	vfio_ccw_sch_quiesce(sch);
-	vfio_ccw_mdev_unreg(sch);
+	mdev_unregister_device(&sch->dev);
 
 	dev_set_drvdata(&sch->dev, NULL);
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 81377270d4a7..a7ea9358e461 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -654,13 +654,3 @@ struct mdev_driver vfio_ccw_mdev_driver = {
 	.remove = vfio_ccw_mdev_remove,
 	.supported_type_groups  = mdev_type_groups,
 };
-
-int vfio_ccw_mdev_reg(struct subchannel *sch)
-{
-	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver);
-}
-
-void vfio_ccw_mdev_unreg(struct subchannel *sch)
-{
-	mdev_unregister_device(&sch->dev);
-}
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 5c128eec596b..4cfdd5fc0961 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -117,9 +117,6 @@ struct vfio_ccw_private {
 	struct work_struct	crw_work;
 } __aligned(8);
 
-extern int vfio_ccw_mdev_reg(struct subchannel *sch);
-extern void vfio_ccw_mdev_unreg(struct subchannel *sch);
-
 extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
 
 extern struct mdev_driver vfio_ccw_mdev_driver;
-- 
2.32.0

[PATCH v2 07/10] vfio/ccw: Create an OPEN FSM Event

[Eric Farman]

Move the process of enabling a subchannel for use by vfio-ccw
into the FSM, such that it can manage the sequence of lifecycle
events for the device.

That is, if the FSM state is NOT_OPER(erational), then do the work
that would enable the subchannel and move the FSM to STANDBY state.
An attempt to perform this event again from any of the other operating
states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device back
to NOT_OPER so the configuration process can be started again.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     |  7 ++-----
 drivers/s390/cio/vfio_ccw_fsm.c     | 21 +++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index fe87a2652a22..52249c40a565 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -231,11 +231,8 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	dev_set_drvdata(&sch->dev, private);
 
-	spin_lock_irq(sch->lock);
-	sch->isc = VFIO_CCW_ISC;
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	spin_unlock_irq(sch->lock);
-	if (ret)
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
+	if (private->state == VFIO_CCW_STATE_NOT_OPER)
 		goto out_free;
 
 	private->state = VFIO_CCW_STATE_STANDBY;
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index bbcc5b486749..7e7ed69e1461 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -11,6 +11,8 @@
 
 #include <linux/vfio.h>
 
+#include <asm/isc.h>
+
 #include "ioasm.h"
 #include "vfio_ccw_private.h"
 
@@ -364,6 +366,20 @@ static void fsm_irq(struct vfio_ccw_private *private,
 		complete(private->completion);
 }
 
+static void fsm_open(struct vfio_ccw_private *private,
+		     enum vfio_ccw_event event)
+{
+	struct subchannel *sch = private->sch;
+	int ret;
+
+	spin_lock_irq(sch->lock);
+	sch->isc = VFIO_CCW_ISC;
+	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
+	if (!ret)
+		private->state = VFIO_CCW_STATE_STANDBY;
+	spin_unlock_irq(sch->lock);
+}
+
 /*
  * Device statemachine
  */
@@ -373,29 +389,34 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_CP_PROCESSING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_retry,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_retry,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_CP_PENDING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
 	},
 };
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 4cfdd5fc0961..8dff1699a7d9 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -142,6 +142,7 @@ enum vfio_ccw_event {
 	VFIO_CCW_EVENT_IO_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
 	VFIO_CCW_EVENT_ASYNC_REQ,
+	VFIO_CCW_EVENT_OPEN,
 	/* last element! */
 	NR_VFIO_CCW_EVENTS
 };
-- 
2.32.0

[PATCH v2 08/10] vfio/ccw: Create a CLOSE FSM event

[Eric Farman]

Refactor the vfio_ccw_sch_quiesce() routine to extract the bit that
disables the subchannel and affects the FSM state. Use this to form
the basis of a CLOSE event that will mirror the OPEN event, and move
the subchannel back to NOT_OPER state.

A key difference with that mirroring is that while OPEN handles the
transition from NOT_OPER => STANDBY, the later probing of the mdev
handles the transition from STANDBY => IDLE. On the other hand,
the CLOSE event will move from one of the operating states {IDLE,
CP_PROCESSING, CP_PENDING} => NOT_OPER. That is, there is no stop
in a STANDBY state on the deconfigure path.

Add a call to cp_free() in this event, such that it is captured for
the various permutations of this event.

In the unlikely event that cio_disable_subchannel() returns -EBUSY,
the remaining logic of vfio_ccw_sch_quiesce() can still be used.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 17 +++++------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 26 ++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     | 14 ++------------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 4 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 52249c40a565..62bd6f969b76 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -41,13 +41,6 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 	DECLARE_COMPLETION_ONSTACK(completion);
 	int iretry, ret = 0;
 
-	spin_lock_irq(sch->lock);
-	if (!sch->schib.pmcw.ena)
-		goto out_unlock;
-	ret = cio_disable_subchannel(sch);
-	if (ret != -EBUSY)
-		goto out_unlock;
-
 	iretry = 255;
 	do {
 
@@ -74,9 +67,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 		spin_lock_irq(sch->lock);
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
-out_unlock:
-	private->state = VFIO_CCW_STATE_NOT_OPER;
-	spin_unlock_irq(sch->lock);
+
 	return ret;
 }
 
@@ -258,7 +249,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
-	vfio_ccw_sch_quiesce(sch);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	mdev_unregister_device(&sch->dev);
 
 	dev_set_drvdata(&sch->dev, NULL);
@@ -272,7 +263,9 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 
 static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 {
-	vfio_ccw_sch_quiesce(sch);
+	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 }
 
 /**
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 7e7ed69e1461..fa546d33e595 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -380,6 +380,27 @@ static void fsm_open(struct vfio_ccw_private *private,
 	spin_unlock_irq(sch->lock);
 }
 
+static void fsm_close(struct vfio_ccw_private *private,
+		      enum vfio_ccw_event event)
+{
+	struct subchannel *sch = private->sch;
+	int ret;
+
+	spin_lock_irq(sch->lock);
+
+	if (!sch->schib.pmcw.ena)
+		goto out_unlock;
+
+	ret = cio_disable_subchannel(sch);
+	if (ret == -EBUSY)
+		vfio_ccw_sch_quiesce(sch);
+
+out_unlock:
+	private->state = VFIO_CCW_STATE_NOT_OPER;
+	spin_unlock_irq(sch->lock);
+	cp_free(&private->cp);
+}
+
 /*
  * Device statemachine
  */
@@ -390,6 +411,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -397,6 +419,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -404,6 +427,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
 	},
 	[VFIO_CCW_STATE_CP_PROCESSING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -411,6 +435,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_retry,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
 	},
 	[VFIO_CCW_STATE_CP_PENDING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -418,5 +443,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
 	},
 };
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index a7ea9358e461..fc5b83187bd9 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -33,9 +33,7 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 	 * There are still a lot more instructions need to be handled. We
 	 * should come back here later.
 	 */
-	ret = vfio_ccw_sch_quiesce(sch);
-	if (ret)
-		return ret;
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 
 	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
 	if (!ret)
@@ -64,7 +62,6 @@ static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
 		if (vfio_ccw_mdev_reset(private))
 			return NOTIFY_BAD;
 
-		cp_free(&private->cp);
 		return NOTIFY_OK;
 	}
 
@@ -159,15 +156,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	vfio_unregister_group_dev(&private->vdev);
 
-	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
-	    (private->state != VFIO_CCW_STATE_STANDBY)) {
-		if (!vfio_ccw_sch_quiesce(private->sch))
-			private->state = VFIO_CCW_STATE_STANDBY;
-		/* The state will be NOT_OPER on error. */
-	}
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 
 	vfio_uninit_group_dev(&private->vdev);
-	cp_free(&private->cp);
 	atomic_inc(&private->avail);
 }
 
@@ -217,7 +208,6 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 		/* The state will be NOT_OPER on error. */
 	}
 
-	cp_free(&private->cp);
 	vfio_ccw_unregister_dev_regions(private);
 	vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY, &private->nb);
 }
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 8dff1699a7d9..435d401b8fb9 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -143,6 +143,7 @@ enum vfio_ccw_event {
 	VFIO_CCW_EVENT_INTERRUPT,
 	VFIO_CCW_EVENT_ASYNC_REQ,
 	VFIO_CCW_EVENT_OPEN,
+	VFIO_CCW_EVENT_CLOSE,
 	/* last element! */
 	NR_VFIO_CCW_EVENTS
 };
-- 
2.32.0

[PATCH v2 09/10] vfio/ccw: Refactor vfio_ccw_mdev_reset

[Eric Farman]

Use both the FSM Close and Open events when resetting an mdev,
rather than making a separate call to cio_enable_subchannel().

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index fc5b83187bd9..4673b7ddfe20 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -21,25 +21,21 @@ static const struct vfio_device_ops vfio_ccw_dev_ops;
 
 static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 {
-	struct subchannel *sch;
-	int ret;
-
-	sch = private->sch;
 	/*
-	 * TODO:
-	 * In the cureent stage, some things like "no I/O running" and "no
-	 * interrupt pending" are clear, but we are not sure what other state
-	 * we need to care about.
-	 * There are still a lot more instructions need to be handled. We
-	 * should come back here later.
+	 * If the FSM state is seen as Not Operational after closing
+	 * and re-opening the mdev, return an error.
+	 *
+	 * Otherwise, change the FSM from STANDBY to IDLE which is
+	 * normally done by vfio_ccw_mdev_probe() in current lifecycle.
 	 */
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
+	if (private->state == VFIO_CCW_STATE_NOT_OPER)
+		return -EINVAL;
 
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	if (!ret)
-		private->state = VFIO_CCW_STATE_IDLE;
+	private->state = VFIO_CCW_STATE_IDLE;
 
-	return ret;
+	return 0;
 }
 
 static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
-- 
2.32.0

[PATCH v2 10/10] vfio/ccw: Move FSM open/close to MDEV open/close

[Eric Farman]

Part of the confusion that has existed is the FSM lifecycle of
subchannels between the common CSS driver and the vfio-ccw driver.
During configuration, the FSM state goes from NOT_OPER to STANDBY
to IDLE, but then back to NOT_OPER. For example:

	vfio_ccw_sch_probe:		VFIO_CCW_STATE_NOT_OPER
	vfio_ccw_sch_probe:		VFIO_CCW_STATE_STANDBY
	vfio_ccw_mdev_probe:		VFIO_CCW_STATE_IDLE
	vfio_ccw_mdev_remove:		VFIO_CCW_STATE_NOT_OPER
	vfio_ccw_sch_remove:		VFIO_CCW_STATE_NOT_OPER
	vfio_ccw_sch_shutdown:		VFIO_CCW_STATE_NOT_OPER

Rearrange the open/close events to align with the mdev open/close,
to better manage the memory and state of the devices as time
progresses. Specifically, make mdev_open() perform the FSM open,
and mdev_close() perform the FSM close instead of reset (which is
both close and open).

This makes the NOT_OPER state a dead-end path, indicating the
device is probably not recoverable without fully probing and
re-configuring the device.

This has the nice side-effect of removing a number of special-cases
where the FSM state is managed outside of the FSM itself (such as
the aforementioned mdev_close() routine).

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 13 +++----------
 drivers/s390/cio/vfio_ccw_fsm.c | 30 ++++++++++++++++++++++--------
 drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++---------------
 3 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 62bd6f969b76..4804101ccb0f 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -138,7 +138,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 
 	private->sch = sch;
 	mutex_init(&private->io_mutex);
-	private->state = VFIO_CCW_STATE_NOT_OPER;
+	private->state = VFIO_CCW_STATE_STANDBY;
 	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
@@ -222,23 +222,15 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	dev_set_drvdata(&sch->dev, private);
 
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
-	if (private->state == VFIO_CCW_STATE_NOT_OPER)
-		goto out_free;
-
-	private->state = VFIO_CCW_STATE_STANDBY;
-
 	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver);
 	if (ret)
-		goto out_disable;
+		goto out_free;
 
 	VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
 	return 0;
 
-out_disable:
-	cio_disable_subchannel(sch);
 out_free:
 	dev_set_drvdata(&sch->dev, NULL);
 	vfio_ccw_free_private(private);
@@ -266,6 +258,7 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 }
 
 /**
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index fa546d33e595..acc80d882640 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -171,6 +171,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;
+	cp_free(&private->cp);
 }
 
 /*
@@ -375,9 +376,16 @@ static void fsm_open(struct vfio_ccw_private *private,
 	spin_lock_irq(sch->lock);
 	sch->isc = VFIO_CCW_ISC;
 	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	if (!ret)
-		private->state = VFIO_CCW_STATE_STANDBY;
+	if (ret)
+		goto err_unlock;
+
+	private->state = VFIO_CCW_STATE_IDLE;
 	spin_unlock_irq(sch->lock);
+	return;
+
+err_unlock:
+	spin_unlock_irq(sch->lock);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 }
 
 static void fsm_close(struct vfio_ccw_private *private,
@@ -389,16 +397,22 @@ static void fsm_close(struct vfio_ccw_private *private,
 	spin_lock_irq(sch->lock);
 
 	if (!sch->schib.pmcw.ena)
-		goto out_unlock;
+		goto err_unlock;
 
 	ret = cio_disable_subchannel(sch);
 	if (ret == -EBUSY)
 		vfio_ccw_sch_quiesce(sch);
+	if (ret)
+		goto err_unlock;
 
-out_unlock:
-	private->state = VFIO_CCW_STATE_NOT_OPER;
+	private->state = VFIO_CCW_STATE_STANDBY;
 	spin_unlock_irq(sch->lock);
 	cp_free(&private->cp);
+	return;
+
+err_unlock:
+	spin_unlock_irq(sch->lock);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 }
 
 /*
@@ -410,15 +424,15 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
-		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
 		[VFIO_CCW_EVENT_CLOSE]		= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
-		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
-		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
 		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 4673b7ddfe20..bc2176421dc5 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -24,17 +24,12 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 	/*
 	 * If the FSM state is seen as Not Operational after closing
 	 * and re-opening the mdev, return an error.
-	 *
-	 * Otherwise, change the FSM from STANDBY to IDLE which is
-	 * normally done by vfio_ccw_mdev_probe() in current lifecycle.
 	 */
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
 	if (private->state == VFIO_CCW_STATE_NOT_OPER)
 		return -EINVAL;
 
-	private->state = VFIO_CCW_STATE_IDLE;
-
 	return 0;
 }
 
@@ -121,8 +116,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_init_group_dev(&private->vdev, &mdev->dev,
 			    &vfio_ccw_dev_ops);
 
-	private->state = VFIO_CCW_STATE_IDLE;
-
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
 			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
@@ -137,7 +130,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 err_atomic:
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
-	private->state = VFIO_CCW_STATE_STANDBY;
 	return ret;
 }
 
@@ -165,6 +157,10 @@ static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
 	unsigned long events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
 	int ret;
 
+	/* Device cannot simply be opened again from this state */
+	if (private->state == VFIO_CCW_STATE_NOT_OPER)
+		return -EINVAL;
+
 	private->nb.notifier_call = vfio_ccw_mdev_notifier;
 
 	ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY,
@@ -184,6 +180,12 @@ static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
 	if (ret)
 		goto out_unregister;
 
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
+	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
+		ret = -EINVAL;
+		goto out_unregister;
+	}
+
 	return ret;
 
 out_unregister:
@@ -197,13 +199,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
-	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
-	    (private->state != VFIO_CCW_STATE_STANDBY)) {
-		if (!vfio_ccw_mdev_reset(private))
-			private->state = VFIO_CCW_STATE_STANDBY;
-		/* The state will be NOT_OPER on error. */
-	}
-
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	vfio_ccw_unregister_dev_regions(private);
 	vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY, &private->nb);
 }
-- 
2.32.0

Re: [PATCH v2 01/10] vfio/ccw: Remove UUID from s390 debug log

[Kirti Wankhede]

On 6/16/2022 2:03 AM, Eric Farman wrote:
> From: Michael Kawano <mkawano@linux.ibm.com>
> 
> As vfio-ccw devices are created/destroyed, the uuid of the associated
> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
> This is because a pointer to the UUID is stored instead of the UUID
> itself, and that memory may have been repurposed if/when the logs are
> examined. The result is usually garbage UUID data in the logs, though
> there is an outside chance of an oops happening here.
> 
> Simply remove the UUID from the traces, as the subchannel number will
> provide useful configuration information for problem determination,
> and is stored directly into the log instead of a pointer.
> 
> As we were the only consumer of mdev_uuid(), remove that too.
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
> Fixes: 60e05d1cf0875 ("vfio-ccw: add some logging")
> Fixes: b7701dfbf9832 ("vfio-ccw: Register a chp_event callback for vfio-ccw")
> [farman: reworded commit message, added Fixes: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---

Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>

Re: [PATCH v2 03/10] vfio/ccw: Do not change FSM state in subchannel event

[Matthew Rosato]

On 6/15/22 4:33 PM, Eric Farman wrote:
> The routine vfio_ccw_sch_event() is tasked with handling subchannel events,
> specifically machine checks, on behalf of vfio-ccw. It correctly calls
> cio_update_schib(), and if that fails (meaning the subchannel is gone)
> it makes an FSM event call to mark the subchannel Not Operational.
> 
> If that worked, however, then it decides that if the FSM state was already
> Not Operational (implying the subchannel just came back), then it should
> simply change the FSM to partially- or fully-open.
> 
> Remove this trickery, since a subchannel returning will require more
> probing than simply "oh all is well again" to ensure it works correctly.
> 
> Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state machine")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

So, I agree that this code does not belong here and should be removed 
-- if the subchannel just came back, we can't assume it's even the same 
device.  We'd better just leave it NOT_OPER for this weird window for 
now.  So...

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

But I also wonder if more work could be done from vfio_ccw_sch_event 
based upon whats in the schib, like what io_subchannel_sch_event does 
(e.g. detecting if a reprobe of the device is necessary).  We should 
probably take a closer look here for potential follow-ups.

> ---
>   drivers/s390/cio/vfio_ccw_drv.c | 14 +++-----------
>   1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 179eb614fa5b..279ad2161f17 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -301,19 +301,11 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>   	if (work_pending(&sch->todo_work))
>   		goto out_unlock;
>   
> -	if (cio_update_schib(sch)) {
> -		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> -		rc = 0;
> -		goto out_unlock;
> -	}
> -
> -	private = dev_get_drvdata(&sch->dev);
> -	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
> -		private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
> -				 VFIO_CCW_STATE_STANDBY;
> -	}
>   	rc = 0;
>   
> +	if (cio_update_schib(sch))
> +		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> +
>   out_unlock:
>   	spin_unlock_irqrestore(sch->lock, flags);
>   

Re: [PATCH v2 04/10] vfio/ccw: Remove private->mdev

[Matthew Rosato]

On 6/15/22 4:33 PM, Eric Farman wrote:
> There are no remaining users of private->mdev. Remove it.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v2 07/10] vfio/ccw: Create an OPEN FSM Event

[Matthew Rosato]

On 6/15/22 4:33 PM, Eric Farman wrote:
> Move the process of enabling a subchannel for use by vfio-ccw
> into the FSM, such that it can manage the sequence of lifecycle
> events for the device.
> 
> That is, if the FSM state is NOT_OPER(erational), then do the work
> that would enable the subchannel and move the FSM to STANDBY state.
> An attempt to perform this event again from any of the other operating
> states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device back
> to NOT_OPER so the configuration process can be started again.

Except STANDBY, which ignores the event via fsm_nop.  I wonder though, 
whether that's the right thing to do.  For each of the other states 
you're saying 'if it's already open, go back to NOT_OPER so we can start 
over' -- In this case a STANDBY->STANDBY is also a case of 'it's already 
open' so shouldn't we also go back to NOT_OPER so we can start over? 
Seems to me really we just don't expect to ever get an OPEN event unless 
we are in NOT_OPER.

If there's a reason to keep STANDBY->STANDBY as a nop, but we don't 
expect to see it and don't' want to WARN because of it, then maybe a log 
entry at least would make sense.

As for the IDLE/CP_PROCESSING/CP_PENDING cases, going fsm_notoper 
because this is unexpected probably makes sense, but the logging is 
going to be really confusing (before this change, you know that you 
called fsm_notoper because you got VFIO_CCW_EVENT_NOT_OPER -- now you'll 
see a log entry cut for NOT_OPER but won't be sure if it was for 
EVENT_NOT_OPER or EVENT_OPEN).  Maybe you can look at 'event' inside 
fsm_notoper and cut a slightly different trace entry when arriving here 
for EVENT_OPEN?

...

> +static void fsm_open(struct vfio_ccw_private *private,
> +		     enum vfio_ccw_event event)
> +{
> +	struct subchannel *sch = private->sch;
> +	int ret;
> +
> +	spin_lock_irq(sch->lock);
> +	sch->isc = VFIO_CCW_ISC;
> +	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> +	if (!ret)
> +		private->state = VFIO_CCW_STATE_STANDBY;

nit: could get rid of 'ret' and just do

if (!cio_enable...)
      private->state = VFIO_CCW_STATE_STANDBY;

> +	spin_unlock_irq(sch->lock);
> +}
> +
>   /*
>    * Device statemachine
>    */
> @@ -373,29 +389,34 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
> +		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
>   	},
>   	[VFIO_CCW_STATE_STANDBY] = {
>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> +		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
>   	},
>   	[VFIO_CCW_STATE_IDLE] = {
>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> +		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
>   	},
>   	[VFIO_CCW_STATE_CP_PROCESSING] = {
>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_retry,
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_retry,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> +		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
>   	},
>   	[VFIO_CCW_STATE_CP_PENDING] = {
>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> +		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
>   	},
>   };
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 4cfdd5fc0961..8dff1699a7d9 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -142,6 +142,7 @@ enum vfio_ccw_event {
>   	VFIO_CCW_EVENT_IO_REQ,
>   	VFIO_CCW_EVENT_INTERRUPT,
>   	VFIO_CCW_EVENT_ASYNC_REQ,
> +	VFIO_CCW_EVENT_OPEN,
>   	/* last element! */
>   	NR_VFIO_CCW_EVENTS
>   };

Re: [PATCH v2 08/10] vfio/ccw: Create a CLOSE FSM event

[Matthew Rosato]

On 6/15/22 4:33 PM, Eric Farman wrote:
> Refactor the vfio_ccw_sch_quiesce() routine to extract the bit that
> disables the subchannel and affects the FSM state. Use this to form
> the basis of a CLOSE event that will mirror the OPEN event, and move
> the subchannel back to NOT_OPER state.

Similar comments here related to previous patch.  If a close event can 
trigger fsm_notoper then it should probably should cut a different trace 
entry when event == CLOSE

> 
> A key difference with that mirroring is that while OPEN handles the
> transition from NOT_OPER => STANDBY, the later probing of the mdev
> handles the transition from STANDBY => IDLE. On the other hand,
> the CLOSE event will move from one of the operating states {IDLE,
> CP_PROCESSING, CP_PENDING} => NOT_OPER. That is, there is no stop
> in a STANDBY state on the deconfigure path.
> 
> Add a call to cp_free() in this event, such that it is captured for
> the various permutations of this event.
> 
> In the unlikely event that cio_disable_subchannel() returns -EBUSY,
> the remaining logic of vfio_ccw_sch_quiesce() can still be used.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_drv.c     | 17 +++++------------
>   drivers/s390/cio/vfio_ccw_fsm.c     | 26 ++++++++++++++++++++++++++
>   drivers/s390/cio/vfio_ccw_ops.c     | 14 ++------------
>   drivers/s390/cio/vfio_ccw_private.h |  1 +
>   4 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 52249c40a565..62bd6f969b76 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -41,13 +41,6 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>   	DECLARE_COMPLETION_ONSTACK(completion);
>   	int iretry, ret = 0;
>   
> -	spin_lock_irq(sch->lock);
> -	if (!sch->schib.pmcw.ena)
> -		goto out_unlock;
> -	ret = cio_disable_subchannel(sch);
> -	if (ret != -EBUSY)
> -		goto out_unlock;
> -
>   	iretry = 255;
>   	do {
>   
> @@ -74,9 +67,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>   		spin_lock_irq(sch->lock);
>   		ret = cio_disable_subchannel(sch);
>   	} while (ret == -EBUSY);
> -out_unlock:
> -	private->state = VFIO_CCW_STATE_NOT_OPER;
> -	spin_unlock_irq(sch->lock);
> +
>   	return ret;
>   }
>   
> @@ -258,7 +249,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
>   {
>   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>   
> -	vfio_ccw_sch_quiesce(sch);
> +	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>   	mdev_unregister_device(&sch->dev);
>   
>   	dev_set_drvdata(&sch->dev, NULL);
> @@ -272,7 +263,9 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
>   
>   static void vfio_ccw_sch_shutdown(struct subchannel *sch)
>   {
> -	vfio_ccw_sch_quiesce(sch);
> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +
> +	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>   }
>   
>   /**
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 7e7ed69e1461..fa546d33e595 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -380,6 +380,27 @@ static void fsm_open(struct vfio_ccw_private *private,
>   	spin_unlock_irq(sch->lock);
>   }
>   
> +static void fsm_close(struct vfio_ccw_private *private,
> +		      enum vfio_ccw_event event)
> +{
> +	struct subchannel *sch = private->sch;
> +	int ret;
> +
> +	spin_lock_irq(sch->lock);
> +
> +	if (!sch->schib.pmcw.ena)
> +		goto out_unlock;
> +
> +	ret = cio_disable_subchannel(sch);
> +	if (ret == -EBUSY)
> +		vfio_ccw_sch_quiesce(sch);
> +
> +out_unlock:
> +	private->state = VFIO_CCW_STATE_NOT_OPER;
> +	spin_unlock_irq(sch->lock);
> +	cp_free(&private->cp);
> +}
> +
>   /*
>    * Device statemachine
>    */
> @@ -390,6 +411,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
>   		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
> +		[VFIO_CCW_EVENT_CLOSE]		= fsm_nop,
>   	},
>   	[VFIO_CCW_STATE_STANDBY] = {
>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> @@ -397,6 +419,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
>   		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
> +		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,

But if we are in STANDBY doesn't that imply we already did the OPEN? 
Don't we need to close it now before going NOT_OPER?

Re: [PATCH v2 03/10] vfio/ccw: Do not change FSM state in subchannel event

[Eric Farman]

On Thu, 2022-06-16 at 11:35 -0400, Matthew Rosato wrote:
> On 6/15/22 4:33 PM, Eric Farman wrote:
> > The routine vfio_ccw_sch_event() is tasked with handling subchannel
> > events,
> > specifically machine checks, on behalf of vfio-ccw. It correctly
> > calls
> > cio_update_schib(), and if that fails (meaning the subchannel is
> > gone)
> > it makes an FSM event call to mark the subchannel Not Operational.
> > 
> > If that worked, however, then it decides that if the FSM state was
> > already
> > Not Operational (implying the subchannel just came back), then it
> > should
> > simply change the FSM to partially- or fully-open.
> > 
> > Remove this trickery, since a subchannel returning will require
> > more
> > probing than simply "oh all is well again" to ensure it works
> > correctly.
> > 
> > Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state
> > machine")
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> So, I agree that this code does not belong here and should be
> removed 
> -- if the subchannel just came back, we can't assume it's even the
> same 
> device.  We'd better just leave it NOT_OPER for this weird window
> for 
> now.  So...
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> But I also wonder if more work could be done from vfio_ccw_sch_event 
> based upon whats in the schib, like what io_subchannel_sch_event
> does 
> (e.g. detecting if a reprobe of the device is necessary).  We should 
> probably take a closer look here for potential follow-ups.

Agreed. Will add that to the backlog.

> 
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c | 14 +++-----------
> >   1 file changed, 3 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 179eb614fa5b..279ad2161f17 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -301,19 +301,11 @@ static int vfio_ccw_sch_event(struct
> > subchannel *sch, int process)
> >   	if (work_pending(&sch->todo_work))
> >   		goto out_unlock;
> >   
> > -	if (cio_update_schib(sch)) {
> > -		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> > -		rc = 0;
> > -		goto out_unlock;
> > -	}
> > -
> > -	private = dev_get_drvdata(&sch->dev);
> > -	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
> > -		private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
> > -				 VFIO_CCW_STATE_STANDBY;
> > -	}
> >   	rc = 0;
> >   
> > +	if (cio_update_schib(sch))
> > +		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> > +
> >   out_unlock:
> >   	spin_unlock_irqrestore(sch->lock, flags);
> >   

Re: [PATCH v2 07/10] vfio/ccw: Create an OPEN FSM Event

[Eric Farman]

On Thu, 2022-06-16 at 12:33 -0400, Matthew Rosato wrote:
> On 6/15/22 4:33 PM, Eric Farman wrote:
> > Move the process of enabling a subchannel for use by vfio-ccw
> > into the FSM, such that it can manage the sequence of lifecycle
> > events for the device.
> > 
> > That is, if the FSM state is NOT_OPER(erational), then do the work
> > that would enable the subchannel and move the FSM to STANDBY state.
> > An attempt to perform this event again from any of the other
> > operating
> > states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device
> > back
> > to NOT_OPER so the configuration process can be started again.
> 
> Except STANDBY, which ignores the event via fsm_nop.  I wonder
> though, 
> whether that's the right thing to do.  For each of the other states 
> you're saying 'if it's already open, go back to NOT_OPER so we can
> start 
> over' -- In this case a STANDBY->STANDBY is also a case of 'it's
> already 
> open' so shouldn't we also go back to NOT_OPER so we can start over?

Yeah, the subchannel's already been probed but the mdev hasn't yet. (Or
perhaps it did, but that failed.) I was viewing it as a "well there's
nothing to do here" but you're right that the rest of the entries take
a "oh that's unexpected, go to NOT_OPER" approach. So should make that
consistent here, since it would be quite a surprise.

>  
> Seems to me really we just don't expect to ever get an OPEN event
> unless 
> we are in NOT_OPER.
> 
> If there's a reason to keep STANDBY->STANDBY as a nop, but we don't 
> expect to see it and don't' want to WARN because of it, then maybe a
> log 
> entry at least would make sense.
> 
> As for the IDLE/CP_PROCESSING/CP_PENDING cases, going fsm_notoper 
> because this is unexpected probably makes sense, but the logging is 
> going to be really confusing (before this change, you know that you 
> called fsm_notoper because you got VFIO_CCW_EVENT_NOT_OPER -- now
> you'll 
> see a log entry cut for NOT_OPER but won't be sure if it was for 
> EVENT_NOT_OPER or EVENT_OPEN).  Maybe you can look at 'event' inside 
> fsm_notoper and cut a slightly different trace entry when arriving
> here 
> for EVENT_OPEN?

Yeah, good idea. Since we don't expect any of these in normal behavior,
perhaps I'll trace both state and event, instead of trying to make
conditionals out of everything.

> 
> ...
> 
> > +static void fsm_open(struct vfio_ccw_private *private,
> > +		     enum vfio_ccw_event event)
> > +{
> > +	struct subchannel *sch = private->sch;
> > +	int ret;
> > +
> > +	spin_lock_irq(sch->lock);
> > +	sch->isc = VFIO_CCW_ISC;
> > +	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> > +	if (!ret)
> > +		private->state = VFIO_CCW_STATE_STANDBY;
> 
> nit: could get rid of 'ret' and just do
> 
> if (!cio_enable...)
>       private->state = VFIO_CCW_STATE_STANDBY;

Ah, fair. Cut/paste and didn't really consider the simplification.

I see that I left the unconditional "private->state = STANDBY" in the
hunk just above this, which can be removed. (I finally do in patch 10.)
Will make that change too.

> 
> > +	spin_unlock_irq(sch->lock);
> > +}
> > +
> >   /*
> >    * Device statemachine
> >    */
> > @@ -373,29 +389,34 @@ fsm_func_t
> > *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> >   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
> >   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
> >   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
> > +		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
> >   	},
> >   	[VFIO_CCW_STATE_STANDBY] = {
> >   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
> >   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
> >   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
> >   	},
> >   	[VFIO_CCW_STATE_IDLE] = {
> >   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
> >   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
> >   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
> >   	},
> >   	[VFIO_CCW_STATE_CP_PROCESSING] = {
> >   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_retry,
> >   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_retry,
> >   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
> >   	},
> >   	[VFIO_CCW_STATE_CP_PENDING] = {
> >   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >   		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
> >   		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
> >   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
> >   	},
> >   };
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h
> > b/drivers/s390/cio/vfio_ccw_private.h
> > index 4cfdd5fc0961..8dff1699a7d9 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -142,6 +142,7 @@ enum vfio_ccw_event {
> >   	VFIO_CCW_EVENT_IO_REQ,
> >   	VFIO_CCW_EVENT_INTERRUPT,
> >   	VFIO_CCW_EVENT_ASYNC_REQ,
> > +	VFIO_CCW_EVENT_OPEN,
> >   	/* last element! */
> >   	NR_VFIO_CCW_EVENTS
> >   };

Re: [PATCH v2 07/10] vfio/ccw: Create an OPEN FSM Event

[Matthew Rosato]

>> As for the IDLE/CP_PROCESSING/CP_PENDING cases, going fsm_notoper
>> because this is unexpected probably makes sense, but the logging is
>> going to be really confusing (before this change, you know that you
>> called fsm_notoper because you got VFIO_CCW_EVENT_NOT_OPER -- now
>> you'll
>> see a log entry cut for NOT_OPER but won't be sure if it was for
>> EVENT_NOT_OPER or EVENT_OPEN).  Maybe you can look at 'event' inside
>> fsm_notoper and cut a slightly different trace entry when arriving
>> here
>> for EVENT_OPEN?
> 
> Yeah, good idea. Since we don't expect any of these in normal behavior,
> perhaps I'll trace both state and event, instead of trying to make
> conditionals out of everything.
> 

Sounds good to me

Re: [PATCH v2 04/10] vfio/ccw: Remove private->mdev

[Jason Gunthorpe]

On Wed, Jun 15, 2022 at 10:33:12PM +0200, Eric Farman wrote:
> There are no remaining users of private->mdev. Remove it.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_async.c   | 1 -
>  drivers/s390/cio/vfio_ccw_ops.c     | 3 ---
>  drivers/s390/cio/vfio_ccw_private.h | 2 --
>  3 files changed, 6 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason