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

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

2016-06-23 Thread Tian, Feng

thanks for your findings.

Yes. It's mistake when I added EraseBlock support. I will update it at v2.

Thanks
Feng

-Original Message-
From: Wu, Hao A 
Sent: Friday, June 24, 2016 11:27 AM
To: Tian, Feng 
Cc: edk2-devel@lists.01.org
Subject: RE: [patch] MdeModulePkg/SdMmc: update TPL to notify to fix UEFI SCT 
hang

Hi Feng,

I found that some "RemoveEntryList" calls that modifies SD/EMMC device's queue 
are not guarded by raising the TPL.

I think most of them are related with EraseBlock feature functions in file 
EmmcBlockIo.c & SdBlockIo.c.

Could you help to double confirm and add the missing guards?

Best Regards,
Hao Wu

> -Original Message-
> From: Tian, Feng
> Sent: Thursday, June 23, 2016 3:53 PM
> To: Wu, Hao A
> Cc: edk2-devel@lists.01.org
> Subject: [patch] MdeModulePkg/SdMmc: update TPL to notify to fix UEFI 
> SCT hang
> 
> 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 |  8 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  2 +-
>  MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c  | 48 +++-
> --
>  MdeModulePkg/Bus/Sd/SdDxe/SdBlockIo.c  | 22 +-
>  MdeModulePkg/Bus/Sd/SdDxe/SdDxe.c  |  2 +-
>  5 files changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index ed6b557..8716fcd 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -665,7 +665,7 @@ SdMmcPciHcDriverBindingStart (
>//
>Status = gBS->CreateEvent (
>EVT_TIMER | EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  TPL_NOTIFY,
>ProcessAsyncTaskList,
>Private,
>>TimerEvent @@ -684,7 +684,7 @@ 
> SdMmcPciHcDriverBindingStart (
>//
>Status = gBS->CreateEvent (
>EVT_TIMER | EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  TPL_NOTIFY,
>SdMmcPciHcEnumerateDevice,
>Private,
>>ConnectEvent @@ -961,7 +961,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 +1273,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..a4c6053 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)

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

2016-06-23 Thread Wu, Hao A

Hi Feng,

I found that some "RemoveEntryList" calls that modifies SD/EMMC device's
queue are not guarded by raising the TPL.

I think most of them are related with EraseBlock feature functions in file
EmmcBlockIo.c & SdBlockIo.c.

Could you help to double confirm and add the missing guards?

Best Regards,
Hao Wu

> -Original Message-
> From: Tian, Feng
> Sent: Thursday, June 23, 2016 3:53 PM
> To: Wu, Hao A
> Cc: edk2-devel@lists.01.org
> Subject: [patch] MdeModulePkg/SdMmc: update TPL to notify to fix UEFI SCT
> hang
> 
> 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 |  8 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  2 +-
>  MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c  | 48 +++-
> --
>  MdeModulePkg/Bus/Sd/SdDxe/SdBlockIo.c  | 22 +-
>  MdeModulePkg/Bus/Sd/SdDxe/SdDxe.c  |  2 +-
>  5 files changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index ed6b557..8716fcd 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -665,7 +665,7 @@ SdMmcPciHcDriverBindingStart (
>//
>Status = gBS->CreateEvent (
>EVT_TIMER | EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  TPL_NOTIFY,
>ProcessAsyncTaskList,
>Private,
>>TimerEvent
> @@ -684,7 +684,7 @@ SdMmcPciHcDriverBindingStart (
>//
>Status = gBS->CreateEvent (
>EVT_TIMER | EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  TPL_NOTIFY,
>SdMmcPciHcEnumerateDevice,
>Private,
>>ConnectEvent
> @@ -961,7 +961,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 +1273,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..a4c6053 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);
>gBS->RestoreTPL (OldTpl);
>if (SetExtCsdReq->Event != NULL) {
> @@ -395,7 +395,7 @@ Error:
>  // For synchronous operation, free request whatever the execution result 
> is.
>

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

2016-06-23 Thread Feng Tian

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 |  8 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  2 +-
 MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c  | 48 +++---
 MdeModulePkg/Bus/Sd/SdDxe/SdBlockIo.c  | 22 +-
 MdeModulePkg/Bus/Sd/SdDxe/SdDxe.c  |  2 +-
 5 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index ed6b557..8716fcd 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -665,7 +665,7 @@ SdMmcPciHcDriverBindingStart (
   //
   Status = gBS->CreateEvent (
   EVT_TIMER | EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  TPL_NOTIFY,
   ProcessAsyncTaskList,
   Private,
   >TimerEvent
@@ -684,7 +684,7 @@ SdMmcPciHcDriverBindingStart (
   //
   Status = gBS->CreateEvent (
   EVT_TIMER | EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  TPL_NOTIFY,
   SdMmcPciHcEnumerateDevice,
   Private,
   >ConnectEvent
@@ -961,7 +961,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 +1273,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..a4c6053 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);
   gBS->RestoreTPL (OldTpl);
   if (SetExtCsdReq->Event != NULL) {
@@ -395,7 +395,7 @@ Error:
 // For synchronous operation, free request whatever the execution result 
is.
 //
 if (SetExtCsdReq != NULL) {
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
   RemoveEntryList (>Link);
   gBS->RestoreTPL (OldTpl);
   FreePool (SetExtCsdReq);
@@ -445,7 +445,7 @@ EmmcSetBlkCount (
   }
 
   SetBlkCntReq->Signature = EMMC_REQUEST_SIGNATURE;
-  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
   InsertTailList (>Queue, >Link);
   gBS->RestoreTPL (OldTpl);
   SetBlkCntReq->Packet.SdMmcCmdBlk= >SdMmcCmdBlk;
@@ -463,7 +463,7 @@ EmmcSetBlkCount (
   if ((Token != NULL) && (Token->Event != NULL)) {
 Status = gBS->CreateEvent (
 EVT_NOTIFY_SIGNAL,
-TPL_CALLBACK,
+TPL_NOTIFY,

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information