| Message ID | 1478618476-12608-8-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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);
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