From 1dd6c50cb3cdbcfe254d9b60f04bbfad100e6848 Mon Sep 17 00:00:00 2001 From: Michael Lotz Date: Mon, 14 Sep 2009 06:26:54 +0000 Subject: [PATCH] When submitting transfers there is a race condition between adding the pending transfer to the transfer list and scheduling the transfer descriptors on the controller by switching the endpoint tail. Since we only check that the endpoint head is equal to the endpoint tail to conclude that there are no active transfers, we need to ensure that we don't accidently do that check before the transfer descriptors are scheduled. Otherwise we could happen to processed a not yet started transfer and finishing it before an actual transfer has taken place. This would then lead to 0 byte transfers and toggle mismatches. To fix this we now protect the transfer addition and tail switching as well as the check in the finisher with a per-endpoint mutex. Note that we allocate the lock on the heap and only store the pointer in the endpoint structure as this one is allocated from the precious physical memory pool. Could fix #4067. Also switched the Jamfile to UsePrivateKernelHeaders (for the MutexLocker) and removed BeOS compatibility. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@33129 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- src/add-ons/kernel/busses/usb/Jamfile | 7 +---- src/add-ons/kernel/busses/usb/ohci.cpp | 28 +++++++++++++++++-- src/add-ons/kernel/busses/usb/ohci_hardware.h | 1 + 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/add-ons/kernel/busses/usb/Jamfile b/src/add-ons/kernel/busses/usb/Jamfile index f5cfb5c05e..a80d199ca2 100644 --- a/src/add-ons/kernel/busses/usb/Jamfile +++ b/src/add-ons/kernel/busses/usb/Jamfile @@ -1,14 +1,9 @@ SubDir HAIKU_TOP src add-ons kernel busses usb ; -SetSubDirSupportedPlatformsBeOSCompatible ; - SubDirC++Flags -fno-rtti ; -UsePrivateHeaders [ FDirName kernel ] ; +UsePrivateKernelHeaders ; UseHeaders [ FDirName $(HAIKU_TOP) src add-ons kernel bus_managers usb ] ; -if $(TARGET_PLATFORM) != haiku { - UsePublicHeaders [ FDirName drivers ] ; -} KernelAddon uhci : uhci.cpp diff --git a/src/add-ons/kernel/busses/usb/ohci.cpp b/src/add-ons/kernel/busses/usb/ohci.cpp index a8b4dcb4f2..7a3179effd 100644 --- a/src/add-ons/kernel/busses/usb/ohci.cpp +++ b/src/add-ons/kernel/busses/usb/ohci.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include "ohci.h" @@ -886,9 +887,11 @@ OHCI::_FinishTransfers() while (transfer) { bool transferDone = false; ohci_general_td *descriptor = transfer->first_descriptor; + ohci_endpoint_descriptor *endpoint = transfer->endpoint; status_t callbackStatus = B_OK; - ohci_endpoint_descriptor *endpoint = transfer->endpoint; + MutexLocker endpointLocker(endpoint->lock); + if ((endpoint->head_physical_descriptor & OHCI_ENDPOINT_HEAD_MASK) != endpoint->tail_physical_descriptor) { // there are still active transfers on this endpoint, we need @@ -900,6 +903,8 @@ OHCI::_FinishTransfers() continue; } + endpointLocker.Unlock(); + while (descriptor && !transfer->canceled) { uint32 status = OHCI_TD_GET_CONDITION_CODE(descriptor->flags); if (status == OHCI_TD_CONDITION_NOT_ACCESSED) { @@ -1148,6 +1153,8 @@ OHCI::_SubmitRequest(Transfer *transfer) // Add to the transfer list ohci_endpoint_descriptor *endpoint = (ohci_endpoint_descriptor *)transfer->TransferPipe()->ControllerCookie(); + + MutexLocker endpointLocker(endpoint->lock); result = _AddPendingTransfer(transfer, endpoint, setupDescriptor, dataDescriptor, statusDescriptor, directionIn); if (result < B_OK) { @@ -1158,6 +1165,7 @@ OHCI::_SubmitRequest(Transfer *transfer) // Add the descriptor chain to the endpoint _SwitchEndpointTail(endpoint, setupDescriptor, statusDescriptor); + endpointLocker.Unlock(); // Tell the controller to process the control list endpoint->flags &= ~OHCI_ENDPOINT_SKIP; @@ -1199,6 +1207,8 @@ OHCI::_SubmitTransfer(Transfer *transfer) // Add to the transfer list ohci_endpoint_descriptor *endpoint = (ohci_endpoint_descriptor *)pipe->ControllerCookie(); + + MutexLocker endpointLocker(endpoint->lock); result = _AddPendingTransfer(transfer, endpoint, firstDescriptor, firstDescriptor, lastDescriptor, directionIn); if (result < B_OK) { @@ -1209,8 +1219,9 @@ OHCI::_SubmitTransfer(Transfer *transfer) // Add the descriptor chain to the endpoint _SwitchEndpointTail(endpoint, firstDescriptor, lastDescriptor); - endpoint->flags &= ~OHCI_ENDPOINT_SKIP; + endpointLocker.Unlock(); + endpoint->flags &= ~OHCI_ENDPOINT_SKIP; if (pipe->Type() & USB_OBJECT_BULK_PIPE) { // Tell the controller to process the bulk list _WriteReg(OHCI_COMMAND_STATUS, OHCI_BULK_LIST_FILLED); @@ -1306,13 +1317,22 @@ OHCI::_AllocateEndpoint() ohci_endpoint_descriptor *endpoint; void *physicalAddress; + mutex *lock = (mutex *)malloc(sizeof(mutex)); + if (lock == NULL) { + TRACE_ERROR("no memory to allocate endpoint lock\n"); + return NULL; + } + // Allocate memory chunk if (fStack->AllocateChunk((void **)&endpoint, &physicalAddress, sizeof(ohci_endpoint_descriptor)) < B_OK) { TRACE_ERROR("failed to allocate endpoint descriptor\n"); + free(lock); return NULL; } + mutex_init(lock, "ohci endpoint lock"); + endpoint->flags = OHCI_ENDPOINT_SKIP; endpoint->physical_address = (addr_t)physicalAddress; endpoint->head_physical_descriptor = 0; @@ -1320,6 +1340,7 @@ OHCI::_AllocateEndpoint() endpoint->tail_physical_descriptor = 0; endpoint->next_logical_endpoint = NULL; endpoint->next_physical_endpoint = 0; + endpoint->lock = lock; return endpoint; } @@ -1330,6 +1351,9 @@ OHCI::_FreeEndpoint(ohci_endpoint_descriptor *endpoint) if (!endpoint) return; + mutex_destroy(endpoint->lock); + free(endpoint->lock); + fStack->FreeChunk((void *)endpoint, (void *)endpoint->physical_address, sizeof(ohci_endpoint_descriptor)); } diff --git a/src/add-ons/kernel/busses/usb/ohci_hardware.h b/src/add-ons/kernel/busses/usb/ohci_hardware.h index a9333dfb9e..ac5850fa23 100644 --- a/src/add-ons/kernel/busses/usb/ohci_hardware.h +++ b/src/add-ons/kernel/busses/usb/ohci_hardware.h @@ -304,6 +304,7 @@ typedef struct { addr_t physical_address; // Physical pointer to this address void *tail_logical_descriptor; // Queue tail logical pointer void *next_logical_endpoint; // Logical pointer to the next endpoint + mutex *lock; // Protects tail changes and checks } ohci_endpoint_descriptor; #define OHCI_ENDPOINT_ADDRESS_MASK 0x0000007f