[ros-diffs] [mjmartin] 51684: [USBEHCI_NEW] - Modify BuildBulkTransferQueueHead to support TransferBufferLengths larger than PAGE_SIZE * 5. - Acquire a SpinLock before adding QueueHeads to AsyncList and Pendin...

Author: mjmartin
Date: Thu May 12 13:35:06 2011
New Revision: 51684

URL: http://svn.reactos.org/svn/reactos?rev=51684&view=rev
Log:
[USBEHCI_NEW]
- Modify BuildBulkTransferQueueHead to support TransferBufferLengths larger than PAGE_SIZE
* 5.
- Acquire a SpinLock before adding QueueHeads to AsyncList and PendingList.
- Dont request a new QueueHead for incomplete transfers in QueueHeadCompletion, as the
memory for the just completed QueueHead has not been released yet. Doing so overwrites the
m_TransferDescriptor[x] members with new address resulting in memory leaks. Instead
request a new QueueHead after the QueueHead has been freed in QueueHeadCleanup.
- Fix a bug where a QueueHead was removed from the m_CompletedRequestAsyncList instead of
the m_PendingRequestAsyncList.
- Temporary hackfix InternalCalculateTransferLength to return the TransferBufferLength.
This hack will be removed as soon as possible.
- With these changes the hub and ehci driver allow viewing content of and transfers
to/from usb disks.

Modified:
    branches/usb-bringup/drivers/usb/usbehci_new/usb_queue.cpp
    branches/usb-bringup/drivers/usb/usbehci_new/usb_request.cpp

Modified: branches/usb-bringup/drivers/usb/usbehci_new/usb_queue.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbehci…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbehci_new/usb_queue.cpp [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbehci_new/usb_queue.cpp [iso-8859-1] Thu May 12
13:35:06 2011
@@ -137,8 +137,7 @@
     // Loop through the pending list and iterrate one for each QueueHead that
     // has a IRP to complete.
     //
-    
-    
+
     return 0;
 }
 
@@ -149,6 +148,7 @@
     PQUEUE_HEAD QueueHead;
     NTSTATUS Status;
     ULONG Type;
+    KIRQL OldLevel;
 
     //
     // sanity check
@@ -214,7 +214,9 @@
         //
         // Add it to the pending list
         //
+        KeAcquireSpinLock(&m_Lock, &OldLevel);
         LinkQueueHead(AsyncListQueueHead, QueueHead);
+        KeReleaseSpinLock(&m_Lock, OldLevel);
     }
 
 
@@ -251,12 +253,12 @@
 
     *OutRequest = NULL;
     Status = InternalCreateUSBRequest(&UsbRequest);
-    
+
     if (NT_SUCCESS(Status))
     {
         *OutRequest = UsbRequest;
     }
-    
+
     return Status;
 }
 
@@ -432,41 +434,20 @@
     PQUEUE_HEAD CurrentQH,
     NTSTATUS Status)
 {
-    IUSBRequest *Request;
-    PQUEUE_HEAD NewQueueHead;
+    KIRQL OldLevel;
 
     //
     // now unlink the queue head
     // FIXME: implement chained queue heads
     //
+
+    KeAcquireSpinLock(&m_Lock, &OldLevel);
+
     UnlinkQueueHead(CurrentQH);
 
-    //
-    // get contained usb request
-    //
-    Request = (IUSBRequest*)CurrentQH->Request;
-
-    //
-    // check if the request is complete
-    //
-    if (Request->IsRequestComplete() == FALSE)
-    {
-        //
-        // request is still in complete
-        // get new queue head
-        //
-        Status = Request->GetQueueHead(&NewQueueHead);
-
-        //
-        // add to pending list
-        //
-        InsertTailList(&m_PendingRequestAsyncList,
&NewQueueHead->LinkedQueueHeads);
-    }
-
-    //
-    // put queue head into completed queue head list
-    //
     InsertTailList(&m_CompletedRequestAsyncList,
&CurrentQH->LinkedQueueHeads);
+
+    KeReleaseSpinLock(&m_Lock, OldLevel);
 
 }
 
@@ -508,7 +489,6 @@
         //
         Request = (IUSBRequest*)QueueHead->Request;
 
-
         //
         // move to next entry
         //
