On Thu, 18 Oct 2018 14:38:22 +0200
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
> On 10/18/2018 09:51 AM, Pierre Morel wrote:
> > VFIO_CCW_STATE_BOXED and VFIO_CCW_STATE_BUSY have
> > identical actions for the same events.
> >
> > Let's merge both into a single state to simplify the code.
> > We choose to keep VFIO_CCW_STATE_BUSY.
> >
>
> Your commit message does not convince me. From what I
> see VFIO_CCW_STATE_BOXED was a bad idea in the first
> place:
> * is very very different than DEV_STATE_BOXED,
> * its role is obscure and undocumented.
>
> With your patch however, we get a weird VFIO_CCW_STATE_BUSY -->
> VFIO_CCW_STATE_BUSY state transition, which I don't consider
> much better:
> First we go into VFIO_CCW_STATE_BUSY in fsm_io_request() and then
> again in fsm_io_helper() which is called by fsm_io_request().
>
> So good path used to go
>
> VFIO_CCW_STATE_IDLE --> VFIO_CCW_STATE_BOXED --> VFIO_CCW_STATE_BUSY --> ?
> and with your patch it goes
> VFIO_CCW_STATE_IDLE --> VFIO_CCW_STATE_BUSY --> VFIO_CCW_STATE_BUSY --> ?
>
> From what I see, the role of VFIO_CCW_STATE_BOXED is to differentiate
> between 'doing I/O on the host subchannel' and 'looking at the request,
> doing translation, and whatever we need to before interacting with the
> host subchannel'. Normally state is exposed via sysfs, but for vfio-ccw
> it ain't. Does anybody know why?
>
> I don't think we need to differentiate between the two, so I'm not against
> removing BOXED. But I'm against the VFIO_CCW_STATE_BUSY --> VFIO_CCW_STATE_BUSY
> transition, and we need a better commit message.
OK, so I can pick patch 2 (if you're happy with that) and we can
discuss this one later.
(Well, I'll probably pick patch 2 next week, but you get the meaning :)
>
> Regards,
> Halil
>
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > ---
> > drivers/s390/cio/vfio_ccw_fsm.c | 7 +------
> > drivers/s390/cio/vfio_ccw_private.h | 1 -
> > 2 files changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index f94aa01..cab1786 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -130,7 +130,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> > struct mdev_device *mdev = private->mdev;
> > char *errstr = "request";
> >
> > - private->state = VFIO_CCW_STATE_BOXED;
> > + private->state = VFIO_CCW_STATE_BUSY;
> >
> > memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
> >
> > @@ -216,11 +216,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
> > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> > },
> > - [VFIO_CCW_STATE_BOXED] = {
> > - [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> > - [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
> > - [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> > - },
> > [VFIO_CCW_STATE_BUSY] = {
> > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> > index 078e46f..08e9a7d 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -63,7 +63,6 @@ enum vfio_ccw_state {
> > VFIO_CCW_STATE_NOT_OPER,
> > VFIO_CCW_STATE_STANDBY,
> > VFIO_CCW_STATE_IDLE,
> > - VFIO_CCW_STATE_BOXED,
> > VFIO_CCW_STATE_BUSY,
> > /* last element! */
> > NR_VFIO_CCW_STATES
> >
>
