[ros-diffs] [janderwald] 51506: [USBEHCI_NEW] - Store number of bytes transferred in the transfer descriptors - Perform queue head completion when the door bell ring has been acknowledged. Fixes race condition...

Author: janderwald
Date: Sat Apr 30 17:44:43 2011
New Revision: 51506

URL: http://svn.reactos.org/svn/reactos?rev=51506&view=rev
Log:
[USBEHCI_NEW]
- Store number of bytes transferred in the transfer descriptors
- Perform queue head completion when the door bell ring has been acknowledged. Fixes race
condition between multiple irps in the async list
- Fix calculation of transfer length when the request is an bulk in operation
- Use EndPointDescriptor member to access transfer type / pid direction
- Use MmGetSystemAddressForMdlSafe to retrieve system address for urb buffer
- Fix check if first transfer buffer finishes on first size if the size is of page_size
- With these changes and little luck and good weather, usb mass storage devices have been
seen to  work in Windows XP SP3 
- Code inspired of mjmartin usbehci driver and Haiku's usb stack


Modified:
    branches/usb-bringup/drivers/usb/usbehci_new/hardware.h
    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/hardware.h
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbehci…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbehci_new/hardware.h [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbehci_new/hardware.h [iso-8859-1] Sat Apr 30
17:44:43 2011
@@ -127,6 +127,7 @@
     //Software
     ULONG PhysicalAddr;
     LIST_ENTRY LinkedDescriptors;
+    ULONG TotalBytesToTransfer;
 } QUEUE_TRANSFER_DESCRIPTOR, *PQUEUE_TRANSFER_DESCRIPTOR;
 
 //

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] Sat Apr 30
17:44:43 2011
@@ -52,6 +52,8 @@
     PDMA_ADAPTER m_Adapter;
     PQUEUE_HEAD AsyncListQueueHead;
     LIST_ENTRY m_CompletedRequestAsyncList;
+    LIST_ENTRY m_PendingRequestAsyncList;
+
 
     // queue head manipulation functions
     VOID LinkQueueHead(PQUEUE_HEAD HeadQueueHead, PQUEUE_HEAD NewQueueHead);
@@ -119,6 +121,11 @@
     // Initialize completed async list head
     //
     InitializeListHead(&m_CompletedRequestAsyncList);
+
+    //
+    // Initialize pending async list head
+    //
+    InitializeListHead(&m_PendingRequestAsyncList);
 
     return Status;
 }
