[1/2] ohci: delay first SOF interrupt

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 <kraxel@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/usb/hcd-ohci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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 <kraxel@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@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