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

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

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, 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?

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;