@@ -425,16 +432,154 @@
     NTSTATUS Status)
 {
     IUSBRequest *Request;
+    PQUEUE_HEAD NewQueueHead;
+
+    //
+    // now unlink the queue head
+    // FIXME: implement chained queue heads
+    //
+    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);
+
+}
+
+VOID
+CUSBQueue::ProcessAsyncList(
+    IN NTSTATUS Status,
+    OUT PULONG ShouldRingDoorBell)
+{
+    KIRQL OldLevel;
+    PLIST_ENTRY Entry;
+    PQUEUE_HEAD QueueHead;
+    IUSBRequest * Request;
+    BOOLEAN IsQueueHeadComplete;
+
+    //
+    // lock completed async list
+    //
+    KeAcquireSpinLock(&m_Lock, &OldLevel);
+
+    //
+    // walk async list 
+    //
+    Entry = AsyncListQueueHead->LinkedQueueHeads.Flink;
+
+    while(Entry != &AsyncListQueueHead->LinkedQueueHeads)
+    {
+        //
+        // get queue head structure
+        //
+        QueueHead = (PQUEUE_HEAD)CONTAINING_RECORD(Entry, QUEUE_HEAD, LinkedQueueHeads);
+
+        //
+        // sanity check
+        //
+        PC_ASSERT(QueueHead->Request);
+
+        //
+        // get IUSBRequest interface
+        //
+        Request = (IUSBRequest*)QueueHead->Request;
+
+
+        //
+        // move to next entry
+        //
+        Entry = Entry->Flink;
+
+        //
+        // check if queue head is complete
+        //
+        IsQueueHeadComplete = Request->IsQueueHeadComplete(QueueHead);
+
+        DPRINT1("Request %p QueueHead %p Complete %d\n", Request, QueueHead,
IsQueueHeadComplete);
+
+        //
+        // check if queue head is complete
+        //
+        if (IsQueueHeadComplete)
+        {
+            //
+            // current queue head is complete
+            //
+            QueueHeadCompletion(QueueHead, Status);
+
+            //
+            // ring door bell is going to be necessary
+            //
+            *ShouldRingDoorBell = TRUE;
+        }
+    }
+
+    //
+    // release lock
+    //
+    KeReleaseSpinLock(&m_Lock, OldLevel);
+}
+
+
+VOID
+CUSBQueue::InterruptCallback(
+    IN NTSTATUS Status, 
+    OUT PULONG ShouldRingDoorBell)
+{
+
+    DPRINT1("CUSBQueue::InterruptCallback\n");
+
+    //
+    // iterate asynchronous list
+    //
+    *ShouldRingDoorBell = FALSE;
+    ProcessAsyncList(Status, ShouldRingDoorBell);
+
+    //
+    // TODO: implement periodic schedule processing
+    //
+}
+
+VOID
+CUSBQueue::QueueHeadCleanup(
+    PQUEUE_HEAD CurrentQH)
+{
+    IUSBRequest * Request;
+    BOOLEAN ShouldReleaseWhenDone;
     USBD_STATUS UrbStatus;
-    PQUEUE_HEAD NewQueueHead;
-
-    //
-    // this function is called when a queue head has been completed
+
+    //
+    // sanity checks
     //
     PC_ASSERT(CurrentQH->Token.Bits.Active == 0);
-
-    //
-    // get contained usb request
+    PC_ASSERT(CurrentQH->Request);
+
+
+    //
+    // get request
     //
     Request = (IUSBRequest*)CurrentQH->Request;
 
@@ -481,145 +626,7 @@
     //
     // notify request that a queue head has been completed
     //
-    Request->CompletionCallback(Status, UrbStatus, CurrentQH);
-
-    //
-    // now unlink the queue head
-    // FIXME: implement chained queue heads
-    //
-    UnlinkQueueHead(CurrentQH);
-
-    //
-    // 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
-        //
-        LinkQueueHead(AsyncListQueueHead, NewQueueHead);
-    }
-    else
-    {
-        //
-        // put queue head into completed queue head list
-        //
-        InsertTailList(&m_CompletedRequestAsyncList,
&CurrentQH->LinkedQueueHeads);
-    }
-}
-
-VOID
-CUSBQueue::ProcessAsyncList(
-    IN NTSTATUS Status,
-    OUT PULONG ShouldRingDoorBell)
-{
-    KIRQL OldLevel;
-    PLIST_ENTRY Entry;
-    PQUEUE_HEAD QueueHead;
-    IUSBRequest * Request;
-
-    //
-    // lock completed async list
-    //
-    KeAcquireSpinLock(&m_Lock, &OldLevel);
-
-    //
-    // walk async list 
-    //
-    Entry = AsyncListQueueHead->LinkedQueueHeads.Flink;
-
-    while(Entry != &AsyncListQueueHead->LinkedQueueHeads)
-    {
-        //
-        // get queue head structure
-        //
-        QueueHead = (PQUEUE_HEAD)CONTAINING_RECORD(Entry, QUEUE_HEAD, LinkedQueueHeads);
-
-        //
-        // sanity check
-        //
-        PC_ASSERT(QueueHead->Request);
-
-        //
-        // get IUSBRequest interface
-        //
-        Request = (IUSBRequest*)QueueHead->Request;
-
-
-        //
-        // move to next entry
-        //
-        Entry = Entry->Flink;
-
-        DPRINT1("Request %p QueueHead %p Complete %d\n", Request, QueueHead,
Request->IsQueueHeadComplete(QueueHead));
-
-        //
-        // check if queue head is complete
-        //
-        if (Request->IsQueueHeadComplete(QueueHead))
-        {
-            //
-            // current queue head is complete
-            //
-            QueueHeadCompletion(QueueHead, Status);
-
-            //
-            // ring door bell is going to be necessary
-            //
-            *ShouldRingDoorBell = TRUE;
-        }
-    }
-
-    //
-    // release lock
-    //
-    KeReleaseSpinLock(&m_Lock, OldLevel);
-
-}
-
-
-VOID
-CUSBQueue::InterruptCallback(
-    IN NTSTATUS Status, 
-    OUT PULONG ShouldRingDoorBell)
-{
-
-    DPRINT1("CUSBQueue::InterruptCallback\n");
-
-    //
-    // iterate asynchronous list
-    //
-    *ShouldRingDoorBell = FALSE;
-    ProcessAsyncList(Status, ShouldRingDoorBell);
-
-    //
-    // TODO: implement periodic schedule processing
-    //
-}
-
-VOID
-CUSBQueue::QueueHeadCleanup(
-    PQUEUE_HEAD CurrentQH)
-{
-    IUSBRequest * Request;
-    BOOLEAN ShouldReleaseWhenDone;
-
-    //
-    // sanity checks
-    //
-    PC_ASSERT(CurrentQH->Token.Bits.Active == 0);
-    PC_ASSERT(CurrentQH->Request);
-
-    //
-    // get request
-    //
-    Request = (IUSBRequest*)CurrentQH->Request;
+    Request->CompletionCallback(STATUS_SUCCESS /*FIXME*/, UrbStatus, CurrentQH);
 
     //
     // let IUSBRequest free the queue head
@@ -690,6 +697,27 @@
     }
 
     //
+    // is there a pending async entry
+    //
+    if (!IsListEmpty(&m_PendingRequestAsyncList))
+    {
+        //
+        // remove first entry
+        //
+        Entry = RemoveHeadList(&m_CompletedRequestAsyncList);
+
+        //
+        // get queue head structure
+        //
+        CurrentQH = (PQUEUE_HEAD)CONTAINING_RECORD(Entry, QUEUE_HEAD, LinkedQueueHeads);
+
+        //
+        // Add it to the pending list
+        //
+        LinkQueueHead(AsyncListQueueHead, CurrentQH);
+    }
+
+    //
     // release lock
     //
     KeReleaseSpinLock(&m_Lock, OldLevel);

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] Sat Apr 30
17:44:43 2011
@@ -61,6 +61,7 @@
     UCHAR GetDeviceAddress();
     NTSTATUS BuildSetupPacket();
     NTSTATUS BuildSetupPacketFromURB();
+    ULONG InternalCalculateTransferLength();
 
     // constructor / destructor
     CUSBRequest(IUnknown *OuterUnknown){}
@@ -253,9 +254,14 @@
                 if (!Urb->UrbBulkOrInterruptTransfer.TransferBufferMDL)
                 {
                     //
+                    // sanity check
+                    //
+                    PC_ASSERT(Urb->UrbBulkOrInterruptTransfer.TransferBuffer);
+
+                    //
                     // Create one using TransferBuffer
                     //
-                    DPRINT1("Creating Mdl from Urb Buffer\n");
+                    DPRINT1("Creating Mdl from Urb Buffer %p Length %lu\n",
Urb->UrbBulkOrInterruptTransfer.TransferBuffer,
Urb->UrbBulkOrInterruptTransfer.TransferBufferLength);
                     m_TransferBufferMDL =
IoAllocateMdl(Urb->UrbBulkOrInterruptTransfer.TransferBuffer,
                                                        
Urb->UrbBulkOrInterruptTransfer.TransferBufferLength,
                                                         FALSE,
@@ -370,8 +376,15 @@
             //
             Urb->UrbHeader.Length = 0;
         }
-
-        DPRINT1("Request %p Completing Irp %p NtStatusCode %x UrbStatusCode
%x\n", this, m_Irp, NtStatusCode, UrbStatusCode);
+        else
+        {
+            //
+            // calculate transfer length
+            //
+            Urb->UrbBulkOrInterruptTransfer.TransferBufferLength =
InternalCalculateTransferLength();
+        }
+
+        DPRINT1("Request %p Completing Irp %p NtStatusCode %x UrbStatusCode %x
Transferred Length %lu\n", this, m_Irp, NtStatusCode, UrbStatusCode,
Urb->UrbBulkOrInterruptTransfer.TransferBufferLength);
 
         //
         // FIXME: check if the transfer was split
@@ -525,9 +538,6 @@
 ULONG
 CUSBRequest::InternalGetTransferType()
 {
-    PIO_STACK_LOCATION IoStack;
-    PURB Urb;
-    PUSB_ENDPOINT_DESCRIPTOR EndpointDescriptor;
     ULONG TransferType;
 
     //
@@ -535,38 +545,12 @@
     //
     if (m_Irp)
     {
-        //
-        // get stack location
-        //
-        IoStack = IoGetCurrentIrpStackLocation(m_Irp);
-
-        //
-        // get urb
-        //
-        Urb = (PURB)IoStack->Parameters.Others.Argument1;
-
-        //
-        // check if there is a handle
-        //
-        if (Urb->UrbBulkOrInterruptTransfer.PipeHandle)
-        {
-            //
-            // cast to end point
-            //
-            EndpointDescriptor =
(PUSB_ENDPOINT_DESCRIPTOR)Urb->UrbBulkOrInterruptTransfer.PipeHandle;
-
-            //
-            // end point is defined in the low byte of bmAttributes
-            //
-            TransferType = (EndpointDescriptor->bmAttributes &
USB_ENDPOINT_TYPE_MASK);
-        }
-        else
-        {
-            //
-            // no pipe handle, assume it is a control transfer
-            //
-            TransferType = USB_ENDPOINT_TYPE_CONTROL;
-        }
+        ASSERT(m_EndpointDescriptor);
+
+        //
+        // end point is defined in the low byte of bmAttributes
+        //
+        TransferType = (m_EndpointDescriptor->bmAttributes &
USB_ENDPOINT_TYPE_MASK);
     }
     else
     {
@@ -585,30 +569,13 @@
 UCHAR
 CUSBRequest::InternalGetPidDirection()
 {
-    PIO_STACK_LOCATION IoStack;
-    PURB Urb;
-    PUSB_ENDPOINT_DESCRIPTOR EndpointDescriptor;
-
     ASSERT(m_Irp);
-    //
-    // get stack location
-    //
-    IoStack = IoGetCurrentIrpStackLocation(m_Irp);
-
-    //
-    // get urb
-    //
-    Urb = (PURB)IoStack->Parameters.Others.Argument1;
-
-    //
-    // cast to end point
-    //
-    EndpointDescriptor =
(PUSB_ENDPOINT_DESCRIPTOR)Urb->UrbBulkOrInterruptTransfer.PipeHandle;
+    ASSERT(m_EndpointDescriptor);
 
     //
     // end point is defined in the low byte of bEndpointAddress
     //
-    return (EndpointDescriptor->bEndpointAddress & USB_ENDPOINT_DIRECTION_MASK)
>> 7;
+    return (m_EndpointDescriptor->bEndpointAddress & USB_ENDPOINT_DIRECTION_MASK)
>> 7;
 }
 

//----------------------------------------------------------------------------------------
@@ -816,16 +783,19 @@
     //
     // get virtual base of mdl
     //
-    Base = MmGetMdlVirtualAddress(m_TransferBufferMDL);
+    Base = MmGetSystemAddressForMdlSafe(m_TransferBufferMDL, NormalPagePriority);
     BytesAvailable = m_TransferBufferLength;
 
     PC_ASSERT(m_EndpointDescriptor);
+    PC_ASSERT(Base);
+
 
     DPRINT1("EndPointAddress %x\n",
m_EndpointDescriptor->bEndpointAddress);
     DPRINT1("EndPointDirection %x\n",
USB_ENDPOINT_DIRECTION_IN(m_EndpointDescriptor->bEndpointAddress));
 
-    DPRINT1("Request %p Base Address %p TransferBytesLength %lu\n", this, Base,
BytesAvailable);
+    DPRINT1("Request %p Base Address %p TransferBytesLength %lu MDL %p\n",
this, Base, BytesAvailable, m_TransferBufferMDL);
     DPRINT1("InternalGetPidDirection() %d EndPointAddress %x\n",
InternalGetPidDirection(), m_EndpointDescriptor->bEndpointAddress & 0x0F);
+    DPRINT1("Irp %p QueueHead %p\n", m_Irp, QueueHead);
 
     //PC_ASSERT(InternalGetPidDirection() ==
USB_ENDPOINT_DIRECTION_IN(m_EndpointDescriptor->bEndpointAddress));
 
@@ -881,7 +851,7 @@
                 //
                 // check if request fills another page
                 //
-                if (PageOffset + BytesAvailable >= PAGE_SIZE)
+                if (PageOffset + BytesAvailable > PAGE_SIZE)
                 {
                     //
                     // move to next page
@@ -977,6 +947,11 @@
         }
 
         //
+        // store transfer bytes of descriptor
+        //
+        m_TransferDescriptors[Index]->TotalBytesToTransfer =
m_TransferDescriptors[Index]->Token.Bits.TotalBytesToTransfer;
+
+        //
         // Go ahead and link descriptors
         //
         if (Index > 0)
@@ -1579,6 +1554,36 @@
     *OutMDL = m_TransferBufferMDL;
     *TransferLength = m_TransferBufferLength;
 }
+//-----------------------------------------------------------------------------------------
+ULONG
+CUSBRequest::InternalCalculateTransferLength()
+{
+    if (!m_Irp)
+    {
+        //
+        // FIXME: get length for control request
+        //
+        return m_TransferBufferLength;
+    }
+
+    //
+    // sanity check
+    //
+    ASSERT(m_EndpointDescriptor);
+
+    if (USB_ENDPOINT_DIRECTION_IN(m_EndpointDescriptor->bEndpointAddress))
+    {
+        //
+        // bulk in request
+        //
+        return m_TransferDescriptors[0]->TotalBytesToTransfer -
m_TransferDescriptors[0]->Token.Bits.TotalBytesToTransfer;
+    }
+
+    //
+    // bulk out transfer
+    //
+    return m_TransferBufferLength;
+}
 

//-----------------------------------------------------------------------------------------
 NTSTATUS