[Spice-devel] [RFC qxl-win 2/2] display: handle interrupt handler disappearance (rhbz 721118)

[Uri Lublin]

On 09/21/2011 04:13 PM, Alon Levy wrote:
> This patch handles the surprise removal whql test. The surprise
> removal test sends a surprise removal IRP via a filter driver to
> qxl.sys. The handling of that IRP is actually done in videoprt.sys,
> and there is no API to get notified. The side effect of the handler
> in videoprt.sys is disabling our interrupt handler. This patch takes
> care of continuing to work after our interrupt is disabled. This is
> in effect the opposite of what the MSDN states that a device should do
> after a surprise removal, since the MSDN requires every IO to fail after
> the surprise removal IRP has been successfully handled. On the other
> hand, this workaround is sufficient to pass the test.
>
> The workaround is to turn the EngWaitForSingleEvent on the io_cmd_event
> event into a timed wait.
>
> If it fails, we revert to synchronouse IO, which doesn't require that
> event at all.
> ---
>   display/qxldd.h |   39 ++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 38 insertions(+), 1 deletions(-)
>
> diff --git a/display/qxldd.h b/display/qxldd.h
> @@ -449,10 +468,28 @@ static _inline RingItem *RingGetTail(PDev *pdev, Ring *ring)
>    */
>   static _inline void async_io(PDev *pdev, asyncable_t op, UCHAR val)
>   {
> +    ENG_TIME_FIELDS start, finish;
> +    LARGE_INTEGER timeout;                      // 1 =>  100 nanoseconds
> +    ULONG64 millis;
> +
>       if (pdev->use_async) {
>           EngAcquireSemaphore(pdev->io_sem);
>           WRITE_PORT_UCHAR(pdev->asyncable[op][ASYNC], val);
> -        WAIT_FOR_EVENT(pdev, pdev->io_cmd_event, NULL);
> +        /* Our Interrupt may be taken from us unexpectedly, by a surprise removal.
> +         * in which case this event will never be set. This happens only during WHQL
> +         * tests (pnpdtest /surprise). So instead: Wait on a timer, if we fail, stop waiting, until
> +         * we get reset. We use EngQueryLocalTime because there is no way to differentiate a return on
> +         * timeout from a return on event set otherwise. */

A couple of suggestions:
1. Can you use time() ? It would make the code a little bit simpler.
2. Is the timeout long enough (2 seconds) ? Maybe use a longer timeout for 
surprise removal.

> +        timeout.QuadPart = -INTERRUPT_NOT_PRESENT_TIMEOUT_100NS; // negative  =>  relative
> +        DEBUG_PRINT((pdev, 15, "WAIT_FOR_EVENT %d\n", (int)op));
> +        EngQueryLocalTime(&start);
> +        WAIT_FOR_EVENT(pdev, pdev->io_cmd_event,&timeout);
> +        EngQueryLocalTime(&finish);
> +        millis = eng_time_diff_ms(&finish,&start);
> +        if (millis>= INTERRUPT_NOT_PRESENT_TIMEOUT_MS) {
> +            pdev->use_async = 0;
> +            DEBUG_PRINT((pdev, 0, "%s: timeout reached, disabling async io!\n", __FUNCTION__));
> +        }
>           EngReleaseSemaphore(pdev->io_sem);
>           DEBUG_PRINT((pdev, 3, "finished async %d\n", (int)op));
>       } else {