| Message ID | 20220615203318.3830778-8-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: > Move the process of enabling a subchannel for use by vfio-ccw > into the FSM, such that it can manage the sequence of lifecycle > events for the device. > > That is, if the FSM state is NOT_OPER(erational), then do the work > that would enable the subchannel and move the FSM to STANDBY state. > An attempt to perform this event again from any of the other operating > states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device back > to NOT_OPER so the configuration process can be started again. Except STANDBY, which ignores the event via fsm_nop. I wonder though, whether that's the right thing to do. For each of the other states you're saying 'if it's already open, go back to NOT_OPER so we can start over' -- In this case a STANDBY->STANDBY is also a case of 'it's already open' so shouldn't we also go back to NOT_OPER so we can start over? Seems to me really we just don't expect to ever get an OPEN event unless we are in NOT_OPER. If there's a reason to keep STANDBY->STANDBY as a nop, but we don't expect to see it and don't' want to WARN because of it, then maybe a log entry at least would make sense. As for the IDLE/CP_PROCESSING/CP_PENDING cases, going fsm_notoper because this is unexpected probably makes sense, but the logging is going to be really confusing (before this change, you know that you called fsm_notoper because you got VFIO_CCW_EVENT_NOT_OPER -- now you'll see a log entry cut for NOT_OPER but won't be sure if it was for EVENT_NOT_OPER or EVENT_OPEN). Maybe you can look at 'event' inside fsm_notoper and cut a slightly different trace entry when arriving here for EVENT_OPEN? ... > +static void fsm_open(struct vfio_ccw_private *private, > + enum vfio_ccw_event event) > +{ > + struct subchannel *sch = private->sch; > + int ret; > + > + spin_lock_irq(sch->lock); > + sch->isc = VFIO_CCW_ISC; > + ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch); > + if (!ret) > + private->state = VFIO_CCW_STATE_STANDBY; nit: could get rid of 'ret' and just do if (!cio_enable...) private->state = VFIO_CCW_STATE_STANDBY; > + spin_unlock_irq(sch->lock); > +} > + > /* > * Device statemachine > */ > @@ -373,29 +389,34 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error, > [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq, > + [VFIO_CCW_EVENT_OPEN] = fsm_open, > }, > [VFIO_CCW_STATE_STANDBY] = { > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error, > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > + [VFIO_CCW_EVENT_OPEN] = fsm_nop, > }, > [VFIO_CCW_STATE_IDLE] = { > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request, > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_request, > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, > }, > [VFIO_CCW_STATE_CP_PROCESSING] = { > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_retry, > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_retry, > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, > }, > [VFIO_CCW_STATE_CP_PENDING] = { > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy, > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_request, > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, > }, > }; > diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h > index 4cfdd5fc0961..8dff1699a7d9 100644 > --- a/drivers/s390/cio/vfio_ccw_private.h > +++ b/drivers/s390/cio/vfio_ccw_private.h > @@ -142,6 +142,7 @@ enum vfio_ccw_event { > VFIO_CCW_EVENT_IO_REQ, > VFIO_CCW_EVENT_INTERRUPT, > VFIO_CCW_EVENT_ASYNC_REQ, > + VFIO_CCW_EVENT_OPEN, > /* last element! */ > NR_VFIO_CCW_EVENTS > };
On Thu, 2022-06-16 at 12:33 -0400, Matthew Rosato wrote: > On 6/15/22 4:33 PM, Eric Farman wrote: > > Move the process of enabling a subchannel for use by vfio-ccw > > into the FSM, such that it can manage the sequence of lifecycle > > events for the device. > > > > That is, if the FSM state is NOT_OPER(erational), then do the work > > that would enable the subchannel and move the FSM to STANDBY state. > > An attempt to perform this event again from any of the other > > operating > > states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device > > back > > to NOT_OPER so the configuration process can be started again. > > Except STANDBY, which ignores the event via fsm_nop. I wonder > though, > whether that's the right thing to do. For each of the other states > you're saying 'if it's already open, go back to NOT_OPER so we can > start > over' -- In this case a STANDBY->STANDBY is also a case of 'it's > already > open' so shouldn't we also go back to NOT_OPER so we can start over? Yeah, the subchannel's already been probed but the mdev hasn't yet. (Or perhaps it did, but that failed.) I was viewing it as a "well there's nothing to do here" but you're right that the rest of the entries take a "oh that's unexpected, go to NOT_OPER" approach. So should make that consistent here, since it would be quite a surprise. > > Seems to me really we just don't expect to ever get an OPEN event > unless > we are in NOT_OPER. > > If there's a reason to keep STANDBY->STANDBY as a nop, but we don't > expect to see it and don't' want to WARN because of it, then maybe a > log > entry at least would make sense. > > As for the IDLE/CP_PROCESSING/CP_PENDING cases, going fsm_notoper > because this is unexpected probably makes sense, but the logging is > going to be really confusing (before this change, you know that you > called fsm_notoper because you got VFIO_CCW_EVENT_NOT_OPER -- now > you'll > see a log entry cut for NOT_OPER but won't be sure if it was for > EVENT_NOT_OPER or EVENT_OPEN). Maybe you can look at 'event' inside > fsm_notoper and cut a slightly different trace entry when arriving > here > for EVENT_OPEN? Yeah, good idea. Since we don't expect any of these in normal behavior, perhaps I'll trace both state and event, instead of trying to make conditionals out of everything. > > ... > > > +static void fsm_open(struct vfio_ccw_private *private, > > + enum vfio_ccw_event event) > > +{ > > + struct subchannel *sch = private->sch; > > + int ret; > > + > > + spin_lock_irq(sch->lock); > > + sch->isc = VFIO_CCW_ISC; > > + ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch); > > + if (!ret) > > + private->state = VFIO_CCW_STATE_STANDBY; > > nit: could get rid of 'ret' and just do > > if (!cio_enable...) > private->state = VFIO_CCW_STATE_STANDBY; Ah, fair. Cut/paste and didn't really consider the simplification. I see that I left the unconditional "private->state = STANDBY" in the hunk just above this, which can be removed. (I finally do in patch 10.) Will make that change too. > > > + spin_unlock_irq(sch->lock); > > +} > > + > > /* > > * Device statemachine > > */ > > @@ -373,29 +389,34 @@ fsm_func_t > > *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { > > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, > > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error, > > [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq, > > + [VFIO_CCW_EVENT_OPEN] = fsm_open, > > }, > > [VFIO_CCW_STATE_STANDBY] = { > > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, > > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error, > > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > > + [VFIO_CCW_EVENT_OPEN] = fsm_nop, > > }, > > [VFIO_CCW_STATE_IDLE] = { > > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request, > > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_request, > > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > > + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, > > }, > > [VFIO_CCW_STATE_CP_PROCESSING] = { > > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_retry, > > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_retry, > > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > > + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, > > }, > > [VFIO_CCW_STATE_CP_PENDING] = { > > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, > > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy, > > [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_request, > > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, > > + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, > > }, > > }; > > diff --git a/drivers/s390/cio/vfio_ccw_private.h > > b/drivers/s390/cio/vfio_ccw_private.h > > index 4cfdd5fc0961..8dff1699a7d9 100644 > > --- a/drivers/s390/cio/vfio_ccw_private.h > > +++ b/drivers/s390/cio/vfio_ccw_private.h > > @@ -142,6 +142,7 @@ enum vfio_ccw_event { > > VFIO_CCW_EVENT_IO_REQ, > > VFIO_CCW_EVENT_INTERRUPT, > > VFIO_CCW_EVENT_ASYNC_REQ, > > + VFIO_CCW_EVENT_OPEN, > > /* last element! */ > > NR_VFIO_CCW_EVENTS > > };
>> As for the IDLE/CP_PROCESSING/CP_PENDING cases, going fsm_notoper >> because this is unexpected probably makes sense, but the logging is >> going to be really confusing (before this change, you know that you >> called fsm_notoper because you got VFIO_CCW_EVENT_NOT_OPER -- now >> you'll >> see a log entry cut for NOT_OPER but won't be sure if it was for >> EVENT_NOT_OPER or EVENT_OPEN). Maybe you can look at 'event' inside >> fsm_notoper and cut a slightly different trace entry when arriving >> here >> for EVENT_OPEN? > > Yeah, good idea. Since we don't expect any of these in normal behavior, > perhaps I'll trace both state and event, instead of trying to make > conditionals out of everything. > Sounds good to me
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index fe87a2652a22..52249c40a565 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -231,11 +231,8 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) dev_set_drvdata(&sch->dev, private); - spin_lock_irq(sch->lock); - sch->isc = VFIO_CCW_ISC; - ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch); - spin_unlock_irq(sch->lock); - if (ret) + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN); + if (private->state == VFIO_CCW_STATE_NOT_OPER) goto out_free; private->state = VFIO_CCW_STATE_STANDBY; diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index bbcc5b486749..7e7ed69e1461 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -11,6 +11,8 @@ #include <linux/vfio.h> +#include <asm/isc.h> + #include "ioasm.h" #include "vfio_ccw_private.h" @@ -364,6 +366,20 @@ static void fsm_irq(struct vfio_ccw_private *private, complete(private->completion); } +static void fsm_open(struct vfio_ccw_private *private, + enum vfio_ccw_event event) +{ + struct subchannel *sch = private->sch; + int ret; + + spin_lock_irq(sch->lock); + sch->isc = VFIO_CCW_ISC; + ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch); + if (!ret) + private->state = VFIO_CCW_STATE_STANDBY; + spin_unlock_irq(sch->lock); +} + /* * Device statemachine */ @@ -373,29 +389,34 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error, [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq, + [VFIO_CCW_EVENT_OPEN] = fsm_open, }, [VFIO_CCW_STATE_STANDBY] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_OPEN] = fsm_nop, }, [VFIO_CCW_STATE_IDLE] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request, [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_request, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, }, [VFIO_CCW_STATE_CP_PROCESSING] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_retry, [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_retry, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, }, [VFIO_CCW_STATE_CP_PENDING] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy, [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_request, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_OPEN] = fsm_notoper, }, }; diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index 4cfdd5fc0961..8dff1699a7d9 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -142,6 +142,7 @@ enum vfio_ccw_event { VFIO_CCW_EVENT_IO_REQ, VFIO_CCW_EVENT_INTERRUPT, VFIO_CCW_EVENT_ASYNC_REQ, + VFIO_CCW_EVENT_OPEN, /* last element! */ NR_VFIO_CCW_EVENTS };