AtaBusDxe: Fix ReadBlockEx andWriteBlockEx to still signal event when the BufferSize...

DiskIoDxe: Fix ReadDiskEx and WriteDiskEx to not modify the user’s buffer when the BufferSize is 0.
DiskIoDxe: Fix ReadDiskEx and WriteDiskEx hang issue when the submitted blockio2 task is completed before submitting another blockio2 task.
DiskIoDxe: Fix FlushEx to free the flush task item in callback (memory leak issue).

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Reviewed-by: Feng Tian <feng.tian@intel.com>
git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@16215 6f19259b-4bc3-4df7-8a09-765794883524

diff --git a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.c b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.c
index b113b8d851b298f1eb993afa9d54c70df1300a06..26783169ea2cdafe7cbf0687387021d7b8debadf 100644 (file)
--- a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.c
+++ b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBus.c
@@ -4,7 +4,7 @@
   This file implements protocol interfaces: Driver Binding protocol,\r
   Block IO protocol and DiskInfo protocol.\r
 \r
-  Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.<BR>\r
+  Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.<BR>\r
   This program and the accompanying materials\r
   are licensed and made available under the terms and conditions of the BSD License\r
   which accompanies this distribution.  The full text of the license may be found at\r
@@ -1072,6 +1072,10 @@ BlockIoReadWrite (
   }\r
 \r
   if (BufferSize == 0) {\r
+    if ((Token != NULL) && (Token->Event != NULL)) {\r
+      Token->TransactionStatus = EFI_SUCCESS;\r
+      gBS->SignalEvent (Token->Event);\r
+    }\r
     return EFI_SUCCESS;\r
   }\r
 \r

diff --git a/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.c b/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.c
index 0658299288afa6aad95d56d669f46a712c7513f7..0f0dbfee8280214aba9d409656ee78d8bcfb72a0 100644 (file)
--- a/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.c
+++ b/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.c
@@ -9,7 +9,7 @@
     Aligned  - A read of N contiguous sectors.\r
     OverRun  - The last byte is not on a sector boundary.\r
 \r
-Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>\r
+Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>\r
 This program and the accompanying materials\r
 are licensed and made available under the terms and conditions of the BSD License\r
 which accompanies this distribution.  The full text of the license may be found at\r
