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.
>
> This does not happen on real hardware because real hardware never send
> interrupt immediately after the controller has been moved to OPERATION state.
>
> This patch tries to delay the first SOF interrupt to let driver exits from
> the critical section (which is not protected against interrupts...)
>
> Some details:
>
> - 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.
>
> - 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.
>
> Suggested-by: Gerd Hoffmann <kra...@redhat.com>
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
> ---
> hw/usb/hcd-ohci.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> 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)
> }
>
> /* 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 = 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);
> + }
> }
>
> /* Process Control and Bulk lists. */
> @@ -1318,7 +1320,7 @@ static void ohci_frame_boundary(void *opaque)
> ohci->done_count--;
>
> /* Do SOF stuff here */
> - ohci_sof(ohci);
> + ohci_sof(ohci, false);
>
> /* Writeback HCCA */
> if (ohci_put_hcca(ohci, ohci->hcca, &hcca)) {
> @@ -1343,7 +1345,7 @@ static int ohci_bus_start(OHCIState *ohci)
>
> trace_usb_ohci_start(ohci->name);
>
> - ohci_sof(ohci);
> + ohci_sof(ohci, true);
>
> return 1;
> }
<bikeshedpainting>
Alternate idea: Split ohci_sof into two functions, e.g. like this:
static void ohci_sof_timer(OHCIState *ohci)
{
ohci->sof_time = 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.
</bikeshedpainting>
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
