[edk2,v4,07/11] MmcDxe: Fix uninitialized mediaid for SD

Message ID 1478618476-12608-8-git-send-email-haojian.zhuang@linaro.org
State New
Headers show

Commit Message

Haojian Zhuang Nov. 8, 2016, 3:21 p.m.
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Nov. 8, 2016, 11:59 p.m. | #1
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Haojian Zhuang Nov. 13, 2016, 6:46 a.m. | #2
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;

>   }

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch hide | download patch | download mbox

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;
 
 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;
+  if (CurrentMediaId > Media->MediaId)
+    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);