[cgutman] 55157: [USBSTOR] - Rewrite pending SRB handling and change some behavior of the IRP queue - The caller is no longer responsible for checking whether it can call USBSTOR_QueueNextRequest; ...

Author: cgutman
Date: Tue Jan 24 23:04:31 2012
New Revision: 55157

URL: http://svn.reactos.org/svn/reactos?rev=55157&view=rev
Log:
[USBSTOR]
- Rewrite pending SRB handling and change some behavior of the IRP queue
- The caller is no longer responsible for checking whether it can call
USBSTOR_QueueNextRequest; frozen IRP queue and pending SRB are both handled for them
- It's no longer required for the caller of USBSTOR_QueueTerminateRequest to know
whether the SRB was active (which was impossible before when handling a cancellation)
- Many potential race issues with IRP cancellation are eliminated
- Debugging hung SRBs is much easier now that pointer to the active one is stored in the
FDO

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

Modified: branches/usb-bringup-trunk/drivers/usb/usbstor/error.c
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbstor/error.c [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbstor/error.c [iso-8859-1] Tue Jan 24
23:04:31 2012
@@ -129,10 +129,10 @@
         /* Complete the master IRP */
         Context->Irp->IoStatus.Status = Status;
         Context->Irp->IoStatus.Information = 0;
+       
USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
Context->Irp);
         IoCompleteRequest(Context->Irp, IO_NO_INCREMENT);
 
         /* Start the next request */
-       
USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
TRUE);
         USBSTOR_QueueNextRequest(Context->PDODeviceExtension->LowerDeviceObject);
 
         /* Signal the context event */

Modified: branches/usb-bringup-trunk/drivers/usb/usbstor/queue.c
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbstor/queue.c [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbstor/queue.c [iso-8859-1] Tue Jan 24
23:04:31 2012
@@ -67,7 +67,13 @@
     //
     // now cancel the irp
     //
+    USBSTOR_QueueTerminateRequest(DeviceObject, Irp);
     IoCompleteRequest(Irp, IO_NO_INCREMENT);
+
+    //
+    // start the next one
+    //
+    USBSTOR_QueueNextRequest(DeviceObject);
 }
 
 VOID
@@ -117,7 +123,13 @@
     //
     // now cancel the irp
     //
+    USBSTOR_QueueTerminateRequest(DeviceObject, Irp);
     IoCompleteRequest(Irp, IO_NO_INCREMENT);
+
+    //
+    // start the next one
+    //
+    USBSTOR_QueueNextRequest(DeviceObject);
 }
 
 BOOLEAN
@@ -358,8 +370,14 @@
         //
         // complete request
         //
+        USBSTOR_QueueTerminateRequest(DeviceObject, Irp);
         IoCompleteRequest(Irp, IO_NO_INCREMENT);
         
+        //
+        // start next one
+        //
+        USBSTOR_QueueNextRequest(DeviceObject);
+
         //
         // acquire lock
         //