@@ -568,6 +548,7 @@
 CUSBQueue::QueueHeadCleanup(
     PQUEUE_HEAD CurrentQH)
 {
+    PQUEUE_HEAD NewQueueHead;
     IUSBRequest * Request;
     BOOLEAN ShouldReleaseWhenDone;
     USBD_STATUS UrbStatus;
@@ -625,9 +606,47 @@
     }
 
     //
-    // notify request that a queue head has been completed
-    //
-    Request->CompletionCallback(STATUS_SUCCESS /*FIXME*/, UrbStatus, CurrentQH);
+    // Check if the transfer was completed and if UrbStatus is ok
+    //
+    if ((Request->IsRequestComplete() == FALSE) && (UrbStatus ==
USBD_STATUS_SUCCESS))
+    {
+        //
+        // let IUSBRequest free the queue head
+        //
+        Request->FreeQueueHead(CurrentQH);
+
+        //
+        // request is incomplete, get new queue head
+        //
+        if (Request->GetQueueHead(&NewQueueHead) == STATUS_SUCCESS)
+        {
+            //
+            // add to pending list
+            //
+            InsertTailList(&m_PendingRequestAsyncList,
&NewQueueHead->LinkedQueueHeads);
+
+            //
+            // Done for now
+            //
+            return;
+        }
+        DPRINT1("Unable to create a new QueueHead\n");
+        PC_ASSERT(FALSE);
+
+        //
+        // Else there was a problem
+        // FIXME: Find better return
+        UrbStatus = USBD_STATUS_INSUFFICIENT_RESOURCES;
+    }
+
+    if (UrbStatus != USBD_STATUS_SUCCESS) PC_ASSERT(FALSE);
+
+    //
+    // notify request that a transfer has completed
+    //
+    Request->CompletionCallback(UrbStatus != USBD_STATUS_SUCCESS ? 
STATUS_UNSUCCESSFUL : STATUS_SUCCESS,
+                                UrbStatus,
+                                CurrentQH);
 
     //
     // let IUSBRequest free the queue head
@@ -666,6 +685,7 @@
     KIRQL OldLevel;
     PLIST_ENTRY Entry;
     PQUEUE_HEAD CurrentQH;
+    IUSBRequest *Request;
 
     DPRINT("CUSBQueue::CompleteAsyncRequests\n");
 
@@ -692,6 +712,11 @@
         CurrentQH = (PQUEUE_HEAD)CONTAINING_RECORD(Entry, QUEUE_HEAD, LinkedQueueHeads);
 
         //
+        // Get the Request for this QueueHead
+        //
+        Request = (IUSBRequest*) CurrentQH->Request;
+
+        //
         // complete request now
         //
         QueueHeadCleanup(CurrentQH);
@@ -705,7 +730,7 @@
         //
         // remove first entry
         //
-        Entry = RemoveHeadList(&m_CompletedRequestAsyncList);
+        Entry = RemoveHeadList(&m_PendingRequestAsyncList);
 
         //
         // get queue head structure
@@ -713,7 +738,7 @@
         CurrentQH = (PQUEUE_HEAD)CONTAINING_RECORD(Entry, QUEUE_HEAD, LinkedQueueHeads);
 
         //
-        // Add it to the pending list
+        // Add it to the AsyncList list
         //
         LinkQueueHead(AsyncListQueueHead, CurrentQH);
     }

