Re: [PATCH v1 02/18] vfio/ccw: Fix FSM state if mdev probe fails

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

On Fri, 2022-06-03 at 09:21 -0400, Matthew Rosato wrote:
> On 6/2/22 1:19 PM, Eric Farman wrote:
> > The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
> > and this routine converts it to IDLE as part of its processing.
> > The error exit sets it to IDLE (again) but clears the private->mdev
> > pointer.
> > 
> > The FSM should of course be managing the state itself, but the
> > correct thing for vfio_ccw_mdev_probe() to do would be to put
> > the state back the way it found it.
> > 
> > The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
> > can be removed, since the distinction is unnecessary at this point.
> > 
> > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use
> > vfio_register_emulated_iommu_dev()")
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
> >   drivers/s390/cio/vfio_ccw_ops.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 35055eb94115..b18b4582bc8b 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct
> > work_struct *work)
> >   	 * has finished. Do not overwrite a possible processing
> >   	 * state if the final interrupt was for HSCH or CSCH.
> >   	 */
> > -	if (private->mdev && cp_is_finished)
> > +	if (cp_is_finished)
> >   		private->state = VFIO_CCW_STATE_IDLE;
> 
> Took me a bit to convince myself this was OK

Me too. :)

> , mainly because AFAICT 
> despite the change below the fsm jumptable would still allow you to 
> reach this code when in STANDBY.  But, it should only be possible for
> an 
> unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so
> we 
> would still avoid a STANDBY->IDLE transition on accident.
> 
> Maybe work unsolicited interrupt into the comment block above along
> with 
> HSCH/CSCH?

Good idea. How about:

        /*
         * Reset to IDLE only if
processing of a channel program
         * has finished. Do not
overwrite a possible processing
         * state if the interrupt was
unsolicited, or if the final
         * interrupt was for HSCH or CSCH.
 
        */

> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> >   
> >   	if (private->io_trigger)
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index bebae21228aa..a403d059a4e6 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device *mdev)
> >   	vfio_uninit_group_dev(&private->vdev);
> >   	atomic_inc(&private->avail);
> >   	private->mdev = NULL;
> > -	private->state = VFIO_CCW_STATE_IDLE;
> > +	private->state = VFIO_CCW_STATE_STANDBY;