| Message ID | 20220615203318.3830778-4-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ccw rework | expand |
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); >
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); > >
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);
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> --- drivers/s390/cio/vfio_ccw_drv.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)