diff mbox

[edk2] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer

Message ID 1498727030-26410-1-git-send-email-jun.nie@linaro.org
State New
Headers show

Commit Message

Jun Nie June 29, 2017, 9:03 a.m. UTC 
ExtCSD structure may be read via DMA. So align it to
page to avoid data corruption.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>

---
 EmbeddedPkg/Universal/MmcDxe/Mmc.h               |  2 +-
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm June 29, 2017, 11:11 a.m. UTC | #1 
On Thu, Jun 29, 2017 at 05:03:50PM +0800, Jun Nie wrote:
> ExtCSD structure may be read via DMA. So align it to

> page to avoid data corruption.


So, this is possibly a valid thing to do, but this is not what you
said in the last version. The last version said you needed 64-bit
alignment, and this is already guaranteed by all of the allocation
functions.

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jun Nie <jun.nie@linaro.org>

> ---

>  EmbeddedPkg/Universal/MmcDxe/Mmc.h               |  2 +-

>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 11 ++++++++---

>  2 files changed, 9 insertions(+), 4 deletions(-)

> 

> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> index 8a7d5a3..f3e56ff 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> @@ -319,7 +319,7 @@ typedef struct  {

>    OCR       OCRData;

>    CID       CIDData;

>    CSD       CSDData;

> -  ECSD      ECSDData;                         // MMC V4 extended card specific

> +  ECSD      *ECSDData;                         // MMC V4 extended card specific

>  } CARD_INFO;

>  

>  typedef struct _MMC_HOST_INSTANCE {

> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> index c28207e..6bb65c3 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> @@ -13,6 +13,7 @@

>  **/

>  

>  #include <Library/BaseMemoryLib.h>

> +#include <Library/MemoryAllocationLib.h>

>  #include <Library/TimerLib.h>

>  

>  #include "Mmc.h"

> @@ -210,12 +211,16 @@ EmmcIdentificationMode (

>    }

>  

>    // Fetch ECSD

> +  MmcHostInstance->CardInfo.ECSDData = AllocatePages (1);


No magic numbers please.
Suggest EFI_SIZE_TO_PAGES (sizeof (ECSD));
Or AllocatePool (EFI_SIZE_TO_PAGES (sizeof (ECSD)));

I'm not seeing a balancing FreePages call in this patch.
My guess would be that DestroyMmcHostInstance() would be the
correct location for it.

> +  if (MmcHostInstance->CardInfo.ECSDData == NULL) {

> +    return EFI_BUFFER_TOO_SMALL;


I think EFI_OUT_OF_RESOURCES would be more appropriate.

/
    Leif

> +  }

>    Status = Host->SendCommand (Host, MMC_CMD8, 0);

>    if (EFI_ERROR (Status)) {

>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));

>    }

>  

> -  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));

> +  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)MmcHostInstance->CardInfo.ECSDData);

>    if (EFI_ERROR (Status)) {

>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));

>      return Status;

> @@ -237,7 +242,7 @@ EmmcIdentificationMode (

>    Media->LogicalBlocksPerPhysicalBlock = 1;

>    Media->IoAlign = 4;

>    // Compute last block using bits [215:212] of the ECSD

> -  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // eMMC isn't supposed to report this for

> +  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData->SECTOR_COUNT - 1; // eMMC isn't supposed to report this for

>    // Cards <2GB in size, but the model does.

>  

>    // Setup card type

> @@ -258,7 +263,7 @@ InitializeEmmcDevice (

>    UINT32     TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, EMMCHS26};

>  

>    Host  = MmcHostInstance->MmcHost;

> -  ECSDData = &MmcHostInstance->CardInfo.ECSDData;

> +  ECSDData = MmcHostInstance->CardInfo.ECSDData;

>    if (ECSDData->DEVICE_TYPE == EMMCBACKWARD)

>      return EFI_SUCCESS;

>  

> -- 

> 1.9.1

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Jun Nie June 29, 2017, 2:33 p.m. UTC | #2 
2017-06-29 19:11 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Jun 29, 2017 at 05:03:50PM +0800, Jun Nie wrote:

>> ExtCSD structure may be read via DMA. So align it to

>> page to avoid data corruption.

>

> So, this is possibly a valid thing to do, but this is not what you

> said in the last version. The last version said you needed 64-bit

> alignment, and this is already guaranteed by all of the allocation

> functions.


Yes, I need 64 bytes alignment. page alignment is better, right? :-)

>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Jun Nie <jun.nie@linaro.org>

>> ---

>>  EmbeddedPkg/Universal/MmcDxe/Mmc.h               |  2 +-

>>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 11 ++++++++---

>>  2 files changed, 9 insertions(+), 4 deletions(-)

>>

>> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

>> index 8a7d5a3..f3e56ff 100644

>> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

>> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

