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

[Leif Lindholm <leif.lindholm@linaro.org>]

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?

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

>    Media->ReadOnly = MmcHostInstance->CardInfo.CSDData.PERM_WRITE_PROTECT;
>    Media->LogicalBlocksPerPhysicalBlock = 1;
>    Media->IoAlign = 4;
> @@ -344,7 +349,7 @@ InitializeSdMmcDevice (
>    MmcHostInstance->BlockIo.Media->BlockSize    = BlockSize;
>    MmcHostInstance->BlockIo.Media->ReadOnly     = MmcHost->IsReadOnly (MmcHost);
>    MmcHostInstance->BlockIo.Media->MediaPresent = TRUE;
> -  MmcHostInstance->BlockIo.Media->MediaId++;
> +  MmcHostInstance->BlockIo.Media->MediaId      = ++CurrentMediaId;
>  
>    CmdArg = MmcHostInstance->CardInfo.RCA << 16;
>    Status = MmcHost->SendCommand (MmcHost, MMC_CMD7, CmdArg);
> -- 
> 2.7.4
>