On 10/18/2018 09:12 AM, Cornelia Huck wrote:
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.
Per Pierre's response, I don't share Halil's concern here. But I think
the state stuff gets better with the other patches in the RFC series.
So if patch 1 waits until we get a chance to look at its cousins, I'm
fine with that too. (I haven't looked at them myself, I just remember
an earlier series and thinking it looked promising. I still intend to
look at them as time permits.)
- Eric
(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
