| Message ID | 1527243678-3140-2-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 25 May 2018 12:21:09 +0200 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > Let's move the state change from the IRQ routine to the > workqueue callback. > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > --- > drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++------------- > drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------ > 2 files changed, 15 insertions(+), 19 deletions(-) This causes a change in behaviour for devices in the notoper state. Now: - vfio_ccw_sch_irq is called - via the state machine, disabling the subchannel is (re-)triggered With your patch: - the work function is queued in any case; eventually, it will change the device's state to idle (unless we don't have an mdev at that point in time) - completion is signaled I'm not sure that's what we want.
On 04/06/2018 15:52, Cornelia Huck wrote: > On Fri, 25 May 2018 12:21:09 +0200 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >> Let's move the state change from the IRQ routine to the >> workqueue callback. >> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++------------- >> drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------ >> 2 files changed, 15 insertions(+), 19 deletions(-) > This causes a change in behaviour for devices in the notoper state. > > Now: > - vfio_ccw_sch_irq is called This should not be done if the subchannel is not operational. > - via the state machine, disabling the subchannel is (re-)triggered I removed the fsm_disabled_irq() callback from VFIO_CCW_STATE_NOT_OPER because the subchannel is not even initialized at that moment. We have no reference to the subchannel. In the previous driver NOT_OPER and STANDBY were quite the same. Now NOT_OPER means "we can not operate on this sub channel" because we do not have it in a correct state (no ISC, no mediated device, the probe is not finiched) Now STANDBY means we have the device ready but is disabled. In this case the software infrastructure is ready and if an interrupt comes (what should not happen) we will disable the subchannel again. > > With your patch: > - the work function is queued in any case; eventually, it will change > the device's state to idle (unless we don't have an mdev at that > point in time) > - completion is signaled > > I'm not sure that's what we want. > Yes it is queued in any case but the IRQ is really treated only if the subchannel is in the right state (STANDBY, BUSY, IDLE and QUIESCING). In the NOT_OPER state we do not have the mdev not the driver initialized.
On Tue, 5 Jun 2018 15:34:52 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 04/06/2018 15:52, Cornelia Huck wrote: > > On Fri, 25 May 2018 12:21:09 +0200 > > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > > > >> Let's move the state change from the IRQ routine to the > >> workqueue callback. > >> > >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > >> --- > >> drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++------------- > >> drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------ > >> 2 files changed, 15 insertions(+), 19 deletions(-) > > This causes a change in behaviour for devices in the notoper state. > > > > Now: > > - vfio_ccw_sch_irq is called > > This should not be done if the subchannel is not operational. > > > - via the state machine, disabling the subchannel is (re-)triggered > > I removed the fsm_disabled_irq() callback from VFIO_CCW_STATE_NOT_OPER > because the subchannel is not even initialized at that moment. > We have no reference to the subchannel. > > In the previous driver NOT_OPER and STANDBY were quite the same. > Now NOT_OPER means "we can not operate on this sub channel" > because we do not have it in a correct state (no ISC, no mediated device, > the probe is not finiched) > > Now STANDBY means we have the device ready but is disabled. > In this case the software infrastructure is ready and if an interrupt comes > (what should not happen) we will disable the subchannel again. > > > > > With your patch: > > - the work function is queued in any case; eventually, it will change > > the device's state to idle (unless we don't have an mdev at that > > point in time) > > - completion is signaled > > > > I'm not sure that's what we want. > > > > Yes it is queued in any case but the IRQ is really treated only if the > subchannel is in the right state (STANDBY, BUSY, IDLE and QUIESCING). > > In the NOT_OPER state we do not have the mdev not the driver initialized. But all of this is only true after the whole series has been applied, isn't it? Is there any way to do the changes without breaking things inbetween? What would also be very helpful is a sketch of the state machine after your rework is done. Otherwise, this leaves me a bit unsure about the intended semantics if I just look at the individual patches.
On 05/06/2018 15:52, Cornelia Huck wrote: > On Tue, 5 Jun 2018 15:34:52 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 04/06/2018 15:52, Cornelia Huck wrote: >>> On Fri, 25 May 2018 12:21:09 +0200 >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: >>> >>>> Let's move the state change from the IRQ routine to the >>>> workqueue callback. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >>>> --- >>>> drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++------------- >>>> drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------ >>>> 2 files changed, 15 insertions(+), 19 deletions(-) >>> This causes a change in behaviour for devices in the notoper state. >>> >>> Now: >>> - vfio_ccw_sch_irq is called >> This should not be done if the subchannel is not operational. >> >>> - via the state machine, disabling the subchannel is (re-)triggered >> I removed the fsm_disabled_irq() callback from VFIO_CCW_STATE_NOT_OPER >> because the subchannel is not even initialized at that moment. >> We have no reference to the subchannel. >> >> In the previous driver NOT_OPER and STANDBY were quite the same. >> Now NOT_OPER means "we can not operate on this sub channel" >> because we do not have it in a correct state (no ISC, no mediated device, >> the probe is not finiched) >> >> Now STANDBY means we have the device ready but is disabled. >> In this case the software infrastructure is ready and if an interrupt comes >> (what should not happen) we will disable the subchannel again. >> >>> With your patch: >>> - the work function is queued in any case; eventually, it will change >>> the device's state to idle (unless we don't have an mdev at that >>> point in time) >>> - completion is signaled >>> >>> I'm not sure that's what we want. >>> >> Yes it is queued in any case but the IRQ is really treated only if the >> subchannel is in the right state (STANDBY, BUSY, IDLE and QUIESCING). >> >> In the NOT_OPER state we do not have the mdev not the driver initialized. > But all of this is only true after the whole series has been applied, > isn't it? Is there any way to do the changes without breaking things > inbetween? I will think about this. May be just disable the all thing untill all patches applied? > > What would also be very helpful is a sketch of the state machine after > your rework is done. Otherwise, this leaves me a bit unsure about the > intended semantics if I just look at the individual patches. > Right, I must enhance the cover letter.
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index ea6a2d0..57ae1ab 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -70,20 +70,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch) static void vfio_ccw_sch_io_todo(struct work_struct *work) { struct vfio_ccw_private *private; - struct irb *irb; private = container_of(work, struct vfio_ccw_private, io_work); - irb = &private->irb; - - if (scsw_is_solicited(&irb->scsw)) { - cp_update_scsw(&private->cp, &irb->scsw); - cp_free(&private->cp); - } - memcpy(private->io_region.irb_area, irb, sizeof(*irb)); - - if (private->io_trigger) - eventfd_signal(private->io_trigger, 1); - + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT); if (private->mdev) private->state = VFIO_CCW_STATE_IDLE; } @@ -94,9 +83,14 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) static void vfio_ccw_sch_irq(struct subchannel *sch) { struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); + struct irb *irb = this_cpu_ptr(&cio_irb); inc_irq_stat(IRQIO_CIO); - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT); + memcpy(&private->irb, irb, sizeof(*irb)); + + queue_work(vfio_ccw_work_q, &private->io_work); + if (private->completion) + complete(private->completion); } static int vfio_ccw_sch_probe(struct subchannel *sch) diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index 3c80064..24e1e04 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -172,14 +172,16 @@ static void fsm_io_request(struct vfio_ccw_private *private, static void fsm_irq(struct vfio_ccw_private *private, enum vfio_ccw_event event) { - struct irb *irb = this_cpu_ptr(&cio_irb); + struct irb *irb = &private->irb; - memcpy(&private->irb, irb, sizeof(*irb)); - - queue_work(vfio_ccw_work_q, &private->io_work); + if (scsw_is_solicited(&irb->scsw)) { + cp_update_scsw(&private->cp, &irb->scsw); + cp_free(&private->cp); + } + memcpy(private->io_region.irb_area, irb, sizeof(*irb)); - if (private->completion) - complete(private->completion); + if (private->io_trigger) + eventfd_signal(private->io_trigger, 1); } /*
Let's move the state change from the IRQ routine to the workqueue callback. Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> --- drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++------------- drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------ 2 files changed, 15 insertions(+), 19 deletions(-)