| Message ID | 20220602171948.2790690-3-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO ccw/mdev rework | expand |
On Thu, Jun 02, 2022 at 07:19:32PM +0200, Eric Farman wrote: > The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(), > and this routine converts it to IDLE as part of its processing. > The error exit sets it to IDLE (again) but clears the private->mdev > pointer. > > The FSM should of course be managing the state itself, but the > correct thing for vfio_ccw_mdev_probe() to do would be to put > the state back the way it found it. > > The corresponding check of private->mdev in vfio_ccw_sch_io_todo() > can be removed, since the distinction is unnecessary at this point. > > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_drv.c | 2 +- > drivers/s390/cio/vfio_ccw_ops.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On 6/2/22 1:19 PM, Eric Farman wrote: > The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(), > and this routine converts it to IDLE as part of its processing. > The error exit sets it to IDLE (again) but clears the private->mdev > pointer. > > The FSM should of course be managing the state itself, but the > correct thing for vfio_ccw_mdev_probe() to do would be to put > the state back the way it found it. > > The corresponding check of private->mdev in vfio_ccw_sch_io_todo() > can be removed, since the distinction is unnecessary at this point. > > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_drv.c | 2 +- > drivers/s390/cio/vfio_ccw_ops.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c > index 35055eb94115..b18b4582bc8b 100644 > --- a/drivers/s390/cio/vfio_ccw_drv.c > +++ b/drivers/s390/cio/vfio_ccw_drv.c > @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) > * has finished. Do not overwrite a possible processing > * state if the final interrupt was for HSCH or CSCH. > */ > - if (private->mdev && cp_is_finished) > + if (cp_is_finished) > private->state = VFIO_CCW_STATE_IDLE; Took me a bit to convince myself this was OK, mainly because AFAICT despite the change below the fsm jumptable would still allow you to reach this code when in STANDBY. But, it should only be possible for an unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so we would still avoid a STANDBY->IDLE transition on accident. Maybe work unsolicited interrupt into the comment block above along with HSCH/CSCH? Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> > > if (private->io_trigger) > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > index bebae21228aa..a403d059a4e6 100644 > --- a/drivers/s390/cio/vfio_ccw_ops.c > +++ b/drivers/s390/cio/vfio_ccw_ops.c > @@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev) > vfio_uninit_group_dev(&private->vdev); > atomic_inc(&private->avail); > private->mdev = NULL; > - private->state = VFIO_CCW_STATE_IDLE; > + private->state = VFIO_CCW_STATE_STANDBY;
On Fri, 2022-06-03 at 09:21 -0400, Matthew Rosato wrote: > On 6/2/22 1:19 PM, Eric Farman wrote: > > The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(), > > and this routine converts it to IDLE as part of its processing. > > The error exit sets it to IDLE (again) but clears the private->mdev > > pointer. > > > > The FSM should of course be managing the state itself, but the > > correct thing for vfio_ccw_mdev_probe() to do would be to put > > the state back the way it found it. > > > > The corresponding check of private->mdev in vfio_ccw_sch_io_todo() > > can be removed, since the distinction is unnecessary at this point. > > > > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use > > vfio_register_emulated_iommu_dev()") > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > > --- > > drivers/s390/cio/vfio_ccw_drv.c | 2 +- > > drivers/s390/cio/vfio_ccw_ops.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/s390/cio/vfio_ccw_drv.c > > b/drivers/s390/cio/vfio_ccw_drv.c > > index 35055eb94115..b18b4582bc8b 100644 > > --- a/drivers/s390/cio/vfio_ccw_drv.c > > +++ b/drivers/s390/cio/vfio_ccw_drv.c > > @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct > > work_struct *work) > > * has finished. Do not overwrite a possible processing > > * state if the final interrupt was for HSCH or CSCH. > > */ > > - if (private->mdev && cp_is_finished) > > + if (cp_is_finished) > > private->state = VFIO_CCW_STATE_IDLE; > > Took me a bit to convince myself this was OK Me too. :) > , mainly because AFAICT > despite the change below the fsm jumptable would still allow you to > reach this code when in STANDBY. But, it should only be possible for > an > unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so > we > would still avoid a STANDBY->IDLE transition on accident. > > Maybe work unsolicited interrupt into the comment block above along > with > HSCH/CSCH? Good idea. How about: /* * Reset to IDLE only if processing of a channel program * has finished. Do not overwrite a possible processing * state if the interrupt was unsolicited, or if the final * interrupt was for HSCH or CSCH. */ > > Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > > > if (private->io_trigger) > > diff --git a/drivers/s390/cio/vfio_ccw_ops.c > > b/drivers/s390/cio/vfio_ccw_ops.c > > index bebae21228aa..a403d059a4e6 100644 > > --- a/drivers/s390/cio/vfio_ccw_ops.c > > +++ b/drivers/s390/cio/vfio_ccw_ops.c > > @@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct > > mdev_device *mdev) > > vfio_uninit_group_dev(&private->vdev); > > atomic_inc(&private->avail); > > private->mdev = NULL; > > - private->state = VFIO_CCW_STATE_IDLE; > > + private->state = VFIO_CCW_STATE_STANDBY;
On 6/3/22 3:12 PM, Eric Farman wrote: > On Fri, 2022-06-03 at 09:21 -0400, Matthew Rosato wrote: >> On 6/2/22 1:19 PM, Eric Farman wrote: >>> The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(), >>> and this routine converts it to IDLE as part of its processing. >>> The error exit sets it to IDLE (again) but clears the private->mdev >>> pointer. >>> >>> The FSM should of course be managing the state itself, but the >>> correct thing for vfio_ccw_mdev_probe() to do would be to put >>> the state back the way it found it. >>> >>> The corresponding check of private->mdev in vfio_ccw_sch_io_todo() >>> can be removed, since the distinction is unnecessary at this point. >>> >>> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use >>> vfio_register_emulated_iommu_dev()") >>> Signed-off-by: Eric Farman <farman@linux.ibm.com> >>> --- >>> drivers/s390/cio/vfio_ccw_drv.c | 2 +- >>> drivers/s390/cio/vfio_ccw_ops.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c >>> b/drivers/s390/cio/vfio_ccw_drv.c >>> index 35055eb94115..b18b4582bc8b 100644 >>> --- a/drivers/s390/cio/vfio_ccw_drv.c >>> +++ b/drivers/s390/cio/vfio_ccw_drv.c >>> @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct >>> work_struct *work) >>> * has finished. Do not overwrite a possible processing >>> * state if the final interrupt was for HSCH or CSCH. >>> */ >>> - if (private->mdev && cp_is_finished) >>> + if (cp_is_finished) >>> private->state = VFIO_CCW_STATE_IDLE; >> >> Took me a bit to convince myself this was OK > > Me too. :) > >> , mainly because AFAICT >> despite the change below the fsm jumptable would still allow you to >> reach this code when in STANDBY. But, it should only be possible for >> an >> unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so >> we >> would still avoid a STANDBY->IDLE transition on accident. >> >> Maybe work unsolicited interrupt into the comment block above along >> with >> HSCH/CSCH? > > Good idea. How about: > > /* > * Reset to IDLE only if > processing of a channel program > * has finished. Do not > overwrite a possible processing > * state if the interrupt was > unsolicited, or if the final > * interrupt was for HSCH or CSCH. > > */ > Sounds good to me
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 35055eb94115..b18b4582bc8b 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) * has finished. Do not overwrite a possible processing * state if the final interrupt was for HSCH or CSCH. */ - if (private->mdev && cp_is_finished) + if (cp_is_finished) private->state = VFIO_CCW_STATE_IDLE; if (private->io_trigger) diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index bebae21228aa..a403d059a4e6 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev) vfio_uninit_group_dev(&private->vdev); atomic_inc(&private->avail); private->mdev = NULL; - private->state = VFIO_CCW_STATE_IDLE; + private->state = VFIO_CCW_STATE_STANDBY; return ret; }
The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(), and this routine converts it to IDLE as part of its processing. The error exit sets it to IDLE (again) but clears the private->mdev pointer. The FSM should of course be managing the state itself, but the correct thing for vfio_ccw_mdev_probe() to do would be to put the state back the way it found it. The corresponding check of private->mdev in vfio_ccw_sch_io_todo() can be removed, since the distinction is unnecessary at this point. Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()") Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_drv.c | 2 +- drivers/s390/cio/vfio_ccw_ops.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)