@@ -366,7 +366,15 @@ DiskIoDestroySubtask (
   )\r
 {\r
   LIST_ENTRY               *Link;\r
+\r
+  if (Subtask->Task != NULL) {\r
+    EfiAcquireLock (&Subtask->Task->SubtasksLock);\r
+  }\r
   Link = RemoveEntryList (&Subtask->Link);\r
+  if (Subtask->Task != NULL) {\r
+    EfiReleaseLock (&Subtask->Task->SubtasksLock);\r
+  }\r
+\r
   if (!Subtask->Blocking) {\r
     if (Subtask->WorkingBuffer != NULL) {\r
       FreeAlignedPages (\r
@@ -430,19 +438,11 @@ DiskIo2OnReadWriteComplete (
       gBS->SignalEvent (Task->Token->Event);\r
 \r
       //\r
-      // Mark token to NULL\r
+      // Mark token to NULL indicating the Task is a dead task.\r
       //\r
       Task->Token = NULL;\r
     }\r
   }\r
-\r
-  if (IsListEmpty (&Task->Subtasks)) {\r
-    EfiAcquireLock (&Instance->TaskQueueLock);\r
-    RemoveEntryList (&Task->Link);\r
-    EfiReleaseLock (&Instance->TaskQueueLock);\r
-\r
-    FreePool (Task);\r
-  }\r
 }\r
 \r
 /**\r
@@ -752,6 +752,42 @@ DiskIo2Cancel (
   return EFI_SUCCESS;\r
 }\r
 \r
+/**\r
+  Remove the completed tasks from Instance->TaskQueue. Completed tasks are those who don't have any subtasks.\r
+\r
+  @param Instance    Pointer to the DISK_IO_PRIVATE_DATA.\r
+\r
+  @retval TRUE       The Instance->TaskQueue is empty after the completed tasks are removed.\r
+  @retval FALSE      The Instance->TaskQueue is not empty after the completed tasks are removed.\r
+**/\r
+BOOLEAN\r
+DiskIo2RemoveCompletedTask (\r
+  IN DISK_IO_PRIVATE_DATA     *Instance\r
+  )\r
+{\r
+  BOOLEAN                     QueueEmpty;\r
+  LIST_ENTRY                  *Link;\r
+  DISK_IO2_TASK               *Task;\r
+\r
+  QueueEmpty = TRUE;\r
+\r
+  EfiAcquireLock (&Instance->TaskQueueLock);\r
+  for (Link = GetFirstNode (&Instance->TaskQueue); !IsNull (&Instance->TaskQueue, Link); ) {\r
+    Task = CR (Link, DISK_IO2_TASK, Link, DISK_IO2_TASK_SIGNATURE);\r
+    if (IsListEmpty (&Task->Subtasks)) {\r
+      Link = RemoveEntryList (&Task->Link);\r
+      ASSERT (Task->Token == NULL);\r
+      FreePool (Task);\r
+    } else {\r
+      Link = GetNextNode (&Instance->TaskQueue, Link);\r
+      QueueEmpty = FALSE;\r
+    }\r
+  }\r
+  EfiReleaseLock (&Instance->TaskQueueLock);\r
+\r
+  return QueueEmpty;\r
+}\r
+\r
 /**\r
   Common routine to access the disk.\r
 \r
@@ -781,12 +817,13 @@ DiskIo2ReadWriteDisk (
   EFI_BLOCK_IO2_PROTOCOL *BlockIo2;\r
   EFI_BLOCK_IO_MEDIA     *Media;\r
   LIST_ENTRY             *Link;\r
+  LIST_ENTRY             *NextLink;\r
   LIST_ENTRY             Subtasks;\r
   DISK_IO_SUBTASK        *Subtask;\r
   DISK_IO2_TASK          *Task;\r
-  BOOLEAN                TaskQueueEmpty;\r
   EFI_TPL                OldTpl;\r
   BOOLEAN                Blocking;\r
+  BOOLEAN                SubtaskBlocking;\r
   LIST_ENTRY             *SubtasksPtr;\r
 \r
   Task      = NULL;\r
@@ -803,24 +840,21 @@ DiskIo2ReadWriteDisk (
   if (Write && Media->ReadOnly) {\r
     return EFI_WRITE_PROTECTED;\r
   }\r
-  \r
+\r
   if (Blocking) {\r
     //\r
     // Wait till pending async task is completed.\r
     //\r
-    do {\r
-      EfiAcquireLock (&Instance->TaskQueueLock);\r
-      TaskQueueEmpty = IsListEmpty (&Instance->TaskQueue);\r
-      EfiReleaseLock (&Instance->TaskQueueLock);\r
-    } while (!TaskQueueEmpty);\r
+    while (!DiskIo2RemoveCompletedTask (Instance));\r
 \r
     SubtasksPtr = &Subtasks;\r
   } else {\r
+    DiskIo2RemoveCompletedTask (Instance);\r
     Task = AllocatePool (sizeof (DISK_IO2_TASK));\r
     if (Task == NULL) {\r
       return EFI_OUT_OF_RESOURCES;\r
     }\r
-    \r
+\r
     EfiAcquireLock (&Instance->TaskQueueLock);\r
     InsertTailList (&Instance->TaskQueue, &Task->Link);\r
     EfiReleaseLock (&Instance->TaskQueueLock);\r
@@ -828,6 +862,7 @@ DiskIo2ReadWriteDisk (
     Task->Signature = DISK_IO2_TASK_SIGNATURE;\r
     Task->Instance  = Instance;\r
     Task->Token     = Token;\r
+    EfiInitializeLock (&Task->SubtasksLock, TPL_NOTIFY);\r
 \r
     SubtasksPtr = &Task->Subtasks;\r
   }\r
@@ -842,12 +877,15 @@ DiskIo2ReadWriteDisk (
   ASSERT (!IsListEmpty (SubtasksPtr));\r
 \r
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);\r
-  for (Link = GetFirstNode (SubtasksPtr); !IsNull (SubtasksPtr, Link); Link = GetNextNode (SubtasksPtr, Link)) {\r
-    Subtask = CR (Link, DISK_IO_SUBTASK, Link, DISK_IO_SUBTASK_SIGNATURE);\r
+  for ( Link = GetFirstNode (SubtasksPtr), NextLink = GetNextNode (SubtasksPtr, Link)\r
+      ; !IsNull (SubtasksPtr, Link)\r
+      ; Link = NextLink, NextLink = GetNextNode (SubtasksPtr, NextLink)\r
+      ) {\r
+    Subtask         = CR (Link, DISK_IO_SUBTASK, Link, DISK_IO_SUBTASK_SIGNATURE);\r
+    Subtask->Task   = Task;\r
+    SubtaskBlocking = Subtask->Blocking;\r
 \r
-    if (!Subtask->Blocking) {\r
-      Subtask->Task = Task;\r
-    }\r
+    ASSERT ((Subtask->Length % Media->BlockSize == 0) || (Subtask->Length < Media->BlockSize));\r
 \r
     if (Subtask->Write) {\r
       //\r
@@ -860,12 +898,12 @@ DiskIo2ReadWriteDisk (
         CopyMem (Subtask->WorkingBuffer + Subtask->Offset, Subtask->Buffer, Subtask->Length);\r
       }\r
 \r
-      if (Subtask->Blocking) {\r
+      if (SubtaskBlocking) {\r
         Status = BlockIo->WriteBlocks (\r
                             BlockIo,\r
                             MediaId,\r
                             Subtask->Lba,\r
-                            Subtask->Length < Media->BlockSize ? Media->BlockSize : Subtask->Length,\r
+                            (Subtask->Length % Media->BlockSize == 0) ? Subtask->Length : Media->BlockSize,\r
                             (Subtask->WorkingBuffer != NULL) ? Subtask->WorkingBuffer : Subtask->Buffer\r
                             );\r
       } else {\r
@@ -874,7 +912,7 @@ DiskIo2ReadWriteDisk (
                              MediaId,\r
                              Subtask->Lba,\r
                              &Subtask->BlockIo2Token,\r
-                             Subtask->Length < Media->BlockSize ? Media->BlockSize : Subtask->Length,\r
+                             (Subtask->Length % Media->BlockSize == 0) ? Subtask->Length : Media->BlockSize,\r
                              (Subtask->WorkingBuffer != NULL) ? Subtask->WorkingBuffer : Subtask->Buffer\r
                              );\r
       }\r
@@ -883,12 +921,12 @@ DiskIo2ReadWriteDisk (
       //\r
       // Read\r
       //\r
-      if (Subtask->Blocking) {\r
+      if (SubtaskBlocking) {\r
         Status = BlockIo->ReadBlocks (\r
                             BlockIo,\r
                             MediaId,\r
                             Subtask->Lba,\r
-                            Subtask->Length < Media->BlockSize ? Media->BlockSize : Subtask->Length,\r
+                            (Subtask->Length % Media->BlockSize == 0) ? Subtask->Length : Media->BlockSize,\r
                             (Subtask->WorkingBuffer != NULL) ? Subtask->WorkingBuffer : Subtask->Buffer\r
                             );\r
         if (!EFI_ERROR (Status) && (Subtask->WorkingBuffer != NULL)) {\r
@@ -900,12 +938,20 @@ DiskIo2ReadWriteDisk (
                              MediaId,\r
                              Subtask->Lba,\r
                              &Subtask->BlockIo2Token,\r
-                             Subtask->Length < Media->BlockSize ? Media->BlockSize : Subtask->Length,\r
+                             (Subtask->Length % Media->BlockSize == 0) ? Subtask->Length : Media->BlockSize,\r
                              (Subtask->WorkingBuffer != NULL) ? Subtask->WorkingBuffer : Subtask->Buffer\r
                              );\r
       }\r
     }\r
-    \r
+\r
+    if (SubtaskBlocking || EFI_ERROR (Status)) {\r
+      //\r
+      // Make sure the subtask list only contains non-blocking subtasks.\r
+      // Remove failed non-blocking subtasks as well because the callback won't be called.\r
+      //\r
+      DiskIoDestroySubtask (Instance, Subtask);\r
+    }\r
+\r
     if (EFI_ERROR (Status)) {\r
       break;\r
     }\r
@@ -918,28 +964,15 @@ DiskIo2ReadWriteDisk (
   // We shouldn't remove all the tasks because the non-blocking requests have been submitted and cannot be canceled.\r
   //\r
   if (EFI_ERROR (Status)) {\r
-    while (!IsNull (SubtasksPtr, Link)) {\r
-      Subtask = CR (Link, DISK_IO_SUBTASK, Link, DISK_IO_SUBTASK_SIGNATURE);\r
-      Link = DiskIoDestroySubtask (Instance, Subtask);\r
+    while (!IsNull (SubtasksPtr, NextLink)) {\r
+      Subtask = CR (NextLink, DISK_IO_SUBTASK, Link, DISK_IO_SUBTASK_SIGNATURE);\r
+      NextLink = DiskIoDestroySubtask (Instance, Subtask);\r
     }\r
   }\r
 \r
   //\r
-  // Remove all the blocking subtasks because the non-blocking callback only removes the non-blocking subtask.\r
-  //\r
-  for (Link = GetFirstNode (SubtasksPtr); !IsNull (SubtasksPtr, Link); ) {\r
-    Subtask = CR (Link, DISK_IO_SUBTASK, Link, DISK_IO_SUBTASK_SIGNATURE);\r
-    if (Subtask->Blocking) {\r
-      Link = DiskIoDestroySubtask (Instance, Subtask);\r
-    } else {\r
-      Link = GetNextNode (SubtasksPtr, Link);\r
-    }\r
-  }\r
-\r
-  //\r
-  // It's possible that the callback runs before raising TPL to NOTIFY,\r
-  // so the subtasks list only contains blocking subtask.\r
-  // Remove the Task after the blocking subtasks are removed in above.\r
+  // It's possible that the non-blocking subtasks finish before raising TPL to NOTIFY,\r
+  // so the subtasks list might be empty at this point.\r
   //\r
   if (!Blocking && IsListEmpty (SubtasksPtr)) {\r
     EfiAcquireLock (&Instance->TaskQueueLock);\r
@@ -1063,6 +1096,8 @@ DiskIo2OnFlushComplete (
   ASSERT (Task->Signature == DISK_IO2_FLUSH_TASK_SIGNATURE);\r
   Task->Token->TransactionStatus = Task->BlockIo2Token.TransactionStatus;\r
   gBS->SignalEvent (Task->Token->Event);\r
+\r
+  FreePool (Task);\r
 }\r
 \r
 /**\r

diff --git a/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.h b/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.h
index 5e89684f66975ee6a7424f56cc3ec49f16bff3e3..7591e469a19b465cb22b12525d11748308d46ecb 100644 (file)
--- a/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.h
+++ b/MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIo.h
@@ -51,6 +51,7 @@ typedef struct {
 typedef struct {\r
   UINT32                          Signature;\r
   LIST_ENTRY                      Link;     /// < link to other task\r
+  EFI_LOCK                        SubtasksLock;\r
   LIST_ENTRY                      Subtasks; /// < header of subtasks\r
   EFI_DISK_IO2_TOKEN              *Token;\r
   DISK_IO_PRIVATE_DATA            *Instance;\r