On 9 November 2016 at 07:59, Leif Lindholm <leif.lindh...@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.zhu...@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;
> }
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
