[reactos] 03/17: [USBSTOR] Do not create a new Irp for USB requests - use the original one from higher-level driver instead. Refactored CSWCompletionRoutine for correct handling different CSW statuses, more work to be done here.

[Victor Perevertkin]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=ed6724cd7e2d769d7453f…

commit ed6724cd7e2d769d7453ff0bd3eb3bc835bf2f04
Author:     Victor Perevertkin <victor(a)perevertkin.ru>
AuthorDate: Thu Mar 28 02:45:32 2019 +0300
Commit:     Victor Perevertkin <victor(a)perevertkin.ru>
CommitDate: Tue Jun 11 04:39:43 2019 +0300

    [USBSTOR] Do not create a new Irp for USB requests - use the original one
    from higher-level driver instead.
    Refactored CSWCompletionRoutine for correct handling different CSW
    statuses, more work to be done here.
---
 drivers/usb/usbstor/scsi.c    | 179 +++++++++++++++++-------------------------
 drivers/usb/usbstor/usbstor.h |   4 +
 2 files changed, 75 insertions(+), 108 deletions(-)

diff --git a/drivers/usb/usbstor/scsi.c b/drivers/usb/usbstor/scsi.c
index b2989f01fce..120a3b09811 100644
--- a/drivers/usb/usbstor/scsi.c
+++ b/drivers/usb/usbstor/scsi.c
@@ -57,6 +57,7 @@ USBSTOR_AllocateIrpContext()
     return Context;
 }
 
+static
 BOOLEAN
 USBSTOR_IsCSWValid(
     PIRP_CONTEXT Context)
@@ -73,12 +74,6 @@ USBSTOR_IsCSWValid(
         return FALSE;
     }
 
-    if (Context->csw->Status != 0x00)
-    {
-        DPRINT1("[USBSTOR] Expected Status 0x00 but got %x\n",
Context->csw->Status);
-        return FALSE;
-    }
-
     return TRUE;
 }
 
