* Fixed memory leak of Transfers never getting deleted (intentionally until now).

* Reduced the Transfer class to a minimum and removed support for synchronous transfers. The only usage of this is in SendRequest(), which now provides a callback and QueueRequest()s it's request instead.
* Clarified semantics of SubmitTransfer(). It returns a status_t indicating wether the transfer was submitted successfully. This includes no information about the outcome. Therefore the UHCI RootHub now reports the status only through the callback and returns B_OK or B_ERROR for SubmitTransfer().
* Added comment to the Transfer class explaining who takes ownership of the Transfer in which cases.

git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@18511 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/add-ons/kernel/bus_managers/usb/Pipe.cpp

@@ -125,7 +125,7 @@ status_t
 InterruptPipe::QueueInterrupt(void *data, size_t dataLength,
 	usb_callback_func callback, void *callbackCookie)
 {
-	Transfer *transfer = new(std::nothrow) Transfer(this, false);
+	Transfer *transfer = new(std::nothrow) Transfer(this);
 	if (!transfer)
 		return B_NO_MEMORY;
 
@@ -133,10 +133,8 @@ InterruptPipe::QueueInterrupt(void *data, size_t dataLength,
 	transfer->SetCallback(callback, callbackCookie);
 
 	status_t result = SubmitTransfer(transfer);
-	if (result == B_OK || result == EINPROGRESS)
-		return B_OK;
-
-	delete transfer;
+	if (result < B_OK)
+		delete transfer;
 	return result;
 }
 
@@ -157,7 +155,7 @@ status_t
 BulkPipe::QueueBulk(void *data, size_t dataLength, usb_callback_func callback,
 	void *callbackCookie)
 {
-	Transfer *transfer = new(std::nothrow) Transfer(this, false);
+	Transfer *transfer = new(std::nothrow) Transfer(this);
 	if (!transfer)
 		return B_NO_MEMORY;
 
@@ -165,10 +163,8 @@ BulkPipe::QueueBulk(void *data, size_t dataLength, usb_callback_func callback,
 	transfer->SetCallback(callback, callbackCookie);
 
 	status_t result = SubmitTransfer(transfer);
-	if (result == B_OK || result == EINPROGRESS)
-		return B_OK;
-
-	delete transfer;
+	if (result < B_OK)
+		delete transfer;
 	return result;
 }
 
@@ -199,6 +195,13 @@ IsochronousPipe::QueueIsochronous(void *data, size_t dataLength,
 //
 
 
+struct transfer_result_data {
+	sem_id	notify_sem;
+	uint32	status;
+	size_t	actual_length;
+};
+
+
 ControlPipe::ControlPipe(Device *device, pipeSpeed speed,
 	uint8 endpointAddress, uint32 maxPacketSize)
 	:	Pipe(device, Pipe::Default, speed, endpointAddress, maxPacketSize)
@@ -230,28 +233,41 @@ ControlPipe::SendRequest(uint8 requestType, uint8 request, uint16 value,
 	uint16 index, uint16 length, void *data, size_t dataLength,
 	size_t *actualLength)
 {
-	usb_request_data requestData;
-	requestData.RequestType = requestType;
-	requestData.Request = request;
-	requestData.Value = value;
-	requestData.Index = index;
-	requestData.Length = length;
+	transfer_result_data transferResult;
+	transferResult.notify_sem = create_sem(0, "Send Request Notify Sem");
+	if (transferResult.notify_sem < B_OK)
+		return B_NO_MORE_SEMS;
 
-	Transfer *transfer = new(std::nothrow) Transfer(this, true);
-	if (!transfer)
-		return B_NO_MEMORY;
+	status_t result = QueueRequest(requestType, request, value, index, length,
+		data, dataLength, SendRequestCallback, &transferResult);
+	if (result < B_OK) {
+		delete_sem(transferResult.notify_sem);
+		return result;
+	}
 
-	transfer->SetRequestData(&requestData);
-	transfer->SetData((uint8 *)data, dataLength);
-	transfer->SetActualLength(actualLength);
+	// the sem will be released in the callback after
+	// the result data was filled into the provided struct
+	acquire_sem(transferResult.notify_sem);
+	delete_sem(transferResult.notify_sem);
 
-	status_t result = SubmitTransfer(transfer);
-	if (result == EINPROGRESS)
-		return transfer->WaitForFinish();
+	if (actualLength)
+		*actualLength = transferResult.actual_length;
 
-	if (result < B_OK)
-		delete transfer;
-	return result;
+	if (transferResult.status == B_USB_STATUS_SUCCESS)
+		return B_OK;
+
+	return B_ERROR;
+}
+
+
+void
+ControlPipe::SendRequestCallback(void *cookie, uint32 status, void *data,
+	size_t actualLength)
+{
+	transfer_result_data *transferResult = (transfer_result_data *)cookie;
+	transferResult->status = status;
+	transferResult->actual_length = actualLength;
+	release_sem(transferResult->notify_sem);
 }
 
 
@@ -270,7 +286,7 @@ ControlPipe::QueueRequest(uint8 requestType, uint8 request, uint16 value,
 	requestData->Index = index;
 	requestData->Length = length;
 
-	Transfer *transfer = new(std::nothrow) Transfer(this, false);
+	Transfer *transfer = new(std::nothrow) Transfer(this);
 	if (!transfer) {
 		delete requestData;
 		return B_NO_MEMORY;
@@ -281,9 +297,7 @@ ControlPipe::QueueRequest(uint8 requestType, uint8 request, uint16 value,
 	transfer->SetCallback(callback, callbackCookie);
 
 	status_t result = SubmitTransfer(transfer);
-	if (result == B_OK || result == EINPROGRESS)
-		return B_OK;
-
-	delete transfer;
+	if (result < B_OK)
+		delete transfer;
 	return result;
 }

src/add-ons/kernel/bus_managers/usb/Transfer.cpp

@@ -9,30 +9,22 @@
 #include "usb_p.h"
 
 
-Transfer::Transfer(Pipe *pipe, bool synchronous)
+Transfer::Transfer(Pipe *pipe)
 	:	fPipe(pipe),
 		fData(NULL),
 		fDataLength(0),
-		fActualLengthPointer(NULL),
-		fActualLength(0),
-		fStatus(B_USB_STATUS_DRIVER_INTERNAL_ERROR),
 		fCallback(NULL),
 		fCallbackCookie(NULL),
-		fSem(-1),
-		fHostPrivate(NULL),
 		fRequestData(NULL)
 {
-	if (synchronous) {
-		fSem = create_sem(0, "USB Transfer");
-		set_sem_owner(fSem, B_SYSTEM_TEAM);
-	}
 }
 
 
 Transfer::~Transfer()
 {
-	if (fSem >= B_OK)
-		delete_sem(fSem);
+	// we take ownership of the request data
+	if (fRequestData)
+		delete fRequestData;
 }
 
 
@@ -51,13 +43,6 @@ Transfer::SetData(uint8 *data, size_t dataLength)
 }
 
 
-void
-Transfer::SetActualLength(size_t *actualLength)
-{
-	fActualLengthPointer = actualLength;
-}
-
-
 void
 Transfer::SetCallback(usb_callback_func callback, void *cookie)
 {
@@ -66,47 +51,9 @@ Transfer::SetCallback(usb_callback_func callback, void *cookie)
 }
 
 
-void
-Transfer::SetHostPrivate(hostcontroller_priv *priv)
-{
-	fHostPrivate = priv;
-}
-
-
-status_t
-Transfer::WaitForFinish()
-{
-	if (fSem < B_OK)
-		return fSem;
-
-	status_t result = acquire_sem(fSem);
-	if (result < B_OK)
-		return result;
-
-	if (fStatus == B_USB_STATUS_SUCCESS)
-		return B_OK;
-
-	return B_ERROR;
-}
-
-
 void
 Transfer::Finished(uint32 status, size_t actualLength)
 {
-	fStatus = status;
-	fActualLength = actualLength;
-	if (fActualLengthPointer)
-		*fActualLengthPointer = actualLength;
-
-	// Call the callback function ...
-	if (fCallback) {
-		fCallback(fCallbackCookie, fStatus, fData, fActualLength);
-		return;
-	}
-
-	// ... or release the sem
-	if (fSem > B_OK) {
-		release_sem(fSem);
-		return;
-	}
+	if (fCallback)
+		fCallback(fCallbackCookie, status, fData, actualLength);
 }

src/add-ons/kernel/bus_managers/usb/usb_p.h

@@ -266,6 +266,9 @@ virtual	void							SetDataToggle(bool toggle) {};
 											uint16 index, uint16 length,
 											void *data, size_t dataLength,
 											size_t *actualLength);
+static	void							SendRequestCallback(void *cookie,
+											uint32 status, void *data,
+											size_t actualLength);
 
 		status_t						QueueRequest(uint8 requestType,
 											uint8 request, uint16 value,
@@ -382,20 +385,19 @@ private:
 
 
 /*
- * This is a forward definition of a struct that is defined in the individual
- * host controller modules
- */
-struct hostcontroller_priv;
-
-
-/*
- * This class is more like an utility class that performs all functions on
- * packets. The class is only used in the bus_manager: the host controllers
- * receive the internal data structures.
+ * A Transfer is allocated on the heap and passed to the Host Controller in
+ * SubmitTransfer(). It is generated for all queued transfers. If queuing
+ * succeds (SubmitTransfer() returns with >= B_OK) the Host Controller takes
+ * ownership of the Transfer and will delete it as soon as it has called the
+ * set callback function. If SubmitTransfer() failes, the calling function is
+ * responsible for deleting the Transfer.
+ * Also, the transfer takes ownership of the usb_request_data passed to it in
+ * SetRequestData(), but does not take ownership of the data buffer set by
+ * SetData().
  */
 class Transfer {
 public:
-									Transfer(Pipe *pipe, bool synchronous);
+									Transfer(Pipe *pipe);
 									~Transfer();
 
 		Pipe						*TransferPipe() { return fPipe; };
@@ -407,16 +409,9 @@ public:
 		uint8						*Data() { return fData; };
 		size_t						DataLength() { return fDataLength; };
 
-		void						SetActualLength(size_t *actualLength);
-		size_t						*ActualLength() { return fActualLengthPointer; };
-
-		void						SetHostPrivate(hostcontroller_priv *priv);
-		hostcontroller_priv			*HostPrivate() { return fHostPrivate; };
-
 		void						SetCallback(usb_callback_func callback,
 										void *cookie);
 
-		status_t					WaitForFinish();
 		void						Finished(uint32 status, size_t actualLength);
 
 private:
@@ -424,16 +419,9 @@ private:
 		Pipe						*fPipe;
 		uint8						*fData;
 		size_t						fDataLength;
-		size_t						*fActualLengthPointer;
-		size_t						fActualLength;
-		uint32						fStatus;
-
 		usb_callback_func			fCallback;
 		void						*fCallbackCookie;
 
-		sem_id						fSem;
-		hostcontroller_priv			*fHostPrivate;
-
 		// For control transfers
 		usb_request_data			*fRequestData;
 };

src/add-ons/kernel/busses/usb/uhci.cpp

@@ -583,7 +583,7 @@ UHCI::SubmitTransfer(Transfer *transfer)
 		return result;
 	}
 
-	return EINPROGRESS;
+	return B_OK;
 }
 
 
@@ -655,7 +655,7 @@ UHCI::SubmitRequest(Transfer *transfer)
 		return result;
 	}
 
-	return EINPROGRESS;
+	return B_OK;
 }
 
 
