Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Andrew Fish <afish@apple.com>]

> On Jun 12, 2017, at 9:13 PM, Jun Nie <jun.nie@linaro.org> wrote:
> 
> 2017-06-13 12:01 GMT+08:00 Andrew Fish <afish@apple.com <mailto:afish@apple.com>>:
>> 
>>> On Jun 12, 2017, at 7:14 PM, Jun Nie <jun.nie@linaro.org> wrote:
>>> 
>>> 2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
>>>> On Mon, Jun 12, 2017 at 09:59:28AM +0800, Jun Nie wrote:
>>>>> Add alignment for ECSD data for DMA access. Otherwise
>>>>> the data is corrupted on Sanechips platform.
>>>> 
>>>> I never did see a reply to my proposed solution, and the below is not
>>>> it. Can you explain why you prefer this one?
>>>> 
>>>> /
>>>>   Leif
>>> 
>>> Sorry, just see your email because that thread is not highlighted for
>>> new email in gmail for unknown reason.
>>> I have concern that "UINT64 VENDOR_SPECIFIC_FIELD[8]" cannot secure
>>> the ECSD alignment because it is not the first member. Changing the
>>> first member to "UINT64 RESERVED_1[2]" shall secure the alignment. But
>>> I preferred Pad method. It is more readable if all ECSD member are
>>> UINT8 type. It is also more clear to add alignment info in CARD_INFO,
>>> just before ECSD member.
>>> I do not get point of Andrew, maybe he share the same concern.
>>> 
>> 
>> Jun
>> 
>> typedef enum {
>>  UNKNOWN_CARD,
>>  MMC_CARD,              //MMC card
>>  MMC_CARD_HIGH,         //MMC Card with High capacity
>>  EMMC_CARD,             //eMMC 4.41 card
>>  SD_CARD,               //SD 1.1 card
>>  SD_CARD_2,             //SD 2.0 or above standard card
>>  SD_CARD_2_HIGH         //SD 2.0 or above high capacity card
>> } CARD_TYPE;
>> 
>> Per C spec sizeof(CARD_TYPE) can be 1, 2, 4, or 8 (64-bit integer), and it is legal for the compiler to pick any of these. So it is not portable C code to use an enum in a data structure when layout maters.
>> 
>> Thanks,
>> 
>> Andrew Fish
> 
> CARD_TYPE CardType is 2nd member of CARD_INFO, while ECSDData is the
> last member and I just want to align it to 8 bytes. I had assume pad
> will be added automatically by compiler if CARD_TYPE is not 8 bytes
> aligned and UNIT64 type appear in following member. Does enum will
> impact the later member alignment? Could you help elaborate more about
> this?
> 

Sure

type struct {
  UINT16 RCA;
  UINT8   CardType;
  UINT32 OCRData;
...

Has different alignment than:
type struct {
  UINT16 RCA;
  UINT32 CardType;
  UINT32 OCRData;
...

Both are legal things for the C compiler to due given the type is an enum. 
1st example OCRData starts at offset 4
2nd example OCRData starts at offset 8

An integer type is not an int. 

Thanks,

Andrew Fish

> Thank you!
> Jun
> 
>>> 
>>>> 
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>>> ---
>>>>> EmbeddedPkg/Universal/MmcDxe/Mmc.h | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>> 
>>>>> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
>>>>> index 8a7d5a3..6e3ab17 100644
>>>>> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
>>>>> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
>>>>> @@ -319,6 +319,7 @@ typedef struct  {
>>>>>  OCR       OCRData;
>>>>>  CID       CIDData;
>>>>>  CSD       CSDData;
>>>>> +  UINT64    Pad;                              // For 8 bytes alignment of ECSDData
>>>>>  ECSD      ECSDData;                         // MMC V4 extended card specific
>>>>> } CARD_INFO;
>>>>> 
>>>>> --
>>>>> 1.9.1
>>>>> 
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>