@@ -128,99 +123,99 @@ USBSTOR_CSWCompletionRoutine(
     PIRP_CONTEXT Context;
     PIO_STACK_LOCATION IoStack;
     PSCSI_REQUEST_BLOCK Request;
-    PCDB pCDB;
     PUFI_CAPACITY_RESPONSE Response;
     NTSTATUS Status;
 
     Context = (PIRP_CONTEXT)Ctx;
 
-    if (Context->TransferBufferMDL)
+    if (Context->TransferBufferMDL && Context->TransferBufferMDL !=
Context->Irp->MdlAddress)
     {
-        // is there an irp associated
-        if (Context->Irp)
-        {
-            // did we allocate the mdl
-            if (Context->TransferBufferMDL != Context->Irp->MdlAddress)
-            {
-                IoFreeMdl(Context->TransferBufferMDL);
-            }
-        }
-        else
-        {
-            IoFreeMdl(Context->TransferBufferMDL);
-        }
+        IoFreeMdl(Context->TransferBufferMDL);
     }
 
-    DPRINT("USBSTOR_CSWCompletionRoutine Status %x\n",
Irp->IoStatus.Status);
+    DPRINT("USBSTOR_CSWCompletionRoutine Irp %p Ctx %p Status %x\n", Irp, Ctx,
Irp->IoStatus.Status);
 
-    if (!NT_SUCCESS(Irp->IoStatus.Information))
+    // first check for Irp errors
+    if (!NT_SUCCESS(Irp->IoStatus.Status))
     {
-        if (Context->ErrorIndex == 0)
+        if (USBD_STATUS(Context->Urb.UrbHeader.Status) ==
USBD_STATUS(USBD_STATUS_STALL_PID))
         {
-            Context->ErrorIndex = 1;
+            if (Context->ErrorIndex == 0)
+            {
+                Context->ErrorIndex = 1;
+
+                // clear stall and resend cbw
+                Status = USBSTOR_QueueWorkItem(Context, Irp);
+                ASSERT(Status == STATUS_MORE_PROCESSING_REQUIRED);
 
-            // clear stall and resend cbw
-            Status = USBSTOR_QueueWorkItem(Context, Irp);
-            ASSERT(Status == STATUS_MORE_PROCESSING_REQUIRED);
-            return STATUS_MORE_PROCESSING_REQUIRED;
+                return STATUS_MORE_PROCESSING_REQUIRED;
+            }
+        }
+        else
+        {
+            DPRINT1("USBSTOR_CSWCompletionRoutine: Urb.Hdr.Status - %x\n",
Context->Urb.UrbHeader.Status);
         }
 
         // perform reset recovery
         Context->ErrorIndex = 2;
-        IoFreeIrp(Irp);
         Status = USBSTOR_QueueWorkItem(Context, NULL);
         ASSERT(Status == STATUS_MORE_PROCESSING_REQUIRED);
         return STATUS_MORE_PROCESSING_REQUIRED;
     }
 
-    if (!USBSTOR_IsCSWValid(Context))
+    // now check the CSW packet validity
+    if (!USBSTOR_IsCSWValid(Context) || Context->csw->Status ==
CSW_STATUS_PHASE_ERROR)
     {
         // perform reset recovery
         Context->ErrorIndex = 2;
-        IoFreeIrp(Irp);
         Status = USBSTOR_QueueWorkItem(Context, NULL);
         ASSERT(Status == STATUS_MORE_PROCESSING_REQUIRED);
         return STATUS_MORE_PROCESSING_REQUIRED;
     }
 
-
-    IoStack = IoGetCurrentIrpStackLocation(Context->Irp);
-
-    Request = (PSCSI_REQUEST_BLOCK)IoStack->Parameters.Others.Argument1;
+    IoStack = IoGetCurrentIrpStackLocation(Irp);
+    Request = IoStack->Parameters.Scsi.Srb;
     ASSERT(Request);
 
     Status = Irp->IoStatus.Status;
 
-    pCDB = (PCDB)Request->Cdb;
-    Request->SrbStatus = SRB_STATUS_SUCCESS;
-
-    // read capacity needs special work
-    if (pCDB->AsByte[0] == SCSIOP_READ_CAPACITY)
+    // finally check for CSW errors
+    if (Context->csw->Status == CSW_STATUS_COMMAND_PASSED)
     {
-        // get output buffer
-        Response = (PUFI_CAPACITY_RESPONSE)Context->TransferData;
+        // read capacity needs special work
+        if (Request->Cdb[0] == SCSIOP_READ_CAPACITY)
+        {
+            // get output buffer
+            Response = (PUFI_CAPACITY_RESPONSE)Context->TransferData;
 
-        // store in pdo
-        Context->PDODeviceExtension->BlockLength =
NTOHL(Response->BlockLength);
-        Context->PDODeviceExtension->LastLogicBlockAddress =
NTOHL(Response->LastLogicalBlockAddress);
+            // store in pdo
+            Context->PDODeviceExtension->BlockLength =
NTOHL(Response->BlockLength);
+            Context->PDODeviceExtension->LastLogicBlockAddress =
NTOHL(Response->LastLogicalBlockAddress);
+        }
+
+        Status = STATUS_SUCCESS;
+        Request->SrbStatus = SRB_STATUS_SUCCESS;
+        Irp->IoStatus.Status = STATUS_SUCCESS;
+        Irp->IoStatus.Information = Request->DataTransferLength;
+    }
+    else if (Context->csw->Status == CSW_STATUS_COMMAND_FAILED)
+    {
+        DPRINT("USBSTOR_CSWCompletionRoutine: CSW_STATUS_COMMAND_FAILED\n");
+        // perform reset recovery
+        Context->ErrorIndex = 2;
+        Status = USBSTOR_QueueWorkItem(Context, NULL);
+        ASSERT(Status == STATUS_MORE_PROCESSING_REQUIRED);
+        return STATUS_MORE_PROCESSING_REQUIRED;
     }
 
     FreeItem(Context->cbw);
 
-    // FIXME: check status
-    Context->Irp->IoStatus.Status = Irp->IoStatus.Status;
-    Context->Irp->IoStatus.Information = Context->TransferDataLength;
-
     // terminate current request
-    USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
Context->Irp);
-
-    IoCompleteRequest(Context->Irp, IO_NO_INCREMENT);
-
+    USBSTOR_QueueTerminateRequest(Context->PDODeviceExtension->LowerDeviceObject,
Irp);
     USBSTOR_QueueNextRequest(Context->PDODeviceExtension->LowerDeviceObject);
 
-    IoFreeIrp(Irp);
     FreeItem(Context);
-    return STATUS_MORE_PROCESSING_REQUIRED;
+    return Status;
 }
 
 VOID
