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

[Eric Farman <farman@linux.ibm.com>]

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