@@ -375,10 +393,12 @@
 VOID
 USBSTOR_QueueTerminateRequest(
     IN PDEVICE_OBJECT FDODeviceObject,
-    IN BOOLEAN ModifySrbState)
+    IN PIRP Irp)
 {
     KIRQL OldLevel;
     PFDO_DEVICE_EXTENSION FDODeviceExtension;
+    PIO_STACK_LOCATION IoStack = IoGetCurrentIrpStackLocation(Irp);
+    PSCSI_REQUEST_BLOCK Request =
(PSCSI_REQUEST_BLOCK)IoStack->Parameters.Others.Argument1;
 
     //
     // get FDO device extension
@@ -400,17 +420,15 @@
     //
     FDODeviceExtension->IrpPendingCount--;
 
-    if (ModifySrbState)
-    {
-        //
-        // sanity check
-        //
-        ASSERT(FDODeviceExtension->SrbActive == TRUE);
-
+    //
+    // check if this was our current active SRB
+    //
+    if (FDODeviceExtension->ActiveSrb == Request)
+    {
         //
         // indicate processing is completed
         //
-        FDODeviceExtension->SrbActive = FALSE;
+        FDODeviceExtension->ActiveSrb = NULL;
     }
 
     //
@@ -440,6 +458,18 @@
     ASSERT(FDODeviceExtension->Common.IsFDO);
 
     //
+    // check first if there's already a request pending or the queue is frozen
+    //
+    if (FDODeviceExtension->ActiveSrb != NULL ||
+        FDODeviceExtension->IrpListFreeze)
+    {
+        //
+        // no work to do yet
+        //
+        return;
+    }
+
+    //
     // remove first irp from list
     //
     Irp = USBSTOR_RemoveIrp(DeviceObject);
@@ -470,6 +500,11 @@
     // sanity check
     //
     ASSERT(Request);
+
+    //
+    // set the active SRB
+    //
+    FDODeviceExtension->ActiveSrb = Request;
 
     //
     // start next packet
@@ -605,21 +640,19 @@
         Irp->IoStatus.Information = 0;
 
         //
+        // terminate request
+        //
+        USBSTOR_QueueTerminateRequest(DeviceObject, Irp);
+
+        //
         // complete request
         //
         IoCompleteRequest(Irp, IO_NO_INCREMENT);
 
         //
-        // check if the queue has been frozen
-        //
-        if (FDODeviceExtension->IrpListFreeze == FALSE)
-        {
-            //
-            // queue next request
-            //
-            USBSTOR_QueueTerminateRequest(DeviceObject, FALSE);
-            USBSTOR_QueueNextRequest(DeviceObject);
-        }
+        // queue next request
+        //
+        USBSTOR_QueueNextRequest(DeviceObject);
 
         //
         // done
@@ -644,12 +677,6 @@
     ASSERT(ResetInProgress == FALSE);
 
     //
-    // sanity check
-    //
-    ASSERT(FDODeviceExtension->SrbActive == FALSE);
-    FDODeviceExtension->SrbActive = TRUE;
-
-    //
     // release lock
     //
     KeReleaseSpinLock(&FDODeviceExtension->IrpListLock, OldLevel);
@@ -679,8 +706,8 @@
         //
         Irp->IoStatus.Information = 0;
         Irp->IoStatus.Status = STATUS_DEVICE_DOES_NOT_EXIST;
+        USBSTOR_QueueTerminateRequest(DeviceObject, Irp);
         IoCompleteRequest(Irp, IO_NO_INCREMENT);
-        USBSTOR_QueueTerminateRequest(DeviceObject, TRUE);
         return;
     }
 

Modified: branches/usb-bringup-trunk/drivers/usb/usbstor/scsi.c
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbstor/scsi.c [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbstor/scsi.c [iso-8859-1] Tue Jan 24 23:04:31
2012
@@ -282,14 +282,14 @@
         Context->Irp->IoStatus.Information = Context->TransferDataLength;
 
         //
+        // terminate current request
+        //
+       
USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
Context->Irp);
+
+        //
         // complete request
         //
         IoCompleteRequest(Context->Irp, IO_NO_INCREMENT);
-
-        //
-        // terminate current request
-        //
-       
USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
TRUE);
 
         //
         // start next request
@@ -840,6 +840,16 @@
     PSCSI_REQUEST_BLOCK Request;
 
     //
+    // get PDO device extension
+    //
+    PDODeviceExtension = (PPDO_DEVICE_EXTENSION)DeviceObject->DeviceExtension;
+
+    //
+    // sanity check
+    //
+    ASSERT(PDODeviceExtension->Common.IsFDO == FALSE);
+
+    //
     // get current stack location
     //
     IoStack = IoGetCurrentIrpStackLocation(Irp);
@@ -853,22 +863,8 @@
     Request->SrbStatus = SRB_STATUS_SUCCESS;
     Irp->IoStatus.Information = Request->DataTransferLength;
     Irp->IoStatus.Status = STATUS_SUCCESS;
+    USBSTOR_QueueTerminateRequest(PDODeviceExtension->LowerDeviceObject, Irp);
     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
@@ -1204,12 +1200,8 @@
         Request->SrbStatus = SRB_STATUS_SUCCESS;
         Irp->IoStatus.Status = STATUS_SUCCESS;
         Irp->IoStatus.Information = Request->DataTransferLength;
+        USBSTOR_QueueTerminateRequest(PDODeviceExtension->LowerDeviceObject, Irp);
         IoCompleteRequest(Irp, IO_NO_INCREMENT);
-
-        //
-        // terminate current request
-        //
-        USBSTOR_QueueTerminateRequest(PDODeviceExtension->LowerDeviceObject, TRUE);
 
         //
         // start next request

Modified: branches/usb-bringup-trunk/drivers/usb/usbstor/usbstor.h
URL:
http://svn.reactos.org/svn/reactos/branches/usb-bringup-trunk/drivers/usb/u…
==============================================================================
--- branches/usb-bringup-trunk/drivers/usb/usbstor/usbstor.h [iso-8859-1] (original)
+++ branches/usb-bringup-trunk/drivers/usb/usbstor/usbstor.h [iso-8859-1] Tue Jan 24
23:04:31 2012
@@ -66,7 +66,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
+    PSCSI_REQUEST_BLOCK ActiveSrb;                                                      
// stores the current active SRB
 }FDO_DEVICE_EXTENSION, *PFDO_DEVICE_EXTENSION;
 
 typedef struct
@@ -439,4 +439,4 @@
 VOID
 USBSTOR_QueueTerminateRequest(
     IN PDEVICE_OBJECT DeviceObject,
-    IN BOOLEAN ModifySrbState);
+    IN PIRP Irp);