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

[Matthew Rosato <mjrosato@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> ---
>   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?