Re: [PATCH] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer

[Jun Nie <jun.nie@linaro.org>]

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
>>