@@ -406,7 +401,6 @@ USBSTOR_SendRequest(
     PIRP_CONTEXT Context;
     PPDO_DEVICE_EXTENSION PDODeviceExtension;
     PFDO_DEVICE_EXTENSION FDODeviceExtension;
-    PIRP Irp;
     PUCHAR MdlVirtualAddress;
 
     Context = USBSTOR_AllocateIrpContext();
@@ -450,50 +444,34 @@ USBSTOR_SendRequest(
     if (Context->TransferDataLength)
     {
         // check if the original request already does have an mdl associated
-        if (OriginalRequest)
+        if ((OriginalRequest->MdlAddress != NULL) &&
+            (Context->TransferData == NULL || Command[0] == SCSIOP_READ || Command[0]
== SCSIOP_WRITE))
         {
-            if ((OriginalRequest->MdlAddress != NULL) &&
-                (Context->TransferData == NULL || Command[0] == SCSIOP_READ ||
Command[0] == SCSIOP_WRITE))
+            // Sanity check that the Mdl does describe the TransferData for read/write
+            if (CommandLength == UFI_READ_WRITE_CMD_LEN)
             {
-                // Sanity check that the Mdl does describe the TransferData for
read/write
-                if (CommandLength == UFI_READ_WRITE_CMD_LEN)
-                {
-                    MdlVirtualAddress =
MmGetMdlVirtualAddress(OriginalRequest->MdlAddress);
+                MdlVirtualAddress =
MmGetMdlVirtualAddress(OriginalRequest->MdlAddress);
 
-                    // is there an offset
-                    if (MdlVirtualAddress != Context->TransferData)
+                // is there an offset
+                if (MdlVirtualAddress != Context->TransferData)
+                {
+                    // lets build an mdl
+                    Context->TransferBufferMDL =
IoAllocateMdl(Context->TransferData, MmGetMdlByteCount(OriginalRequest->MdlAddress),
FALSE, FALSE, NULL);
+                    if (!Context->TransferBufferMDL)
                     {
-                        // lets build an mdl
-                        Context->TransferBufferMDL =
IoAllocateMdl(Context->TransferData, MmGetMdlByteCount(OriginalRequest->MdlAddress),
FALSE, FALSE, NULL);
-                        if (!Context->TransferBufferMDL)
-                        {
-                            FreeItem(Context->cbw);
-                            FreeItem(Context);
-                            return STATUS_INSUFFICIENT_RESOURCES;
-                        }
-
-                        IoBuildPartialMdl(OriginalRequest->MdlAddress,
Context->TransferBufferMDL, Context->TransferData, Context->TransferDataLength);
+                        FreeItem(Context->cbw);
+                        FreeItem(Context);
+                        return STATUS_INSUFFICIENT_RESOURCES;
                     }
-                }
 
-                if (!Context->TransferBufferMDL)
-                {
-                    // I/O paging request
-                    Context->TransferBufferMDL = OriginalRequest->MdlAddress;
+                    IoBuildPartialMdl(OriginalRequest->MdlAddress,
Context->TransferBufferMDL, Context->TransferData, Context->TransferDataLength);
                 }
             }
-            else
-            {
-                // allocate mdl for buffer, buffer must be allocated from NonPagedPool
-                Context->TransferBufferMDL = IoAllocateMdl(Context->TransferData,
Context->TransferDataLength, FALSE, FALSE, NULL);
-                if (!Context->TransferBufferMDL)
-                {
-                    FreeItem(Context->cbw);
-                    FreeItem(Context);
-                    return STATUS_INSUFFICIENT_RESOURCES;
-                }
 
-                MmBuildMdlForNonPagedPool(Context->TransferBufferMDL);
+            if (!Context->TransferBufferMDL)
+            {
+                // I/O paging request
+                Context->TransferBufferMDL = OriginalRequest->MdlAddress;
             }
         }
         else
@@ -511,22 +489,7 @@ USBSTOR_SendRequest(
         }
     }
 
-    Irp = IoAllocateIrp(DeviceObject->StackSize, FALSE);
-    if (!Irp)
-    {
-        FreeItem(Context->cbw);
-        FreeItem(Context);
-        return STATUS_INSUFFICIENT_RESOURCES;
-    }
-
-    if (OriginalRequest)
-    {
-        IoMarkIrpPending(OriginalRequest);
-    }
-
-    USBSTOR_SendCBW(Context, Irp);
-
-    return STATUS_PENDING;
+    return USBSTOR_SendCBW(Context, OriginalRequest);
 }
 
 NTSTATUS
diff --git a/drivers/usb/usbstor/usbstor.h b/drivers/usb/usbstor/usbstor.h
index f477e9a1a04..40b369bd15e 100644
--- a/drivers/usb/usbstor/usbstor.h
+++ b/drivers/usb/usbstor/usbstor.h
@@ -105,6 +105,10 @@ C_ASSERT(sizeof(CBW) == 31);
 
 #define MAX_LUN 0xF
 
+#define CSW_STATUS_COMMAND_PASSED 0x00
+#define CSW_STATUS_COMMAND_FAILED 0x01
+#define CSW_STATUS_PHASE_ERROR    0x02
+
 typedef struct
 {
     ULONG Signature;                                                 // CSW signature