@@ -804,6 +804,7 @@ UHCI::FinishTransfers()
 						fLastTransfer = lastTransfer;
 
 					transfer_data *next = transfer->link;
+					delete transfer->transfer;
 					delete transfer;
 					transfer = next;
 

src/add-ons/kernel/busses/usb/uhci_rh.cpp

@@ -141,18 +141,20 @@ UHCIRootHub::SubmitTransfer(Transfer *transfer)
 	usb_request_data *request = transfer->RequestData();
 	TRACE(("usb_uhci_roothub: rh_submit_packet called. request: %u\n", request->Request));
 
+	// ToDo: define better status codes. We should return a request error.
+	uint32 status = B_USB_STATUS_DEVICE_TIMEOUT;
 	size_t actualLength = 0;
-	status_t result = B_ERROR;
 	switch (request->Request) {
 		case RH_GET_STATUS: {
 			if (request->Index == 0) {
 				// Get the hub status -- everything as 0 means all-right
-				memset(transfer->Data(), 0, sizeof(get_status_buffer));
-				result = B_OK;
+				actualLength = MIN(sizeof(get_status_buffer),
+					transfer->DataLength());
+				memset(transfer->Data(), 0, actualLength);
+				status = B_USB_STATUS_SUCCESS;
 				break;
 			} else if (request->Index > sUHCIRootHubConfig.hub.num_ports) {
 				// This port doesn't exist
-				result = EINVAL;
 				break;
 			}
 
@@ -162,18 +164,18 @@ UHCIRootHub::SubmitTransfer(Transfer *transfer)
 			actualLength = MIN(4, transfer->DataLength());
 			memcpy(transfer->Data(),
 				(void *)&fPortStatus[request->Index - 1], actualLength);
-			result = B_OK;
+			status = B_USB_STATUS_SUCCESS;
 			break;
 		}
 
 		case RH_SET_ADDRESS:
 			if (request->Value >= 128) {
-				result = EINVAL;
+				status = B_USB_STATUS_DEVICE_TIMEOUT;
 				break;
 			}
 
 			TRACE(("usb_uhci_roothub: rh_submit_packet RH_ADDRESS: %d\n", request->Value));
-			result = B_OK;
+			status = B_USB_STATUS_SUCCESS;
 			break;
 
 		case RH_GET_DESCRIPTOR:
@@ -185,7 +187,7 @@ UHCIRootHub::SubmitTransfer(Transfer *transfer)
 						transfer->DataLength());
 					memcpy(transfer->Data(), (void *)&sUHCIRootHubDevice,
 						actualLength);
