Re: [edk2] [PATCH] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer
2017-06-29 19:11 GMT+08:00 Leif Lindholm :
> 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
>> ---
>> 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
>> +#include
>> #include
>>
>> #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
Re: [edk2] [PATCH] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer
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
> ---
> 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
> +#include
> #include
>
> #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
[edk2] [PATCH] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer
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
---
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
+#include
#include
#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;
--
1.9.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
