From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47063) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Uog-00054D-7m for qemu-devel@nongnu.org; Thu, 17 Dec 2015 04:25:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9Uoc-0001LU-1b for qemu-devel@nongnu.org; Thu, 17 Dec 2015 04:25:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55946) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Uob-0001LP-PW for qemu-devel@nongnu.org; Thu, 17 Dec 2015 04:25:37 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 7AA99C7946 for ; Thu, 17 Dec 2015 09:25:37 +0000 (UTC) References: <1450273165-2367-1-git-send-email-lvivier@redhat.com> <1450273165-2367-2-git-send-email-lvivier@redhat.com> From: Thomas Huth Message-ID: <56727F8E.2000609@redhat.com> Date: Thu, 17 Dec 2015 10:25:34 +0100 MIME-Version: 1.0 In-Reply-To: <1450273165-2367-2-git-send-email-lvivier@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] ohci: delay first SOF interrupt List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier , qemu-devel@nongnu.org Cc: Gerd Hoffmann On 16/12/15 14:39, Laurent Vivier wrote: > On overcommitted CPU, kernel can be so slow that an interrupt can > be triggered by the device whereas the driver is not ready to receive > it. This drives us into an infinite loop. >=20 > This does not happen on real hardware because real hardware never send > interrupt immediately after the controller has been moved to OPERATION = state. >=20 > This patch tries to delay the first SOF interrupt to let driver exits f= rom > the critical section (which is not protected against interrupts...) >=20 > Some details: >=20 > - ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ > only if the state of the driver (rh_state) is OHCI_STATE_RUNNING. > So if this interrupt happens and the driver is not in this state, > the function is called again and again, moving the system to a > CPU starvation. >=20 > - ohci_rh_resume(): the driver re-enables operation with OHCI_USB_OPER. > In QEMU this start the SOF timer and QEMU starts to send IRQs. As > the driver is not in OHCI_STATE_RUNNING and not protected against IRQ= , > the ohci_irq() can be called and the driver never moved to > OHCI_STATE_RUNNING. >=20 > Suggested-by: Gerd Hoffmann > Signed-off-by: Laurent Vivier > --- > hw/usb/hcd-ohci.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) >=20 > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index 7d65818..5f15ebb 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -1232,11 +1232,13 @@ static int ohci_service_ed_list(OHCIState *ohci= , uint32_t head, int completion) > } > =20 > /* Generate a SOF event, and set a timer for EOF */ May I suggest to reflect the new behavior (with the explanation) in the comment above? ... otherwise this might be hard to understand in a couple of years if you only read the source code and not the changelog. > -static void ohci_sof(OHCIState *ohci) > +static void ohci_sof(OHCIState *ohci, bool first) > { > ohci->sof_time =3D qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > timer_mod(ohci->eof_timer, ohci->sof_time + usb_frame_time); > - ohci_set_interrupt(ohci, OHCI_INTR_SF); > + if (!first) { > + ohci_set_interrupt(ohci, OHCI_INTR_SF); > + } > } > =20 > /* Process Control and Bulk lists. */ > @@ -1318,7 +1320,7 @@ static void ohci_frame_boundary(void *opaque) > ohci->done_count--; > =20 > /* Do SOF stuff here */ > - ohci_sof(ohci); > + ohci_sof(ohci, false); > =20 > /* Writeback HCCA */ > if (ohci_put_hcca(ohci, ohci->hcca, &hcca)) { > @@ -1343,7 +1345,7 @@ static int ohci_bus_start(OHCIState *ohci) > =20 > trace_usb_ohci_start(ohci->name); > =20 > - ohci_sof(ohci); > + ohci_sof(ohci, true); > =20 > return 1; > } Alternate idea: Split ohci_sof into two functions, e.g. like this: static void ohci_sof_timer(OHCIState *ohci) { ohci->sof_time =3D qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); timer_mod(ohci->eof_timer, ohci->sof_time + usb_frame_time); } static void ohci_sof(OHCIState *ohci) { ohci_sof_timer(ohci); ohci_set_interrupt(ohci, OHCI_INTR_SF); } ... and then only call ohci_sof_timer() in ohci_bus_start(). I think that would be a little bit easier to read than the stuff with the "first" parameter. Anyway, the patch looks basically like a good idea to me, so I'm also fine with the original form if you don't want to change it. Thomas