Modified: branches/usb-bringup/drivers/usb/usbehci_new/usb_request.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbehci…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbehci_new/usb_request.cpp [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbehci_new/usb_request.cpp [iso-8859-1] Thu May 12
13:35:06 2011
@@ -86,6 +86,16 @@
     ULONG m_TransferBufferLength;
 
     //
+    // current transfer length
+    //
+    ULONG m_TransferBufferLengthCompleted;
+
+    //
+    // Total Transfer Length
+    //
+    ULONG m_TotalBytesTransferred;
+
+    //
     // transfer buffer MDL
     //
     PMDL m_TransferBufferMDL;
@@ -169,6 +179,12 @@
     m_TransferBufferMDL = TransferBuffer;
     m_DeviceAddress = DeviceAddress;
     m_EndpointDescriptor = EndpointDescriptor;
+    m_TotalBytesTransferred = 0;
+
+    //
+    // Set Length Completed to 0
+    //
+    m_TransferBufferLengthCompleted = 0;
 
     //
     // allocate completion event
@@ -208,6 +224,7 @@
     PC_ASSERT(Irp);
 
     m_DmaManager = DmaManager;
+    m_TotalBytesTransferred = 0;
 
     //
     // get current irp stack location
@@ -244,7 +261,7 @@
         case URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER:
         {
             //
-            // bulk / interrupt transfer
+            // bulk interrupt transfer
             //
             if (Urb->UrbBulkOrInterruptTransfer.TransferBufferLength)
             {
@@ -281,6 +298,10 @@
                     // FIXME: Does hub driver already do this when passing MDL?
                     //
                     MmBuildMdlForNonPagedPool(m_TransferBufferMDL);
+
+                    //
+                    // Keep that ehci created the MDL and needs to free it.
+                    //
                 }
                 else
                 {
@@ -291,6 +312,11 @@
                 // save buffer length
                 //
                 m_TransferBufferLength =
Urb->UrbBulkOrInterruptTransfer.TransferBufferLength;
+
+                //
+                // Set Length Completed to 0
+                //
+                m_TransferBufferLengthCompleted = 0;
 
                 //
                 // get endpoint descriptor
@@ -357,7 +383,6 @@
         //
         // Check if the MDL was created
         //
-
         if (!Urb->UrbBulkOrInterruptTransfer.TransferBufferMDL)
         {
             //
@@ -439,6 +464,7 @@
         //
         // store urb status
         //
+        DPRINT1("Request Cancelled\n");
         Urb->UrbHeader.Status = USBD_STATUS_CANCELED;
         Urb->UrbHeader.Length = 0;
 
@@ -521,6 +547,18 @@
     //
     // FIXME: check if request was split
     //
+
+    //
+    // Check if the transfer was completed, only valid for Bulk Transfers
+    //
+    if ((m_TransferBufferLengthCompleted < m_TransferBufferLength)
+        && (GetTransferType() == USB_ENDPOINT_TYPE_BULK))
+    {
+        //
+        // Transfer not completed
+        //
+        return FALSE;
+    }
     return TRUE;
 }

//----------------------------------------------------------------------------------------
@@ -642,7 +680,7 @@
     // now initialize the queue head
     //
     QueueHead->EndPointCharacteristics.DeviceAddress = GetDeviceAddress();
-    
+
     if (m_EndpointDescriptor)
     {
         //
@@ -751,7 +789,7 @@
     ULONG TransferDescriptorCount, Index;
     ULONG BytesAvailable, BufferIndex;
     PVOID Base;
-    ULONG PageOffset;
+    ULONG PageOffset, CurrentTransferBufferLength;
 
     //
     // Allocate the queue head
@@ -770,24 +808,59 @@
     // sanity checks
     //
     PC_ASSERT(QueueHead);
-
-    //
-    // FIXME: support more than one descriptor
-    //
-    PC_ASSERT(m_TransferBufferLength < PAGE_SIZE * 5);
     PC_ASSERT(m_TransferBufferLength);
 
-    TransferDescriptorCount = 1;
+    //
+    // Max default of 3 descriptors
+    //
+    TransferDescriptorCount = 3;
 
     //
     // get virtual base of mdl
     //
     Base = MmGetSystemAddressForMdlSafe(m_TransferBufferMDL, NormalPagePriority);
-    BytesAvailable = m_TransferBufferLength;
+
+    //
+    // Increase the size of last transfer, 0 in case this is the first
+    //
+    Base = (PVOID)((ULONG_PTR)Base + m_TransferBufferLengthCompleted);
 
     PC_ASSERT(m_EndpointDescriptor);
     PC_ASSERT(Base);
 
+    //
+    // Get the offset from page size
+    //
+    PageOffset = BYTE_OFFSET(Base);
+
+    //
+    // PageOffset should only be > 0 if this is the  first transfer for this requests
+    //
+    if ((PageOffset != 0) && (m_TransferBufferLengthCompleted != 0))
+    {
+        ASSERT(FALSE);
+    }
+
+    //
+    // Calculate the size of this transfer
+    //
+    if ((PageOffset != 0) && ((m_TransferBufferLength -
m_TransferBufferLengthCompleted) >= (PAGE_SIZE * 4) + PageOffset))
+    {
+        CurrentTransferBufferLength = (PAGE_SIZE * 4) + PageOffset;
+    }
+    else if ((m_TransferBufferLength - m_TransferBufferLengthCompleted) >= PAGE_SIZE *
5)
+    {
+        CurrentTransferBufferLength = PAGE_SIZE * 5;
+    }
+    else
+        CurrentTransferBufferLength = (m_TransferBufferLength -
m_TransferBufferLengthCompleted);
+
+    //
+    // Add current transfer length to transfer length completed
+    //
+    m_TransferBufferLengthCompleted += CurrentTransferBufferLength;
+    BytesAvailable = CurrentTransferBufferLength;
+    DPRINT("CurrentTransferBufferLength %x, m_TransferBufferLengthCompleted
%x\n", CurrentTransferBufferLength, m_TransferBufferLengthCompleted);
 
     DPRINT("EndPointAddress %x\n", m_EndpointDescriptor->bEndpointAddress);
     DPRINT("EndPointDirection %x\n",
USB_ENDPOINT_DIRECTION_IN(m_EndpointDescriptor->bEndpointAddress));
@@ -833,9 +906,9 @@
         for(BufferIndex = 0; BufferIndex < 5; BufferIndex++)
         {
             //
-            // setup buffer
-            //
-            if (BufferIndex == 0)
+            // If this is the first buffer of the first descriptor and there is a
PageOffset
+            //
+            if ((BufferIndex == 0) && (PageOffset != 0) && (Index == 0))
             {
                 //
                 // use physical address
@@ -843,45 +916,25 @@
                 m_TransferDescriptors[Index]->BufferPointer[0] =
MmGetPhysicalAddress(Base).LowPart;
 
                 //
-                // get offset within page
-                //
-                PageOffset =
BYTE_OFFSET(m_TransferDescriptors[Index]->BufferPointer[0]);
-
-                //
-                // check if request fills another page
-                //
-                if (PageOffset + BytesAvailable > PAGE_SIZE)
-                {
-                    //
-                    // move to next page
-                    //
-                    Base = (PVOID)ROUND_TO_PAGES(Base);
-
-                    //
-                    // increment transfer bytes
-                    //
+                // move to next page
+                //
+                Base = (PVOID)ROUND_TO_PAGES(Base);
+
+                //
+                // increment transfer bytes
+                //
+                if (CurrentTransferBufferLength > PAGE_SIZE - PageOffset)
                     m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer =
PAGE_SIZE - PageOffset;
-
-                    //
-                    // decrement available byte count
-                    //
-                    BytesAvailable -=
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer;
-
-                    DPRINT("TransferDescriptor %p BufferPointer %p BufferIndex %lu
TotalBytes %lu Remaining %lu\n", m_TransferDescriptors[Index],
m_TransferDescriptors[Index]->BufferPointer[BufferIndex],
-                        BufferIndex,
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer, BytesAvailable);
-               }
-               else
-               {
-                   //
-                   // request ends on the first buffer page
-                   //
-                   m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer =
BytesAvailable;
-                   BytesAvailable = 0;
-
-                   DPRINT("TransferDescriptor %p BufferPointer %p BufferIndex %lu
TotalBytes %lu Remaining %lu\n", m_TransferDescriptors[Index],
m_TransferDescriptors[Index]->BufferPointer[BufferIndex],
-                       BufferIndex,
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer, BytesAvailable);
-                   break;
-               }
+                else
+                    m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer =
CurrentTransferBufferLength;
+
+                //
+                // decrement available byte count
+                //
+                BytesAvailable -=
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer;
+
+                DPRINT("TransferDescriptor %p BufferPointer %p BufferIndex %lu
TotalBytes %lu Remaining %lu\n", m_TransferDescriptors[Index],
m_TransferDescriptors[Index]->BufferPointer[BufferIndex],
+                    BufferIndex,
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer, BytesAvailable);
             }
             else
             {
@@ -935,7 +988,7 @@
                     BytesAvailable -= BytesAvailable;
 
                     //
-                    // done
+                    // done as this is the last partial or full page
                     //
                     DPRINT("TransferDescriptor %p BufferPointer %p BufferIndex %lu
TotalBytes %lu Remaining %lu\n", m_TransferDescriptors[Index],
m_TransferDescriptors[Index]->BufferPointer[BufferIndex],
                             BufferIndex,
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer, BytesAvailable);
@@ -943,6 +996,12 @@
                     break;
                 }
             }
+
+            //
+            // Check if all bytes have been consumed
+            //
+            if (BytesAvailable == 0)
+                break;
         }
 
         //
@@ -971,6 +1030,12 @@
         //
         // FIXME need dead queue transfer descriptor?
         //
+
+        //
+        // Check if all bytes have been consumed
+        //
+        if (BytesAvailable == 0)
+            break;
     }
 
     //
@@ -982,7 +1047,7 @@
     // Initialize the QueueHead
     //
     QueueHead->EndPointCharacteristics.DeviceAddress = GetDeviceAddress();
-    
+
     if (m_EndpointDescriptor)
     {
         //
@@ -1440,10 +1505,12 @@
 CUSBRequest::FreeQueueHead(
     IN struct _QUEUE_HEAD * QueueHead)
 {
+    LONG DescriptorCount;
+
     //
     // FIXME: support chained queue heads
     //
-    PC_ASSERT(QueueHead == m_QueueHead);
+    //PC_ASSERT(QueueHead == m_QueueHead);
 
     //
     // release queue head
@@ -1458,32 +1525,40 @@
     //
     // release transfer descriptors
     //
-
-    if (m_TransferDescriptors[0])
-    {
-        //
-        // release transfer descriptors
-        //
-        m_DmaManager->Release(m_TransferDescriptors[0],
sizeof(QUEUE_TRANSFER_DESCRIPTOR));
-        m_TransferDescriptors[0] = 0;
-    }
-
-    if (m_TransferDescriptors[1])
-    {
-        //
-        // release transfer descriptors
-        //
-        m_DmaManager->Release(m_TransferDescriptors[1],
sizeof(QUEUE_TRANSFER_DESCRIPTOR));
-        m_TransferDescriptors[1] = 0;
-    }
-
-    if (m_TransferDescriptors[2])
-    {
-        //
-        // release transfer descriptors
-        //
-        m_DmaManager->Release(m_TransferDescriptors[2],
sizeof(QUEUE_TRANSFER_DESCRIPTOR));
-        m_TransferDescriptors[2] = 0;
+    for (DescriptorCount = 0; DescriptorCount < 3; DescriptorCount++)
+    {
+        if (m_TransferDescriptors[DescriptorCount])
+        {
+            //
+            // Calculate Total Bytes Transferred
+            // FIXME: Is this the correct method of determine bytes transferred?
+            //
+            if (USB_ENDPOINT_TYPE_BULK == GetTransferType())
+            {
+                //
+                // sanity check
+                //
+                ASSERT(m_EndpointDescriptor);
+
+                if
(USB_ENDPOINT_DIRECTION_IN(m_EndpointDescriptor->bEndpointAddress))
+                {
+                    DPRINT1("m_TotalBytesTransferred %x, %x - %x\n",
+                        m_TotalBytesTransferred,
+                        m_TransferDescriptors[DescriptorCount]->TotalBytesToTransfer,
+                       
m_TransferDescriptors[DescriptorCount]->Token.Bits.TotalBytesToTransfer);
+
+                    m_TotalBytesTransferred +=
+                        m_TransferDescriptors[DescriptorCount]->TotalBytesToTransfer
-
+                       
m_TransferDescriptors[DescriptorCount]->Token.Bits.TotalBytesToTransfer;
+                }
+            }
+
+            //
+            // release transfer descriptors
+            //
+            m_DmaManager->Release(m_TransferDescriptors[DescriptorCount],
sizeof(QUEUE_TRANSFER_DESCRIPTOR));
+            m_TransferDescriptors[DescriptorCount] = 0;
+        }
     }
 
     if (m_DescriptorPacket)
@@ -1573,8 +1648,9 @@
     {
         //
         // bulk in request
-        //
-        return m_TransferDescriptors[0]->TotalBytesToTransfer -
m_TransferDescriptors[0]->Token.Bits.TotalBytesToTransfer;
+        // HACK: Properly determine transfer length
+        //
+        return m_TransferBufferLength;//m_TotalBytesTransferred;
     }
 
     //