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

Matthew Rosato
2022-06-06T20:59:40Z

On 6/3/22 3:12 PM, Eric Farman wrote:
> 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.
>
> */
>

Sounds good to me