Re: [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD

[Haojian Zhuang <haojian.zhuang@linaro.org>]

On 9 November 2016 at 07:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Nov 08, 2016 at 11:21:12PM +0800, Haojian Zhuang wrote:
>> When SD card is used, mediaid is not initialized and used directly. So
>> fix it.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> index cefc2b6..5b802c0 100644
>> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> @@ -57,6 +57,7 @@ typedef enum _EMMC_DEVICE_STATE {
>>  } EMMC_DEVICE_STATE;
>>
>>  UINT32 mEmmcRcaCount = 0;
>> +UINT32 CurrentMediaId = 0;
>
> Should have an 'm' prefix.
>
>>
>>  STATIC
>>  EFI_STATUS
>> @@ -231,6 +232,10 @@ EmmcIdentificationMode (
>>    // Set up media
>>    Media->BlockSize = EMMC_CARD_SIZE; // 512-byte support is mandatory for eMMC cards
>>    Media->MediaId = MmcHostInstance->CardInfo.CIDData.PSN;
>
> I spent some time digging through this code in order to understand
> what is going on below. I think this could do with a comment
> explaining the logic. Also, always use {} with if/else.
>
>> +  if (CurrentMediaId > Media->MediaId)
>> +    Media->MediaId = ++CurrentMediaId;
>> +  else
>> +    CurrentMediaId = Media->MediaId;
>
> I will give my interpretation of how this works, and you can confirm
> or deny it.
>
> When this Media entity is created, it is based on mMmcMediaTemplate.
> mMmcMediaTemplate (ab)uses the MediaId field as a Signature - "mmco",
> meaning any new MMC device starts with a MediaId of 0x6d6d626f (or
> 0x6f626d6d if I got the endianness wrong).
>
> So the value is initialised, just to the same for every MMC media in
> the system.
>
> Since the module-global (m)CurrentMediaId is initialised to 0, the
> first time MmcIdentificationMode() is called for the first eMMC
> device, (m)CurrentMediaId will be set to 0x6d6d6270 (or 0x6f626d6e).
>
> Does this not leave it uninitialised for MMC and other media types?
>
> I guess it's not a huge deal if the MediaID clashes for different
> devices, because it is mainly meant to indicate a removable media has
> changed. But that also means there should not have been a problem in
> the first place.
>
> So can you describe in the commit message what failure mode this patch
> gets rid of?
>

System won't crash with the patch removed. Let's remove this patch first.

Best Regards
Haojian

> Regardless, I think the code would be more clear in what it is doing
> if it did:
>   if (Media->MediaId == SIGNATURE_32('m','m','c','o')) {
>     Media->MediaId = ++CurrentMediaId;
>   } else {
>     CurrentMediaId = Media->MediaId;
>   }
>