Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

From: Pierre Morel
Date: Tue Jun 05 2018 - 10:21:17 EST

On 05/06/2018 15:35, Cornelia Huck wrote:
On Tue, 5 Jun 2018 15:10:11 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

On 05/06/2018 13:38, Cornelia Huck wrote:
On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:
We use mutex around the FSM function call to make the FSM
event handling and state change atomic.
I'm still not really clear as to what this mutex is supposed to

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
the state machine.)
yes to all
But wouldn't that imply that you need to either take the mutex if you
do something dependent on the state, or fire an event in that case?

Yes, if it is not I forgot something important (like I did in patch 10)
vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.

I have some cases where I do not respect this.
This is false and I must handle this with a new private variable,
this is where I test the state after having fired an event.
I will need to change this, in quiesce, reset, probe and open (others?).

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx>
drivers/s390/cio/vfio_ccw_drv.c | 3 +--
drivers/s390/cio/vfio_ccw_private.h | 3 +++
2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
private = container_of(work, struct vfio_ccw_private, io_work);
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
- if (private->mdev)
- private->state = VFIO_CCW_STATE_IDLE;
Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?
It is moved to fsm_irq() which is called under mutex.
fsm_irq() returns VFIO_CCW_STATE_IDLE.
So, should that go into another patch?

I will see if I can put it inside the patch 01/10 moving state change out of IRQ context.

Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany