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
> > };
