From cc1d054d9ca302689c3900854befe43ac9339ab0 Mon Sep 17 00:00:00 2001 From: iedowse Date: Sat, 19 Mar 2005 19:08:46 +0000 Subject: [PATCH] It was possible to have two threads concurrently aborting the same transfer, which lead to panics or page faults. For example if a transfer timed out, another thread could come along and attempt to abort the same transfer while the timeout task was sleeping in the *_abort_xfer() function. Add an "aborting" flag to the private transfer state in each host controller driver and use this to ensure that the abort is only executed once. Also prioritise normal abort requests over timeouts so that the callback is always given a status of USB_CANCELLED even if the timeout-initiated abort began first. The crashes caused by this bug were mainly reported in connection with lpd printing to a USB printer. PR: usb/78208, usb/78986 --- sys/dev/usb/ehci.c | 26 ++++++++++++++++++++++++++ sys/dev/usb/ehcivar.h | 4 ++++ sys/dev/usb/ohci.c | 28 ++++++++++++++++++++++++++++ sys/dev/usb/ohcivar.h | 2 ++ sys/dev/usb/uhci.c | 29 ++++++++++++++++++++++++++++- sys/dev/usb/uhcivar.h | 4 ++++ 6 files changed, 92 insertions(+), 1 deletion(-) diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c index a52824a722e8..d85a826dd3c0 100644 --- a/sys/dev/usb/ehci.c +++ b/sys/dev/usb/ehci.c @@ -1179,6 +1179,7 @@ ehci_allocx(struct usbd_bus *bus) memset(xfer, 0, sizeof(struct ehci_xfer)); usb_init_task(&EXFER(xfer)->abort_task, ehci_timeout_task, xfer); + EXFER(xfer)->ehci_xfer_flags = 0; #ifdef DIAGNOSTIC EXFER(xfer)->isdone = 1; xfer->busy_free = XFER_BUSY; @@ -2518,10 +2519,29 @@ ehci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) if (xfer->device->bus->intr_context || !curproc) panic("ehci_abort_xfer: not in process context"); + /* + * If an abort is already in progress then just wait for it to + * complete and return. + */ + if (exfer->ehci_xfer_flags & EHCI_XFER_ABORTING) { + DPRINTFN(2, ("ehci_abort_xfer: already aborting\n")); + /* No need to wait if we're aborting from a timeout. */ + if (status == USBD_TIMEOUT) + return; + /* Override the status which might be USBD_TIMEOUT. */ + xfer->status = status; + DPRINTFN(2, ("ehci_abort_xfer: waiting for abort to finish\n")); + exfer->ehci_xfer_flags |= EHCI_XFER_ABORTWAIT; + while (exfer->ehci_xfer_flags & EHCI_XFER_ABORTING) + tsleep(&exfer->ehci_xfer_flags, PZERO, "ehciaw", 0); + return; + } + /* * Step 1: Make interrupt routine and timeouts ignore xfer. */ s = splusb(); + exfer->ehci_xfer_flags |= EHCI_XFER_ABORTING; xfer->status = status; /* make software ignore it */ usb_uncallout(xfer->timeout_handle, ehci_timeout, xfer); usb_rem_task(epipe->pipe.device, &exfer->abort_task); @@ -2639,6 +2659,12 @@ ehci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) #ifdef DIAGNOSTIC exfer->isdone = 1; #endif + /* Do the wakeup first to avoid touching the xfer after the callback. */ + exfer->ehci_xfer_flags &= ~EHCI_XFER_ABORTING; + if (exfer->ehci_xfer_flags & EHCI_XFER_ABORTWAIT) { + exfer->ehci_xfer_flags &= ~EHCI_XFER_ABORTWAIT; + wakeup(&exfer->ehci_xfer_flags); + } usb_transfer_complete(xfer); /* printf("%s: %d TDs aborted\n", __func__, count); */ diff --git a/sys/dev/usb/ehcivar.h b/sys/dev/usb/ehcivar.h index 0639b70f0170..fe791ec36d43 100644 --- a/sys/dev/usb/ehcivar.h +++ b/sys/dev/usb/ehcivar.h @@ -65,10 +65,14 @@ struct ehci_xfer { LIST_ENTRY(ehci_xfer) inext; /* list of active xfers */ ehci_soft_qtd_t *sqtdstart; ehci_soft_qtd_t *sqtdend; + u_int32_t ehci_xfer_flags; #ifdef DIAGNOSTIC int isdone; #endif }; +#define EHCI_XFER_ABORTING 0x0001 /* xfer is aborting. */ +#define EHCI_XFER_ABORTWAIT 0x0002 /* abort completion is being awaited. */ + #define EXFER(xfer) ((struct ehci_xfer *)(xfer)) /* diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c index 47e8b3fa01dd..e0db856e797a 100644 --- a/sys/dev/usb/ohci.c +++ b/sys/dev/usb/ohci.c @@ -998,6 +998,7 @@ ohci_allocx(struct usbd_bus *bus) memset(xfer, 0, sizeof (struct ohci_xfer)); usb_init_task(&OXFER(xfer)->abort_task, ohci_timeout_task, xfer); + OXFER(xfer)->ohci_xfer_flags = 0; #ifdef DIAGNOSTIC xfer->busy_free = XFER_BUSY; #endif @@ -2254,6 +2255,7 @@ ohci_close_pipe(usbd_pipe_handle pipe, ohci_soft_ed_t *head) void ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) { + struct ohci_xfer *oxfer = OXFER(xfer); struct ohci_pipe *opipe = (struct ohci_pipe *)xfer->pipe; ohci_softc_t *sc = (ohci_softc_t *)opipe->pipe.device->bus; ohci_soft_ed_t *sed = opipe->sed; @@ -2276,10 +2278,29 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) if (xfer->device->bus->intr_context || !curproc) panic("ohci_abort_xfer: not in process context"); + /* + * If an abort is already in progress then just wait for it to + * complete and return. + */ + if (oxfer->ohci_xfer_flags & OHCI_XFER_ABORTING) { + DPRINTFN(2, ("ohci_abort_xfer: already aborting\n")); + /* No need to wait if we're aborting from a timeout. */ + if (status == USBD_TIMEOUT) + return; + /* Override the status which might be USBD_TIMEOUT. */ + xfer->status = status; + DPRINTFN(2, ("ohci_abort_xfer: waiting for abort to finish\n")); + oxfer->ohci_xfer_flags |= OHCI_XFER_ABORTWAIT; + while (oxfer->ohci_xfer_flags & OHCI_XFER_ABORTING) + tsleep(&oxfer->ohci_xfer_flags, PZERO, "ohciaw", 0); + return; + } + /* * Step 1: Make interrupt routine and hardware ignore xfer. */ s = splusb(); + oxfer->ohci_xfer_flags |= OHCI_XFER_ABORTING; xfer->status = status; /* make software ignore it */ usb_uncallout(xfer->timeout_handle, ohci_timeout, xfer); usb_rem_task(xfer->pipe->device, &OXFER(xfer)->abort_task); @@ -2314,6 +2335,7 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) p = xfer->hcpriv; #ifdef DIAGNOSTIC if (p == NULL) { + oxfer->ohci_xfer_flags &= ~OHCI_XFER_ABORTING; /* XXX */ splx(s); printf("ohci_abort_xfer: hcpriv is NULL\n"); return; @@ -2350,6 +2372,12 @@ ohci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) /* * Step 5: Execute callback. */ + /* Do the wakeup first to avoid touching the xfer after the callback. */ + oxfer->ohci_xfer_flags &= ~OHCI_XFER_ABORTING; + if (oxfer->ohci_xfer_flags & OHCI_XFER_ABORTWAIT) { + oxfer->ohci_xfer_flags &= ~OHCI_XFER_ABORTWAIT; + wakeup(&oxfer->ohci_xfer_flags); + } usb_transfer_complete(xfer); splx(s); diff --git a/sys/dev/usb/ohcivar.h b/sys/dev/usb/ohcivar.h index 478bcea30dcf..7cdca0af4616 100644 --- a/sys/dev/usb/ohcivar.h +++ b/sys/dev/usb/ohcivar.h @@ -159,6 +159,8 @@ struct ohci_xfer { u_int32_t ohci_xfer_flags; }; #define OHCI_ISOC_DIRTY 0x01 +#define OHCI_XFER_ABORTING 0x02 /* xfer is aborting. */ +#define OHCI_XFER_ABORTWAIT 0x04 /* abort completion is being awaited. */ #define OXFER(xfer) ((struct ohci_xfer *)(xfer)) diff --git a/sys/dev/usb/uhci.c b/sys/dev/usb/uhci.c index 213c6b3d2a67..4340a252340b 100644 --- a/sys/dev/usb/uhci.c +++ b/sys/dev/usb/uhci.c @@ -646,6 +646,7 @@ uhci_allocx(struct usbd_bus *bus) UXFER(xfer)->iinfo.sc = sc; usb_init_task(&UXFER(xfer)->abort_task, uhci_timeout_task, xfer); + UXFER(xfer)->uhci_xfer_flags = 0; #ifdef DIAGNOSTIC UXFER(xfer)->iinfo.isdone = 1; xfer->busy_free = XFER_BUSY; @@ -1933,7 +1934,8 @@ uhci_device_bulk_abort(usbd_xfer_handle xfer) void uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) { - uhci_intr_info_t *ii = &UXFER(xfer)->iinfo; + struct uhci_xfer *uxfer = UXFER(xfer); + uhci_intr_info_t *ii = &uxfer->iinfo; struct uhci_pipe *upipe = (struct uhci_pipe *)xfer->pipe; uhci_softc_t *sc = (uhci_softc_t *)upipe->pipe.device->bus; uhci_soft_td_t *std; @@ -1955,10 +1957,29 @@ uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) if (xfer->device->bus->intr_context || !curproc) panic("uhci_abort_xfer: not in process context"); + /* + * If an abort is already in progress then just wait for it to + * complete and return. + */ + if (uxfer->uhci_xfer_flags & UHCI_XFER_ABORTING) { + DPRINTFN(2, ("uhci_abort_xfer: already aborting\n")); + /* No need to wait if we're aborting from a timeout. */ + if (status == USBD_TIMEOUT) + return; + /* Override the status which might be USBD_TIMEOUT. */ + xfer->status = status; + DPRINTFN(2, ("uhci_abort_xfer: waiting for abort to finish\n")); + uxfer->uhci_xfer_flags |= UHCI_XFER_ABORTWAIT; + while (uxfer->uhci_xfer_flags & UHCI_XFER_ABORTING) + tsleep(&uxfer->uhci_xfer_flags, PZERO, "uhciaw", 0); + return; + } + /* * Step 1: Make interrupt routine and hardware ignore xfer. */ s = splusb(); + uxfer->uhci_xfer_flags |= UHCI_XFER_ABORTING; xfer->status = status; /* make software ignore it */ usb_uncallout(xfer->timeout_handle, uhci_timeout, ii); usb_rem_task(xfer->pipe->device, &UXFER(xfer)->abort_task); @@ -1992,6 +2013,12 @@ uhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status) #ifdef DIAGNOSTIC ii->isdone = 1; #endif + /* Do the wakeup first to avoid touching the xfer after the callback. */ + uxfer->uhci_xfer_flags &= ~UHCI_XFER_ABORTING; + if (uxfer->uhci_xfer_flags & UHCI_XFER_ABORTWAIT) { + uxfer->uhci_xfer_flags &= ~UHCI_XFER_ABORTWAIT; + wakeup(&uxfer->uhci_xfer_flags); + } usb_transfer_complete(xfer); splx(s); } diff --git a/sys/dev/usb/uhcivar.h b/sys/dev/usb/uhcivar.h index 80dba663d05d..3100db756c86 100644 --- a/sys/dev/usb/uhcivar.h +++ b/sys/dev/usb/uhcivar.h @@ -85,8 +85,12 @@ struct uhci_xfer { uhci_intr_info_t iinfo; struct usb_task abort_task; int curframe; + u_int32_t uhci_xfer_flags; }; +#define UHCI_XFER_ABORTING 0x0001 /* xfer is aborting. */ +#define UHCI_XFER_ABORTWAIT 0x0002 /* abort completion is being awaited. */ + #define UXFER(xfer) ((struct uhci_xfer *)(xfer)) /*