[PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation

[PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation

[Ard Biesheuvel]

Currently, we complete a synchronous operation without unmapping the
DMA mappings, and free the pages using FreePages () rather than calling
EFI_PCI_IO_PROTOCOL::FreeBuffer. This is simply incorrecnt, but it also
breaks non-coherent DMA as well as DMA protection and/or memory encryption
so let's do it correctly and call SdMmcFreeTrb() instead.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index 23faec5e2be0..0be8828abfcc 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -1008,13 +1008,7 @@ SdMmcPassThruPassThru (
   }
 
 Done:
-  if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
-    FreePages (Trb->AdmaDesc, Trb->AdmaPages);
-  }
-
-  if (Trb != NULL) {
-    FreePool (Trb);
-  }
+  SdMmcFreeTrb (Trb);
 
   return Status;
 }
-- 
2.11.0

Re: [PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation

[Wu, Hao A]

Reviewed-by: Hao Wu <hao.a.wu@intel.com>


Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Sunday, November 05, 2017 5:31 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; Dong, Eric; Zeng, Star; Ard Biesheuvel
> Subject: [edk2] [PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call
> SdMmcFreeTrb() to complete sync operation
> 
> Currently, we complete a synchronous operation without unmapping the
> DMA mappings, and free the pages using FreePages () rather than calling
> EFI_PCI_IO_PROTOCOL::FreeBuffer. This is simply incorrecnt, but it also
> breaks non-coherent DMA as well as DMA protection and/or memory
> encryption
> so let's do it correctly and call SdMmcFreeTrb() instead.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 23faec5e2be0..0be8828abfcc 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -1008,13 +1008,7 @@ SdMmcPassThruPassThru (
>    }
> 
>  Done:
> -  if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
> -    FreePages (Trb->AdmaDesc, Trb->AdmaPages);
> -  }
> -
> -  if (Trb != NULL) {
> -    FreePool (Trb);
> -  }
> +  SdMmcFreeTrb (Trb);
> 
>    return Status;
>  }
> --
> 2.11.0
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation

[Zeng, Star]

Except the typo "incorrecnt" needs to be "incorrect" in commit log, others are good to me.

With typo fixed, Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Sunday, November 5, 2017 5:31 PM
To: edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation

Currently, we complete a synchronous operation without unmapping the DMA mappings, and free the pages using FreePages () rather than calling EFI_PCI_IO_PROTOCOL::FreeBuffer. This is simply incorrecnt, but it also breaks non-coherent DMA as well as DMA protection and/or memory encryption so let's do it correctly and call SdMmcFreeTrb() instead.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index 23faec5e2be0..0be8828abfcc 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -1008,13 +1008,7 @@ SdMmcPassThruPassThru (
   }
 
 Done:
-  if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
-    FreePages (Trb->AdmaDesc, Trb->AdmaPages);
-  }
-
-  if (Trb != NULL) {
-    FreePool (Trb);
-  }
+  SdMmcFreeTrb (Trb);
 
   return Status;
 }
--
2.11.0

Re: [PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation

[Ard Biesheuvel]

On 6 November 2017 at 08:56, Zeng, Star <star.zeng@intel.com> wrote:
> Except the typo "incorrecnt" needs to be "incorrect" in commit log, others are good to me.
>
> With typo fixed, Reviewed-by: Star Zeng <star.zeng@intel.com>
>

Thanks

Pushed as 6743455e34d1b313d644d9f7ca726b9932effb1f


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Sunday, November 5, 2017 5:31 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH v2] MdeModulePkg/SdMmcPciHcDxe: call SdMmcFreeTrb() to complete sync operation
>
> Currently, we complete a synchronous operation without unmapping the DMA mappings, and free the pages using FreePages () rather than calling EFI_PCI_IO_PROTOCOL::FreeBuffer. This is simply incorrecnt, but it also breaks non-coherent DMA as well as DMA protection and/or memory encryption so let's do it correctly and call SdMmcFreeTrb() instead.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 23faec5e2be0..0be8828abfcc 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -1008,13 +1008,7 @@ SdMmcPassThruPassThru (
>    }
>
>  Done:
> -  if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
> -    FreePages (Trb->AdmaDesc, Trb->AdmaPages);
> -  }
> -
> -  if (Trb != NULL) {
> -    FreePool (Trb);
> -  }
> +  SdMmcFreeTrb (Trb);
>
>    return Status;
>  }
> --
> 2.11.0
>