>> @@ -319,7 +319,7 @@ typedef struct  {

>>    OCR       OCRData;

>>    CID       CIDData;

>>    CSD       CSDData;

>> -  ECSD      ECSDData;                         // MMC V4 extended card specific

>> +  ECSD      *ECSDData;                         // MMC V4 extended card specific

>>  } CARD_INFO;

>>

>>  typedef struct _MMC_HOST_INSTANCE {

>> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

>> index c28207e..6bb65c3 100644

>> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

>> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

>> @@ -13,6 +13,7 @@

>>  **/

>>

>>  #include <Library/BaseMemoryLib.h>

>> +#include <Library/MemoryAllocationLib.h>

>>  #include <Library/TimerLib.h>

>>

>>  #include "Mmc.h"

>> @@ -210,12 +211,16 @@ EmmcIdentificationMode (

>>    }

>>

>>    // Fetch ECSD

>> +  MmcHostInstance->CardInfo.ECSDData = AllocatePages (1);

>

> No magic numbers please.

> Suggest EFI_SIZE_TO_PAGES (sizeof (ECSD));

> Or AllocatePool (EFI_SIZE_TO_PAGES (sizeof (ECSD)));


You are right.

>

> I'm not seeing a balancing FreePages call in this patch.

> My guess would be that DestroyMmcHostInstance() would be the

> correct location for it.


Add FreePages shall be more clear. Will do in next version.

>

>> +  if (MmcHostInstance->CardInfo.ECSDData == NULL) {

>> +    return EFI_BUFFER_TOO_SMALL;

>

> I think EFI_OUT_OF_RESOURCES would be more appropriate.


Will do.

>

> /

>     Leif

>

>> +  }

>>    Status = Host->SendCommand (Host, MMC_CMD8, 0);

>>    if (EFI_ERROR (Status)) {

>>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));

>>    }

>>

>> -  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));

>> +  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)MmcHostInstance->CardInfo.ECSDData);

>>    if (EFI_ERROR (Status)) {

>>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));

>>      return Status;

>> @@ -237,7 +242,7 @@ EmmcIdentificationMode (

>>    Media->LogicalBlocksPerPhysicalBlock = 1;

>>    Media->IoAlign = 4;

>>    // Compute last block using bits [215:212] of the ECSD

>> -  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // eMMC isn't supposed to report this for

>> +  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData->SECTOR_COUNT - 1; // eMMC isn't supposed to report this for

>>    // Cards <2GB in size, but the model does.

>>

>>    // Setup card type

>> @@ -258,7 +263,7 @@ InitializeEmmcDevice (

>>    UINT32     TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, EMMCHS26};

>>

>>    Host  = MmcHostInstance->MmcHost;

>> -  ECSDData = &MmcHostInstance->CardInfo.ECSDData;

>> +  ECSDData = MmcHostInstance->CardInfo.ECSDData;

>>    if (ECSDData->DEVICE_TYPE == EMMCBACKWARD)

>>      return EFI_SUCCESS;

>>

>> --

>> 1.9.1

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
index 8a7d5a3..f3e56ff 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
@@ -319,7 +319,7 @@  typedef struct  {
   OCR       OCRData;
   CID       CIDData;
   CSD       CSDData;
-  ECSD      ECSDData;                         // MMC V4 extended card specific
+  ECSD      *ECSDData;                         // MMC V4 extended card specific
 } CARD_INFO;
 
 typedef struct _MMC_HOST_INSTANCE {
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index c28207e..6bb65c3 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -13,6 +13,7 @@ 
 **/
 
 #include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
 #include <Library/TimerLib.h>
 
 #include "Mmc.h"
@@ -210,12 +211,16 @@  EmmcIdentificationMode (
   }
 
   // Fetch ECSD
+  MmcHostInstance->CardInfo.ECSDData = AllocatePages (1);
+  if (MmcHostInstance->CardInfo.ECSDData == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
   Status = Host->SendCommand (Host, MMC_CMD8, 0);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
   }
 
-  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
+  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)MmcHostInstance->CardInfo.ECSDData);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
     return Status;
@@ -237,7 +242,7 @@  EmmcIdentificationMode (
   Media->LogicalBlocksPerPhysicalBlock = 1;
   Media->IoAlign = 4;
   // Compute last block using bits [215:212] of the ECSD
-  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // eMMC isn't supposed to report this for
+  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData->SECTOR_COUNT - 1; // eMMC isn't supposed to report this for
   // Cards <2GB in size, but the model does.
 
   // Setup card type
@@ -258,7 +263,7 @@  InitializeEmmcDevice (
   UINT32     TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, EMMCHS26};
 
   Host  = MmcHostInstance->MmcHost;
-  ECSDData = &MmcHostInstance->CardInfo.ECSDData;
+  ECSDData = MmcHostInstance->CardInfo.ECSDData;
   if (ECSDData->DEVICE_TYPE == EMMCBACKWARD)
     return EFI_SUCCESS;