[ros-diffs] [janderwald] 51764: [USBSTOR] - Add asserts - Rewrite queue methods to accept the FDO only - Fix multiple synchronization bugs - Don't start next irp before current irp has completed - Use contious...

Author: janderwald
Date: Sun May 15 14:59:16 2011
New Revision: 51764

URL: http://svn.reactos.org/svn/reactos?rev=51764&view=rev
Log:
[USBSTOR]
- Add asserts
- Rewrite queue methods to accept the FDO only 
- Fix multiple synchronization bugs
- Don't start next irp before current irp has completed
- Use contiouslogicalbytes blocks from srb
- Should fix race conditions

Modified:
    branches/usb-bringup/drivers/usb/usbstor/disk.c
    branches/usb-bringup/drivers/usb/usbstor/queue.c
    branches/usb-bringup/drivers/usb/usbstor/scsi.c
    branches/usb-bringup/drivers/usb/usbstor/usbstor.h

Modified: branches/usb-bringup/drivers/usb/usbstor/disk.c
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbstor…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbstor/disk.c [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbstor/disk.c [iso-8859-1] Sun May 15 14:59:16 2011
@@ -32,6 +32,11 @@
     Request = (PSCSI_REQUEST_BLOCK)IoStack->Parameters.Others.Argument1;
 
     //
+    // sanity check
+    //
+    ASSERT(Request);
+
+    //
     // get device extension
     //
     PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
@@ -39,8 +44,7 @@
     //
     // sanity check
     //
-    ASSERT(Request);
-    ASSERT(PDODeviceExtension);
+    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
 
     switch(Request->Function)
     {
@@ -87,7 +91,7 @@
             //
             // add the request
             //
-            if (!USBSTOR_QueueAddIrp(DeviceObject, Irp))
+            if (!USBSTOR_QueueAddIrp(PDODeviceExtension->LowerDeviceObject, Irp))
             {
                 //
                 // irp was not added to the queue
@@ -154,8 +158,7 @@
             //
             // release queue
             //
-            USBSTOR_QueueRelease(DeviceObject);
-
+            USBSTOR_QueueRelease(PDODeviceExtension->LowerDeviceObject);
 
             //
             // set status success
@@ -173,7 +176,7 @@
             //
             // flush all requests
             //
-            USBSTOR_QueueFlushIrps(DeviceObject);
+            USBSTOR_QueueFlushIrps(PDODeviceExtension->LowerDeviceObject);
 
             //
             // set status success

Modified: branches/usb-bringup/drivers/usb/usbstor/queue.c
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbstor…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbstor/queue.c [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbstor/queue.c [iso-8859-1] Sun May 15 14:59:16
2011
@@ -15,6 +15,10 @@
 USBSTOR_QueueInitialize(
     PFDO_DEVICE_EXTENSION FDODeviceExtension)
 {
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
     // initialize queue lock
@@ -85,21 +89,19 @@
 {
     PDRIVER_CANCEL OldDriverCancel;
     KIRQL OldLevel;
-    PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PFDO_DEVICE_EXTENSION FDODeviceExtension;
     BOOLEAN IrpListFreeze;
     BOOLEAN SrbProcessing;
 
     //
-    // get pdo device extension
-    //
-    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
-    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
-
-    //
     // get FDO device extension
     //
-    FDODeviceExtension =
(PFDO_DEVICE_EXTENSION)PDODeviceExtension->LowerDeviceObject->DeviceExtension;
+    FDODeviceExtension = (PFDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
     // mark irp pending
@@ -183,25 +185,19 @@
     IN PDEVICE_OBJECT DeviceObject)
 {
     KIRQL OldLevel;
-    PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PFDO_DEVICE_EXTENSION FDODeviceExtension;
     PLIST_ENTRY Entry;
     PIRP Irp = NULL;
 
     //
-    // get pdo device extension
-    //
-    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
-
-    //
-    // sanity check
-    //
-    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
-
-    //
     // get FDO device extension
     //
-    FDODeviceExtension =
(PFDO_DEVICE_EXTENSION)PDODeviceExtension->LowerDeviceObject->DeviceExtension;
+    FDODeviceExtension = (PFDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
     // acquire lock
@@ -240,7 +236,6 @@
     IN PDEVICE_OBJECT DeviceObject)
 {
     KIRQL OldLevel;
-    PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PFDO_DEVICE_EXTENSION FDODeviceExtension;
     PLIST_ENTRY Entry;
     PIRP Irp;
@@ -248,19 +243,14 @@
     PSCSI_REQUEST_BLOCK Request;
 
     //
-    // get pdo device extension
-    //
-    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
-
-    //
-    // sanity check
-    //
-    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
-
-    //
     // get FDO device extension
     //
-    FDODeviceExtension =
(PFDO_DEVICE_EXTENSION)PDODeviceExtension->LowerDeviceObject->DeviceExtension;
+    FDODeviceExtension = (PFDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
     // acquire lock
@@ -320,10 +310,58 @@
 }
 
 VOID
+USBSTOR_QueueTerminateRequest(
+    IN PDEVICE_OBJECT FDODeviceObject,
+    IN BOOLEAN ModifySrbState)
+{
+    KIRQL OldLevel;
+    PFDO_DEVICE_EXTENSION FDODeviceExtension;
+
+    //
+    // get FDO device extension
+    //
+    FDODeviceExtension = (PFDO_DEVICE_EXTENSION)FDODeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
+
+    //
+    // acquire lock
+    //
+    KeAcquireSpinLock(&FDODeviceExtension->IrpListLock, &OldLevel);
+
+    //
+    // decrement pending irp count
+    //
+    FDODeviceExtension->IrpPendingCount--;
+
+    if (ModifySrbState)
+    {
+        //
+        // sanity check
+        //
+        ASSERT(FDODeviceExtension->SrbActive == TRUE);
+
+        //
+        // indicate processing is completed
+        //
+        FDODeviceExtension->SrbActive = FALSE;
+    }
+
+    //
+    // release lock
+    //
+    KeReleaseSpinLock(&FDODeviceExtension->IrpListLock, OldLevel);
+
+}
+
+VOID
 USBSTOR_QueueNextRequest(
     IN PDEVICE_OBJECT DeviceObject)
 {
-    PPDO_DEVICE_EXTENSION PDODeviceExtension;
+    PFDO_DEVICE_EXTENSION FDODeviceExtension;
     PIRP Irp;
     PIO_STACK_LOCATION IoStack;
     PSCSI_REQUEST_BLOCK Request;
@@ -331,12 +369,12 @@
     //
     // get pdo device extension
     //
-    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
-
-    //
-    // sanity check
-    //
-    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
+    FDODeviceExtension = (PFDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
     // remove first irp from list
@@ -351,6 +389,7 @@
         //
         // no work to do
         //
+        IoStartNextPacket(DeviceObject, TRUE);
         return;
     }
 
@@ -365,16 +404,25 @@
     Request = (PSCSI_REQUEST_BLOCK)IoStack->Parameters.Others.Argument1;
 
     //
+    // sanity check
+    //
+    ASSERT(Request);
+
+    //
     // start next packet
     //
-    IoStartPacket(PDODeviceExtension->LowerDeviceObject, Irp,
&Request->QueueSortKey, USBSTOR_CancelIo);
+    IoStartPacket(DeviceObject, Irp, &Request->QueueSortKey, USBSTOR_CancelIo);
+
+    //
+    // start next request
+    //
+    IoStartNextPacket(DeviceObject, TRUE);
 }
 
 VOID
 USBSTOR_QueueRelease(
     IN PDEVICE_OBJECT DeviceObject)
 {
-    PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PFDO_DEVICE_EXTENSION FDODeviceExtension;
     PIRP Irp;
     KIRQL OldLevel;
@@ -382,19 +430,14 @@
     PSCSI_REQUEST_BLOCK Request;
 
     //
-    // get pdo device extension
-    //
-    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
-
-    //
-    // sanity check
-    //
-    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
-
-    //
     // get FDO device extension
     //
-    FDODeviceExtension =
(PFDO_DEVICE_EXTENSION)PDODeviceExtension->LowerDeviceObject->DeviceExtension;
+    FDODeviceExtension = (PFDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
     // acquire lock
@@ -440,7 +483,7 @@
     //
     // start new packet
     //
-    IoStartPacket(PDODeviceExtension->LowerDeviceObject, // FDO
+    IoStartPacket(DeviceObject,
                   Irp,
                   &Request->QueueSortKey, 
                   USBSTOR_CancelIo);
@@ -511,12 +554,8 @@
             //
             // queue next request
             //
+            USBSTOR_QueueTerminateRequest(DeviceObject, FALSE);
             USBSTOR_QueueNextRequest(DeviceObject);
-
-            //
-            // start next request
-            //
-            IoStartNextPacket(DeviceObject, TRUE);
         }
 
         //
@@ -540,6 +579,12 @@
     //
     ResetInProgress = FDODeviceExtension->ResetInProgress;
     ASSERT(ResetInProgress == FALSE);
+
+    //
+    // sanity check
+    //
+    ASSERT(FDODeviceExtension->SrbActive == FALSE);
+    FDODeviceExtension->SrbActive = TRUE;
 
     //
     // release lock
@@ -572,37 +617,16 @@
         Irp->IoStatus.Information = 0;
         Irp->IoStatus.Status = STATUS_DEVICE_DOES_NOT_EXIST;
         IoCompleteRequest(Irp, IO_NO_INCREMENT);
-    }
-    else
-    {
-        //
-        // execute scsi
-        //
-        Status = USBSTOR_HandleExecuteSCSI(IoStack->DeviceObject, Irp);
-
-        //
-        // acquire lock
-        //
-        KeAcquireSpinLock(&FDODeviceExtension->IrpListLock, &OldLevel);
-
-        //
-        // FIXME: synchronize with error handler
-        //
-        FDODeviceExtension->IrpPendingCount--;
-
-        //
-        // release lock
-        //
-        KeReleaseSpinLock(&FDODeviceExtension->IrpListLock, OldLevel);
-    }
-
-    //
-    // FIXME: synchronize action with error handling
-    //
-    USBSTOR_QueueNextRequest(IoStack->DeviceObject);
-
-    //
-    // start next request
-    //
-    IoStartNextPacket(DeviceObject, TRUE);
-}
+        USBSTOR_QueueTerminateRequest(DeviceObject, TRUE);
+        return;
+    }
+
+    //
+    // execute scsi
+    //
+    Status = USBSTOR_HandleExecuteSCSI(IoStack->DeviceObject, Irp);
+
+    //
+    // FIXME: handle error
+    //
+}

Modified: branches/usb-bringup/drivers/usb/usbstor/scsi.c
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbstor…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbstor/scsi.c [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbstor/scsi.c [iso-8859-1] Sun May 15 14:59:16 2011
@@ -233,6 +233,16 @@
         // complete request
         //
         IoCompleteRequest(Context->Irp, IO_NO_INCREMENT);
+
+        //
+        // terminate current request
+        //
+       
USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
TRUE);
+
+        //
+        // start next request
+        //
+        USBSTOR_QueueNextRequest(Context->PDODeviceExtension->LowerDeviceObject);
     }
 
     if (Context->Event)
@@ -432,7 +442,7 @@
     IN PDEVICE_OBJECT DeviceObject,
     IN PIRP OriginalRequest,
     IN OPTIONAL PKEVENT Event,
-    IN ULONG CommandLength,
+    IN UCHAR CommandLength,
     IN PUCHAR Command,
     IN ULONG TransferDataLength,
     IN PUCHAR TransferData)
@@ -740,11 +750,11 @@
     UFI_SENSE_CMD Cmd;
     NTSTATUS Status;
     PVOID Response;
-    PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PCBW OutControl;
     PCDB pCDB;
     PUFI_MODE_PARAMETER_HEADER Header;
 #endif
+    PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PIO_STACK_LOCATION IoStack;
     PSCSI_REQUEST_BLOCK Request;
 
@@ -763,6 +773,26 @@
     Irp->IoStatus.Information = Request->DataTransferLength;
     Irp->IoStatus.Status = STATUS_SUCCESS;
     IoCompleteRequest(Irp, IO_NO_INCREMENT);
+
+    //
+    // get PDO device extension
+    //
+    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
+
+    //
+    // terminate current request
+    //
+    USBSTOR_QueueTerminateRequest(PDODeviceExtension->LowerDeviceObject, TRUE);
+
+    //
+    // start next request
+    //
+    USBSTOR_QueueNextRequest(PDODeviceExtension->LowerDeviceObject);
 
     return STATUS_SUCCESS;
 
@@ -956,7 +986,8 @@
     RtlZeroMemory(&Cmd, sizeof(UFI_READ_WRITE_CMD));
     Cmd.Code = pCDB->AsByte[0];
     Cmd.LUN = (PDODeviceExtension->LUN & MAX_LUN);
-    Cmd.ContiguousLogicBlocks = _byteswap_ushort(BlockCount);
+    Cmd.ContiguousLogicBlocksByte0 = pCDB->CDB10.TransferBlocksMsb;
+    Cmd.ContiguousLogicBlocksByte1 = pCDB->CDB10.TransferBlocksLsb;
     Cmd.LogicalBlockByte0 = pCDB->CDB10.LogicalBlockByte0;
     Cmd.LogicalBlockByte1 = pCDB->CDB10.LogicalBlockByte1;
     Cmd.LogicalBlockByte2 = pCDB->CDB10.LogicalBlockByte2;
@@ -1023,6 +1054,17 @@
     NTSTATUS Status;
     PIO_STACK_LOCATION IoStack;
     PSCSI_REQUEST_BLOCK Request;
+    PPDO_DEVICE_EXTENSION PDODeviceExtension;
+
+    //
+    // get PDO device extension
+    //
+    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
 
     //
     // get current stack location
@@ -1082,6 +1124,17 @@
         Irp->IoStatus.Status = STATUS_SUCCESS;
         Irp->IoStatus.Information = Request->DataTransferLength;
         IoCompleteRequest(Irp, IO_NO_INCREMENT);
+
+        //
+        // terminate current request
+        //
+        USBSTOR_QueueTerminateRequest(PDODeviceExtension->LowerDeviceObject, TRUE);
+
+        //
+        // start next request
+        //
+        USBSTOR_QueueNextRequest(PDODeviceExtension->LowerDeviceObject);
+
         return STATUS_SUCCESS;
     }
     else if (pCDB->MODE_SENSE.OperationCode == SCSIOP_TEST_UNIT_READY)

Modified: branches/usb-bringup/drivers/usb/usbstor/usbstor.h
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup/drivers/usb/usbstor…
==============================================================================
--- branches/usb-bringup/drivers/usb/usbstor/usbstor.h [iso-8859-1] (original)
+++ branches/usb-bringup/drivers/usb/usbstor/usbstor.h [iso-8859-1] Sun May 15 14:59:16
2011
@@ -68,6 +68,7 @@
     BOOLEAN IrpListFreeze;                                                              
// if true the irp list is freezed
     BOOLEAN ResetInProgress;                                                            
// if hard reset is in progress
     ULONG IrpPendingCount;                                                              
// count of irp pending
+    BOOLEAN SrbActive;                                                                  
// debug field if srb is pending
 }FDO_DEVICE_EXTENSION, *PFDO_DEVICE_EXTENSION;
 
 typedef struct
@@ -164,7 +165,8 @@
     UCHAR LogicalBlockByte2;                                         // lba byte 2
     UCHAR LogicalBlockByte3;                                         // lba byte 3
     UCHAR Reserved;                                                  // reserved 0x00
-    USHORT ContiguousLogicBlocks;                                    // num of contiguous
logical blocks
+    UCHAR ContiguousLogicBlocksByte0;                                // msb contigious
logic blocks byte
+    UCHAR ContiguousLogicBlocksByte1;                                // msb contigious
logic blocks
     UCHAR Reserved1[3];                                              // reserved 0x00
 }UFI_READ_WRITE_CMD;
 
@@ -424,3 +426,11 @@
 USBSTOR_QueueInitialize(
     PFDO_DEVICE_EXTENSION FDODeviceExtension);
 
+VOID
+USBSTOR_QueueNextRequest(
+    IN PDEVICE_OBJECT DeviceObject);
+
+VOID
+USBSTOR_QueueTerminateRequest(
+    IN PDEVICE_OBJECT DeviceObject,
+    IN BOOLEAN ModifySrbState);