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