Re: [qxl-win PATCH 2/2] display: fix deadlock when dbg_level >= 15

[Yonit Halperin <yhalperi@xxxxxxxxxx>]

On 06/23/2013 06:30 AM, Uri Lublin wrote:
> On 06/21/2013 04:50 PM, Yonit Halperin wrote:
> > DebugPrintV first locks print_sem, and then locks io_sem.
> > async_io, locks io_sem.
> > In ordr to avoid a deadlock, DebugPrintV MUSTN'T be called when
> > io_sem is locked.
> >
> > Also notice, that locking io_sem during DebugPrintV limits our ability
> > to use the log_port for debugging concurrency problems related to ios.
> > ---
> >   xddm/display/qxldd.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xddm/display/qxldd.h b/xddm/display/qxldd.h
> > index 1a1b5d5..362c293 100644
> > --- a/xddm/display/qxldd.h
> > +++ b/xddm/display/qxldd.h
> > @@ -474,6 +474,7 @@ static _inline void async_io(PDev *pdev,
> > asyncable_t op, UCHAR val)
> >       ULONG64 millis;
> >       if (pdev->use_async) {
> > +        DEBUG_PRINT((pdev, 3, "%s: start async %d\n", __FUNCTION__,
> > (int)op));
> >           EngAcquireSemaphore(pdev->io_sem);
> >           WRITE_PORT_UCHAR(pdev->asyncable[op][ASYNC], val);
> >           /* Our Interrupt may be taken from us unexpectedly, by a
> > surprise removal.
> > @@ -482,7 +483,6 @@ static _inline void async_io(PDev *pdev,
> > asyncable_t op, UCHAR val)
> >            * we get reset. We use EngQueryLocalTime because there is
> > no way to differentiate a return on
> >            * timeout from a return on event set otherwise. */
> >           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);
> > @@ -492,7 +492,7 @@ static _inline void async_io(PDev *pdev,
> > asyncable_t op, UCHAR val)
> >               DEBUG_PRINT((pdev, 0, "%s: timeout reached, disabling
> > async io!\n", __FUNCTION__));
>
> This DEBUG_PRINT should be deleted or marked and postpone to
> after pdev->io_sem is released below.
You are right, and probably we should better lock io_sem before testing 
pdev->use_async...
> >           }
> >           EngReleaseSemaphore(pdev->io_sem);
> > -        DEBUG_PRINT((pdev, 3, "finished async %d\n", (int)op));
> > +        DEBUG_PRINT((pdev, 3, "%s: finished async %d\n",
> > __FUNCTION__, (int)op));
> >       } else {
> >           if (pdev->asyncable[op][SYNC] == NULL) {
> >               DEBUG_PRINT((pdev, 0, "ERROR: trying calling sync io on
> > NULL port %d\n", op));

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel