[ros-diffs] [janderwald] 55389: [USBOHCI] - Check if the transfer descriptors reported errors and propagate the error in the urb status field - OHCI now reports error which the class driver are handling - Allo...

Author: janderwald
Date: Fri Feb  3 09:52:29 2012
New Revision: 55389

URL: http://svn.reactos.org/svn/reactos?rev=55389&view=rev
Log:
[USBOHCI]
- Check if the transfer descriptors reported errors and propagate the error in the urb
status field
- OHCI now reports error which the class driver are handling
- Allocate device descriptor from non paged pool to prevent possible alignment problems
- Fix checking of completed transfer descriptors
- Fix double freeing of descriptors
- Cleanup endpoints when the halted bit is set by the host controllers
- Check for the endpoint direction in the data descriptor
- Setup descriptor needs buffer rounding set

Modified:
    branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.cpp
    branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.h
    branches/usb-bringup-trunk/drivers/usb/usbohci/hub_controller.cpp
    branches/usb-bringup-trunk/drivers/usb/usbohci/usb_device.cpp
    branches/usb-bringup-trunk/drivers/usb/usbohci/usb_queue.cpp
    branches/usb-bringup-trunk/drivers/usb/usbohci/usb_request.cpp

Modified: branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.cpp [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.cpp [iso-8859-1] Fri Feb  3
09:52:29 2012
@@ -1457,6 +1457,8 @@
         // the interrupt was not caused by DoneHead update
         // check if something important happened
         //
+        DPRINT1("InterruptStatus %x  InterruptEnable %x\n",
READ_REGISTER_ULONG((PULONG)((PUCHAR)This->m_Base + OHCI_INTERRUPT_STATUS_OFFSET)), 
+                                                           
READ_REGISTER_ULONG((PULONG)((PUCHAR)This->m_Base + OHCI_INTERRUPT_ENABLE_OFFSET)));
         Status = READ_REGISTER_ULONG((PULONG)((PUCHAR)This->m_Base +
OHCI_INTERRUPT_STATUS_OFFSET)) & READ_REGISTER_ULONG((PULONG)((PUCHAR)This->m_Base
+ OHCI_INTERRUPT_ENABLE_OFFSET)) & (~OHCI_WRITEBACK_DONE_HEAD); 
         if (Status == 0)
         {

Modified: branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.h
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.h [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbohci/hardware.h [iso-8859-1] Fri Feb  3
09:52:29 2012
@@ -239,7 +239,8 @@
 #define OHCI_ENDPOINT_DIRECTION_IN              0x00001000
 #define OHCI_ENDPOINT_GENERAL_FORMAT            0x00000000
 #define OHCI_ENDPOINT_ISOCHRONOUS_FORMAT        0x00008000
-
+#define	OHCI_ENDPOINT_HEAD_MASK                 0xfffffffc
+#define	OHCI_ENDPOINT_HALTED					0x00000001
 //
 // Maximum port count set by OHCI
 //

Modified: branches/usb-bringup-trunk/drivers/usb/usbohci/hub_controller.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbohci/hub_controller.cpp [iso-8859-1]
(original)
+++ branches/usb-bringup-trunk/drivers/usb/usbohci/hub_controller.cpp [iso-8859-1] Fri Feb
 3 09:52:29 2012
@@ -1763,8 +1763,13 @@
     //
     // assert on failure
     //
-    PC_ASSERT(NT_SUCCESS(Status));
-
+    if (!NT_SUCCESS(Status))
+    {
+        //
+        // display error
+        //
+        DPRINT1("URB_FUNCTION_CLASS_INTERFACE failed with Urb Status %x\n",
Urb->UrbHeader.Status);
+    }
 
     //
     // done

Modified: branches/usb-bringup-trunk/drivers/usb/usbohci/usb_device.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbohci/usb_device.cpp [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbohci/usb_device.cpp [iso-8859-1] Fri Feb  3
09:52:29 2012
@@ -598,11 +598,24 @@
     USB_DEFAULT_PIPE_SETUP_PACKET CtrlSetup;
     PMDL Mdl;
     NTSTATUS Status;
+    PVOID DeviceDescriptor;
+
+    //
+    // allocate descriptor page aligned
+    //
+    DeviceDescriptor = ExAllocatePool(NonPagedPool, PAGE_SIZE);
+    if (!DeviceDescriptor)
+    {
+        //
+        // no memory
+        //
+        return STATUS_INSUFFICIENT_RESOURCES;
+    }
 
     //
     // zero descriptor
     //
-    RtlZeroMemory(&m_DeviceDescriptor, sizeof(USB_DEVICE_DESCRIPTOR));
+    RtlZeroMemory(DeviceDescriptor, sizeof(USB_DEVICE_DESCRIPTOR));
     RtlZeroMemory(&CtrlSetup, sizeof(USB_DEFAULT_PIPE_SETUP_PACKET));
 
     //
@@ -616,7 +629,7 @@
     //
     // allocate mdl describing the device descriptor
     //
-    Mdl = IoAllocateMdl(&m_DeviceDescriptor, sizeof(USB_DEVICE_DESCRIPTOR), FALSE,
FALSE, 0);
+    Mdl = IoAllocateMdl(DeviceDescriptor, PAGE_SIZE, FALSE, FALSE, 0);
     if (!Mdl)
     {
         //
@@ -643,10 +656,20 @@
     if (NT_SUCCESS(Status))
     {
         //
+        // copy back device descriptor
+        //
+        RtlCopyMemory(&m_DeviceDescriptor, DeviceDescriptor,
sizeof(USB_DEVICE_DESCRIPTOR));
+
+        //
         // informal dbg print
         //
         DumpDeviceDescriptor(&m_DeviceDescriptor);
     }
+
+    //
+    // free item
+    //
+    ExFreePool(DeviceDescriptor);
 
     //
     // done

Modified: branches/usb-bringup-trunk/drivers/usb/usbohci/usb_queue.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbohci/usb_queue.cpp [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbohci/usb_queue.cpp [iso-8859-1] Fri Feb  3
09:52:29 2012
@@ -363,7 +363,7 @@
         //
         // check if the transfer descriptor is inside the list
         //
-        if (IsTransferDescriptorInEndpoint(EndpointDescriptor,
TransferDescriptorLogicalAddress))
+        if ((EndpointDescriptor->HeadPhysicalDescriptor & OHCI_ENDPOINT_HEAD_MASK)
== EndpointDescriptor->TailPhysicalDescriptor ||
(EndpointDescriptor->HeadPhysicalDescriptor & OHCI_ENDPOINT_HALTED))
         {
             //
             // found endpoint
@@ -585,13 +585,6 @@
     Request->FreeEndpointDescriptor(EndpointDescriptor);
 
     //
-    // FIXME: check if complete
-    //
-    //ASSERT(Request->IsRequestComplete());
-
-    Request->FreeEndpointDescriptor(EndpointDescriptor);
-
-    //
     // release request
     //
     Request->Release();

Modified: branches/usb-bringup-trunk/drivers/usb/usbohci/usb_request.cpp
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbohci/usb_request.cpp [iso-8859-1]
(original)
+++ branches/usb-bringup-trunk/drivers/usb/usbohci/usb_request.cpp [iso-8859-1] Fri Feb  3
09:52:29 2012
@@ -64,6 +64,8 @@
     NTSTATUS CreateIsochronousTransferDescriptor(OUT POHCI_ISO_TD *OutDescriptor, ULONG
FrameCount);
     UCHAR GetEndpointAddress();
     USHORT GetMaxPacketSize();
+    VOID CheckError(struct _OHCI_ENDPOINT_DESCRIPTOR * OutDescriptor);
+
 
     // constructor / destructor
     CUSBRequest(IUnknown *OuterUnknown){}
@@ -968,7 +970,6 @@
 
     }
 
-
     //
     // FIXME: detect type
     //
@@ -1269,7 +1270,37 @@
         //
         // initialize data descriptor
         //
-        DataDescriptor->Flags = OHCI_TD_BUFFER_ROUNDING|
OHCI_TD_SET_CONDITION_CODE(OHCI_TD_CONDITION_NOT_ACCESSED) |
OHCI_TD_SET_DELAY_INTERRUPT(OHCI_TD_INTERRUPT_NONE) | OHCI_TD_TOGGLE_CARRY |
OHCI_TD_DIRECTION_PID_IN;
+        DataDescriptor->Flags =
OHCI_TD_SET_CONDITION_CODE(OHCI_TD_CONDITION_NOT_ACCESSED) |
OHCI_TD_SET_DELAY_INTERRUPT(OHCI_TD_INTERRUPT_NONE) | OHCI_TD_TOGGLE_CARRY |
OHCI_TD_TOGGLE_1;
+
+        if (m_EndpointDescriptor)
+        {
+            if (USB_ENDPOINT_DIRECTION_OUT(m_EndpointDescriptor->bEndpointAddress))
+            {
+                //
+                // direction out
+                //
+                DataDescriptor->Flags |= OHCI_TD_DIRECTION_PID_OUT;
+            }
+            else
+            {
+                //
+                // direction in
+                //
+                DataDescriptor->Flags |= OHCI_TD_DIRECTION_PID_IN;
+            }
+        }
+        else
+        {
+            //
+            // no end point address provided - assume its an in direction
+            //
+            DataDescriptor->Flags |= OHCI_TD_DIRECTION_PID_IN;
+        }
+
+        //
+        // FIXME verify short packets are ok
+        //
+        //DataDescriptor->Flags |= OHCI_TD_BUFFER_ROUNDING;
 
         //
         // store physical address of buffer
@@ -1282,7 +1313,7 @@
     //
     // initialize setup descriptor
     //
-    SetupDescriptor->Flags = OHCI_TD_DIRECTION_PID_SETUP |
OHCI_TD_SET_CONDITION_CODE(OHCI_TD_CONDITION_NOT_ACCESSED) | OHCI_TD_TOGGLE_0 |
OHCI_TD_SET_DELAY_INTERRUPT(OHCI_TD_INTERRUPT_NONE);
+    SetupDescriptor->Flags = OHCI_TD_BUFFER_ROUNDING | OHCI_TD_DIRECTION_PID_SETUP |
OHCI_TD_SET_CONDITION_CODE(OHCI_TD_CONDITION_NOT_ACCESSED) | OHCI_TD_TOGGLE_0 |
OHCI_TD_SET_DELAY_INTERRUPT(OHCI_TD_INTERRUPT_NONE);
 
     if (m_SetupPacket)
     {
@@ -1561,6 +1592,134 @@
 }
 
 VOID
+CUSBRequest::CheckError(
+    struct _OHCI_ENDPOINT_DESCRIPTOR * OutDescriptor)
+{
+    POHCI_GENERAL_TD TransferDescriptor;
+    ULONG ConditionCode;
+    PURB Urb;
+    PIO_STACK_LOCATION IoStack;
+
+
+    //
+    // set status code
+    //
+    m_NtStatusCode = STATUS_SUCCESS;
+    m_UrbStatusCode = USBD_STATUS_SUCCESS;
+
+
+    if (OutDescriptor->Flags & OHCI_ENDPOINT_ISOCHRONOUS_FORMAT)
+    {
+        //
+        // FIXME: handle isochronous support
+        //
+        ASSERT(FALSE);
+    }
+    else
+    {
+        //
+        // get first general transfer descriptor
+        //
+        TransferDescriptor = (POHCI_GENERAL_TD)OutDescriptor->HeadLogicalDescriptor;
+
+        while(TransferDescriptor)
+        {
+             //
+             // the descriptor must have been processed
+             //
+            ASSERT(OHCI_TD_GET_CONDITION_CODE(TransferDescriptor->Flags) !=
OHCI_TD_CONDITION_NOT_ACCESSED);
+
+            //
+            // get condition code
+            //
+            ConditionCode = OHCI_TD_GET_CONDITION_CODE(TransferDescriptor->Flags);
+            if (ConditionCode != OHCI_TD_CONDITION_NO_ERROR)
+            {
+                //
+                // FIXME status code
+                //
+                m_NtStatusCode = STATUS_UNSUCCESSFUL;
+
+                switch(ConditionCode)
+                {
+                    case OHCI_TD_CONDITION_CRC_ERROR:
+                        DPRINT1("OHCI_TD_CONDITION_CRC_ERROR detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_CRC;
+                        break;
+                    case OHCI_TD_CONDITION_BIT_STUFFING:
+                        DPRINT1("OHCI_TD_CONDITION_BIT_STUFFING detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_BTSTUFF;
+                        break;
+                    case OHCI_TD_CONDITION_TOGGLE_MISMATCH:
+                        DPRINT1("OHCI_TD_CONDITION_TOGGLE_MISMATCH detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_DATA_TOGGLE_MISMATCH;
+                        break;
+                    case OHCI_TD_CONDITION_STALL:
+                        DPRINT1("OHCI_TD_CONDITION_STALL detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_STALL_PID;
+                        break;
+                    case OHCI_TD_CONDITION_NO_RESPONSE:
+                        DPRINT1("OHCI_TD_CONDITION_NO_RESPONSE detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_DEV_NOT_RESPONDING;
+                        break;
+                    case OHCI_TD_CONDITION_PID_CHECK_FAILURE:
+                        DPRINT1("OHCI_TD_CONDITION_PID_CHECK_FAILURE detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_PID_CHECK_FAILURE;
+                        break;
+                    case OHCI_TD_CONDITION_UNEXPECTED_PID:
+                        DPRINT1("OHCI_TD_CONDITION_UNEXPECTED_PID detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_UNEXPECTED_PID;
+                        break;
+                    case OHCI_TD_CONDITION_DATA_OVERRUN:
+                        DPRINT1("OHCI_TD_CONDITION_DATA_OVERRUN detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_DATA_OVERRUN;
+                        break;
+                    case OHCI_TD_CONDITION_DATA_UNDERRUN:
+                        if (m_Irp)
+                        {
+                            //
+                            // get current irp stack location
+                            //
+                            IoStack = IoGetCurrentIrpStackLocation(m_Irp);
+
+                            //
+                            // get urb
+                            //
+                            Urb = (PURB)IoStack->Parameters.Others.Argument1;
+
+                            if(Urb->UrbBulkOrInterruptTransfer.TransferFlags &
USBD_SHORT_TRANSFER_OK)
+                            {
+                                //
+                                // short packets are ok
+                                //
+                                ASSERT(Urb->UrbHeader.Function ==
URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER);
+                                m_NtStatusCode = STATUS_SUCCESS;
+                                break;
+                            }
+                        }
+                        DPRINT1("OHCI_TD_CONDITION_DATA_UNDERRUN detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_DATA_UNDERRUN;
+                        break;
+                    case OHCI_TD_CONDITION_BUFFER_OVERRUN:
+                        DPRINT1("OHCI_TD_CONDITION_BUFFER_OVERRUN detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_BUFFER_OVERRUN;
+                        break;
+                    case OHCI_TD_CONDITION_BUFFER_UNDERRUN:
+                        DPRINT1("OHCI_TD_CONDITION_BUFFER_UNDERRUN detected in
TransferDescriptor TransferDescriptor %p\n", TransferDescriptor);
+                        m_UrbStatusCode = USBD_STATUS_BUFFER_UNDERRUN;
+                        break;
+                }
+            }
+
+            //
+            // get next
+            //
+            TransferDescriptor =
(POHCI_GENERAL_TD)TransferDescriptor->NextLogicalDescriptor;
+        }
+    }
+}
+
+VOID
 CUSBRequest::CompletionCallback(
     struct _OHCI_ENDPOINT_DESCRIPTOR * OutDescriptor)
 {
@@ -1570,17 +1729,16 @@
     DPRINT("CUSBRequest::CompletionCallback Descriptor %p PhysicalAddress
%x\n", OutDescriptor, OutDescriptor->PhysicalAddress.LowPart);
 
     //
-    // set status code
-    //
-    m_NtStatusCode = STATUS_SUCCESS;
-    m_UrbStatusCode = USBD_STATUS_SUCCESS;
+    // check for errors
+    //
+    CheckError(OutDescriptor);
 
     if (m_Irp)
     {
         //
         // set irp completion status
         //
-        m_Irp->IoStatus.Status = STATUS_SUCCESS; //FIXME
+        m_Irp->IoStatus.Status = m_NtStatusCode;
 
         //
         // get current irp stack location
@@ -1595,7 +1753,7 @@
         //
         // store urb status
         //
-        Urb->UrbHeader.Status = USBD_STATUS_SUCCESS; //FIXME
+        Urb->UrbHeader.Status = m_UrbStatusCode;
 
         //
         // Check if the MDL was created