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

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

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