diff mbox series

[v3,11/11] vfio/ccw: Move FSM open/close to MDEV open/close

Message ID 20220630203647.2529815-12-farman@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390/vfio-ccw rework | expand

Commit Message

Eric Farman June 30, 2022, 8:36 p.m. UTC 
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 | 11 +++--------
 drivers/s390/cio/vfio_ccw_fsm.c | 32 +++++++++++++++++++++++---------
 drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++---------------
 3 files changed, 37 insertions(+), 32 deletions(-)

Comments

Matthew Rosato July 5, 2022, 8:17 p.m. UTC | #1 
On 6/30/22 4:36 PM, Eric Farman wrote:
> 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>

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

> ---
>   drivers/s390/cio/vfio_ccw_drv.c | 11 +++--------
>   drivers/s390/cio/vfio_ccw_fsm.c | 32 +++++++++++++++++++++++---------
>   drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++---------------
>   3 files changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index f98c9915e73d..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,21 +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;
> -
>   	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);
> @@ -264,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 89eb3feffa41..472e77f1bb6e 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -175,6 +175,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);
>   }
>   
>   /*
> @@ -379,9 +380,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,
> @@ -393,16 +401,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);
>   }
>   
>   /*
> @@ -414,16 +428,16 @@ 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_notoper,
> -		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
> +		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
> +		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
> +		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,
>   	},
>   	[VFIO_CCW_STATE_IDLE] = {
>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> 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);
>   }
diff mbox series

Patch

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index f98c9915e73d..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,21 +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;
-
 	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);
@@ -264,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 89eb3feffa41..472e77f1bb6e 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -175,6 +175,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);
 }
 
 /*
@@ -379,9 +380,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,
@@ -393,16 +401,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);
 }
 
 /*
@@ -414,16 +428,16 @@  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_notoper,
-		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
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);
 }