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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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
>   };