From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v3 1/1] vfio-ccw: Prevent quiesce function going into an infinite loop References: <4d5a4b98ab1b41ac6131b5c36de18b76c5d66898.1555449329.git.alifm@linux.ibm.com> <20190417110348.28efc8e3.cohuck@redhat.com> From: Eric Farman Date: Wed, 17 Apr 2019 09:58:24 -0400 MIME-Version: 1.0 In-Reply-To: <20190417110348.28efc8e3.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Message-Id: Sender: kvm-owner@vger.kernel.org List-Archive: List-Post: To: Cornelia Huck , Farhan Ali Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org, pasic@linux.ibm.com, pmorel@linux.ibm.com List-ID: On 4/17/19 5:03 AM, Cornelia Huck wrote: > On Tue, 16 Apr 2019 17:23:14 -0400 > Farhan Ali wrote: > >> The quiesce function calls cio_cancel_halt_clear() and if we >> get an -EBUSY we go into a loop where we: >> - wait for any interrupts >> - flush all I/O in the workqueue >> - retry cio_cancel_halt_clear >> >> During the period where we are waiting for interrupts or >> flushing all I/O, the channel subsystem could have completed >> a halt/clear action and turned off the corresponding activity >> control bits in the subchannel status word. This means the next >> time we call cio_cancel_halt_clear(), we will again start by >> calling cancel subchannel and so we can be stuck between calling >> cancel and halt forever. >> >> Rather than calling cio_cancel_halt_clear() immediately after >> waiting, let's try to disable the subchannel. If we succeed in >> disabling the subchannel then we know nothing else can happen >> with the device. >> >> Suggested-by: Eric Farman >> Signed-off-by: Farhan Ali >> --- >> ChangeLog: >> v2 -> v3 >> - Log an error message when cio_cancel_halt_clear >> returns EIO and break out of the loop. >> >> - Did not include past change log as the other patches >> of the original series have been queued by Conny. >> Old series (v2) can be found here: >> https://marc.info/?l=kvm&m=155475754101769&w=2 >> >> drivers/s390/cio/vfio_ccw_drv.c | 32 ++++++++++++++++++-------------- >> 1 file changed, 18 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c >> index 78517aa..66a66ac 100644 >> --- a/drivers/s390/cio/vfio_ccw_drv.c >> +++ b/drivers/s390/cio/vfio_ccw_drv.c >> @@ -43,26 +43,30 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch) >> if (ret != -EBUSY) >> goto out_unlock; >> >> + iretry = 255; >> do { >> - iretry = 255; >> >> ret = cio_cancel_halt_clear(sch, &iretry); >> - while (ret == -EBUSY) { >> - /* >> - * Flush all I/O and wait for >> - * cancel/halt/clear completion. >> - */ >> - private->completion = &completion; >> - spin_unlock_irq(sch->lock); >> >> - wait_for_completion_timeout(&completion, 3*HZ); >> + if (ret == -EIO) { >> + pr_err("vfio_ccw: could not quiesce subchannel 0.%x.%04x!\n", >> + sch->schid.ssid, sch->schid.sch_no); > > What about using > dev_err(&sch->dev, "could not quiesce"); > instead? +1 > > (Can make that change while applying, no need to resend for that.) > >> + break; >> + } >> + >> + /* >> + * Flush all I/O and wait for >> + * cancel/halt/clear completion. >> + */ >> + private->completion = &completion; >> + spin_unlock_irq(sch->lock); >> >> - private->completion = NULL; >> - flush_workqueue(vfio_ccw_work_q); >> - spin_lock_irq(sch->lock); >> - ret = cio_cancel_halt_clear(sch, &iretry); >> - }; >> + if (ret == -EBUSY) >> + wait_for_completion_timeout(&completion, 3*HZ); >> >> + private->completion = NULL; >> + flush_workqueue(vfio_ccw_work_q); >> + spin_lock_irq(sch->lock); >> ret = cio_disable_subchannel(sch); >> } while (ret == -EBUSY); >> out_unlock: > > Otherwise, looks good to me. Will queue when I get some ack/r-b. > I like it, but I feel weird giving an r-b to something I suggested: Acked-by: Eric Farman