-					result = B_OK;
+					status = B_USB_STATUS_SUCCESS;
 					break;
 				}
 
@@ -194,22 +196,20 @@ UHCIRootHub::SubmitTransfer(Transfer *transfer)
 						transfer->DataLength());
 					memcpy(transfer->Data(), (void *)&sUHCIRootHubConfig,
 						actualLength);
-					result =  B_OK;
+					status = B_USB_STATUS_SUCCESS;
 					break;
 				}
 
 				case RH_STRING_DESCRIPTOR: {
 					uint8 index = request->Value & 0x00ff;
-					if (index > 2) {
-						result = EINVAL;
+					if (index > 2)
 						break;
-					}
 
 					actualLength = MIN(sUHCIRootHubStrings[index].length,
 						transfer->DataLength());
 					memcpy(transfer->Data(), (void *)&sUHCIRootHubStrings[index],
 						actualLength);
-					result = B_OK;
+					status = B_USB_STATUS_SUCCESS;
 					break;
 				}
 
@@ -218,48 +218,41 @@ UHCIRootHub::SubmitTransfer(Transfer *transfer)
 						transfer->DataLength());
 					memcpy(transfer->Data(), (void *)&sUHCIRootHubConfig.hub,
 						actualLength);
-					result = B_OK;
+					status = B_USB_STATUS_SUCCESS;
 					break;
 				}
