Re: [edk2] [patch v3] MdeModulePkg/SdMmc: update TPL to notify to fix UEFI SCT hang

2016-06-26 Thread Wu, Hao A 
The patch is good to me.

Reviewed-by: Hao Wu 

Best Regards,
Hao Wu

> -Original Message-
> From: Tian, Feng
> Sent: Friday, June 24, 2016 3:25 PM
> To: Wu, Hao A
> Cc: edk2-devel@lists.01.org
> Subject: [patch v3] MdeModulePkg/SdMmc: update TPL to notify to fix UEFI SCT
> hang
> 
> Compared with v2, we add missing critical region protection to error handling
> path of SdRwSingleBlock & SdRwMultiBlocks functions.
> 
> We also fix an issue found at v1 & v2, in which the enumeration timer should
> work at TPL_CALLBACK level as DiskIo is required to be <= TPL_CALLBACK.
> 
> Compared with v1, we added critical region protection on queue access made
> in EraseBlock support.
> 
> We have to upgrade the TPL level used by SdMmc stack because the
> following flow:
> 
> DiskIo2ReadWriteDisk() in logical partition -> PartitionReadBlocksEx()
> in logical partition at TPL callback level -> ProbeMediaStatusEx()
> with sync request -> DiskIo2ReadWriteDisk() in physical partition ->
>  waiting for async task completion.
> 
> if the low layer driver doesn't run at TPL_NOTIFY level, it will have
> no time to trigger async task and cause system hang.
> 
> Cc: Hao Wu 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Feng Tian 
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  9 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  2 +-
>  MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c  | 60 +--
> ---
>  MdeModulePkg/Bus/Sd/SdDxe/SdBlockIo.c  | 42 +++
>  MdeModulePkg/Bus/Sd/SdDxe/SdDxe.c  |  2 +-
>  5 files changed, 75 insertions(+), 40 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index ed6b557..0be081d 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -238,6 +238,7 @@ SdMmcPciHcEnumerateDevice (
>LIST_ENTRY  *Link;
>LIST_ENTRY  *NextLink;
>SD_MMC_HC_TRB   *Trb;
> +  EFI_TPL OldTpl;
> 
>Private = (SD_MMC_HC_PRIVATE_DATA*)Context;
> 
> @@ -251,6 +252,7 @@ SdMmcPciHcEnumerateDevice (
>  //
>  // Signal all async task events at the slot with EFI_NO_MEDIA status.
>  //
> +OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>  for (Link = GetFirstNode (>Queue);
>   !IsNull (>Queue, Link);
>   Link = NextLink) {
> @@ -263,6 +265,7 @@ SdMmcPciHcEnumerateDevice (
>  SdMmcFreeTrb (Trb);
>}
>  }
> +gBS->RestoreTPL (OldTpl);
>  //
>  // Notify the upper layer the connect state change through
> ReinstallProtocolInterface.
>  //
> @@ -665,7 +668,7 @@ SdMmcPciHcDriverBindingStart (
>//
>Status = gBS->CreateEvent (
>EVT_TIMER | EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  TPL_NOTIFY,
>ProcessAsyncTaskList,
>Private,
>>TimerEvent
> @@ -961,7 +964,7 @@ SdMmcPassThruPassThru (
>// Wait async I/O list is empty before execute sync I/O operation.
>//
>while (TRUE) {
> -OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> +OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>  if (IsListEmpty (>Queue)) {
>gBS->RestoreTPL (OldTpl);
>break;
> @@ -1273,7 +1276,7 @@ SdMmcPassThruResetDevice (
>//
>// Free all async I/O requests in the queue
>//
> -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> 
>for (Link = GetFirstNode (>Queue);
> !IsNull (>Queue, Link);
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 8978182..b4ff2af 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -1315,7 +1315,7 @@ SdMmcCreateTrb (
>}
> 
>if (Event != NULL) {
> -OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> +OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>  InsertTailList (>Queue, >TrbList);
>  gBS->RestoreTPL (OldTpl);
>}
> diff --git a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> index 5fe710d..fc705e1 100644
> --- a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> +++ b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> @@ -339,7 +339,7 @@ EmmcSetExtCsd (
>}
> 
>SetExtCsdReq->Signature = EMMC_REQUEST_SIGNATURE;
> -  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>InsertTailList (>Queue, >Link);
>gBS->RestoreTPL (OldTpl);
>SetExtCsdReq->Packet.SdMmcCmdBlk= >SdMmcCmdBlk;
> @@ -361,7 +361,7 @@ EmmcSetExtCsd (
>if ((Token != NULL) && (Token->Event != NULL)) {
>

[edk2] [patch v3] MdeModulePkg/SdMmc: update TPL to notify to fix UEFI SCT hang

2016-06-24 Thread Feng Tian 
Compared with v2, we add missing critical region protection to error handling
path of SdRwSingleBlock & SdRwMultiBlocks functions.

We also fix an issue found at v1 & v2, in which the enumeration timer should
work at TPL_CALLBACK level as DiskIo is required to be <= TPL_CALLBACK.

Compared with v1, we added critical region protection on queue access made
in EraseBlock support.

We have to upgrade the TPL level used by SdMmc stack because the
following flow:

DiskIo2ReadWriteDisk() in logical partition -> PartitionReadBlocksEx()
in logical partition at TPL callback level -> ProbeMediaStatusEx()
with sync request -> DiskIo2ReadWriteDisk() in physical partition ->
 waiting for async task completion.

if the low layer driver doesn't run at TPL_NOTIFY level, it will have
no time to trigger async task and cause system hang.

Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Feng Tian 
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  9 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  2 +-
 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c  | 60 +-
 MdeModulePkg/Bus/Sd/SdDxe/SdBlockIo.c  | 42 +++
 MdeModulePkg/Bus/Sd/SdDxe/SdDxe.c  |  2 +-
 5 files changed, 75 insertions(+), 40 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index ed6b557..0be081d 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -238,6 +238,7 @@ SdMmcPciHcEnumerateDevice (
   LIST_ENTRY  *Link;
   LIST_ENTRY  *NextLink;
   SD_MMC_HC_TRB   *Trb;
+  EFI_TPL OldTpl;
 
   Private = (SD_MMC_HC_PRIVATE_DATA*)Context;
 
@@ -251,6 +252,7 @@ SdMmcPciHcEnumerateDevice (
 //
 // Signal all async task events at the slot with EFI_NO_MEDIA status.
 //
+OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
 for (Link = GetFirstNode (>Queue);
  !IsNull (>Queue, Link);
  Link = NextLink) {
@@ -263,6 +265,7 @@ SdMmcPciHcEnumerateDevice (
 SdMmcFreeTrb (Trb);
   }
 }
+gBS->RestoreTPL (OldTpl);
 //
 // Notify the upper layer the connect state change through 
ReinstallProtocolInterface.
 //
@@ -665,7 +668,7 @@ SdMmcPciHcDriverBindingStart (
   //
   Status = gBS->CreateEvent (
   EVT_TIMER | EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  TPL_NOTIFY,
   ProcessAsyncTaskList,
   Private,
   >TimerEvent
@@ -961,7 +964,7 @@ SdMmcPassThruPassThru (
   // Wait async I/O list is empty before execute sync I/O operation.
   //
   while (TRUE) {
-OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
 if (IsListEmpty (>Queue)) {
   gBS->RestoreTPL (OldTpl);
   break;
@@ -1273,7 +1276,7 @@ SdMmcPassThruResetDevice (
   //
   // Free all async I/O requests in the queue
   //
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
 
   for (Link = GetFirstNode (>Queue);
!IsNull (>Queue, Link);
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 8978182..b4ff2af 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1315,7 +1315,7 @@ SdMmcCreateTrb (
   }
 
   if (Event != NULL) {
-OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
 InsertTailList (>Queue, >TrbList);
 gBS->RestoreTPL (OldTpl);
   }
diff --git a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c 
b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
index 5fe710d..fc705e1 100644
--- a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
+++ b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
@@ -339,7 +339,7 @@ EmmcSetExtCsd (
   }
 
   SetExtCsdReq->Signature = EMMC_REQUEST_SIGNATURE;
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
   InsertTailList (>Queue, >Link);
   gBS->RestoreTPL (OldTpl);
   SetExtCsdReq->Packet.SdMmcCmdBlk= >SdMmcCmdBlk;
@@ -361,7 +361,7 @@ EmmcSetExtCsd (
   if ((Token != NULL) && (Token->Event != NULL)) {
 Status = gBS->CreateEvent (
 EVT_NOTIFY_SIGNAL,
-TPL_CALLBACK,
+TPL_NOTIFY,
 AsyncIoCallback,
 SetExtCsdReq,
 >Event
@@ -382,7 +382,7 @@ Error:
 // The request and event will be freed in asynchronous callback for 
success case.
 //
 if (EFI_ERROR (Status) && (SetExtCsdReq != NULL)) {
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
   RemoveEntryList (>Link);