Re: [Qemu-devel] [PATCH 1/2] ohci: delay first SOF interrupt

[Thomas Huth <thuth@redhat.com>]

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