-
-				default:
-					result = EINVAL;
-					break;
 			}
 			break;
 
 		case RH_SET_CONFIG:
-			result = B_OK;
+			status = B_USB_STATUS_SUCCESS;
 			break;
 
 		case RH_CLEAR_FEATURE: {
 			if (request->Index == 0) {
 				// We don't support any hub changes
 				TRACE(("usb_uhci_roothub: RH_CLEAR_FEATURE no hub changes!\n"));
-				result = EINVAL;
 				break;
 			} else if (request->Index > sUHCIRootHubConfig.hub.num_ports) {
 				// Invalid port number
 				TRACE(("usb_uhci_roothub: RH_CLEAR_FEATURE invalid port!\n"));
-				result = EINVAL;
 				break;
 			}
 
 			TRACE(("usb_uhci_roothub: RH_CLEAR_FEATURE called. Feature: %u!\n", request->Value));
-			uint16 status;
+			uint16 portStatus;
 			switch(request->Value) {
 				case C_PORT_RESET:
 					fUHCI->SetPortResetChange(request->Index - 1, false);
-					result = B_OK;
+					status = B_USB_STATUS_SUCCESS;
 					break;
 
 				case C_PORT_CONNECTION:
-					status = fUHCI->PortStatus(request->Index - 1);
-					result = fUHCI->SetPortStatus(request->Index - 1,
-						(status & UHCI_PORTSC_DATAMASK) | UHCI_PORTSC_STATCHA);
-					break;
-				default:
-					result = EINVAL;
+					portStatus = fUHCI->PortStatus(request->Index - 1);
+					if (fUHCI->SetPortStatus(request->Index - 1,
+						(portStatus & UHCI_PORTSC_DATAMASK)
+						| UHCI_PORTSC_STATCHA) >= B_OK)
+						status = B_USB_STATUS_SUCCESS;
 					break;
 			}
 			break;
@@ -269,40 +262,32 @@ UHCIRootHub::SubmitTransfer(Transfer *transfer)
 			if (request->Index == 0) {
 				// We don't support any hub changes
 				TRACE(("usb_uhci_roothub: RH_SET_FEATURE no hub changes!\n"));
-				result = EINVAL;
 				break;
 			} else if (request->Index > sUHCIRootHubConfig.hub.num_ports) {
 				// Invalid port number
 				TRACE(("usb_uhci_roothub: RH_SET_FEATURE invalid port!\n"));
-				result = EINVAL;
 				break;
 			}
 
 			TRACE(("usb_uhci_roothub: RH_SET_FEATURE called. Feature: %u!\n", request->Value));
 			switch(request->Value) {
 				case PORT_RESET:
-					result = fUHCI->ResetPort(request->Index - 1);
+					if (fUHCI->ResetPort(request->Index - 1) >= B_OK)
+						status = B_USB_STATUS_SUCCESS;
 					break;
 
 				case PORT_POWER:
 					// the ports are automatically powered
-					result = B_OK;
-					break;
-
-				default:
-					result = EINVAL;
+					status = B_USB_STATUS_SUCCESS;
 					break;
 			}
 			break;
 		}
-
-		default: 
-			result = EINVAL;
-			break;
 	}
 
-	transfer->Finished(result, actualLength);
-	return result;
+	transfer->Finished(status, actualLength);
+	delete transfer;
+	return B_OK;
 }