From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x233.google.com (mail-qk0-x233.google.com [IPv6:2607:f8b0:400d:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4809481DB6 for ; Sat, 12 Nov 2016 22:46:10 -0800 (PST) Received: by mail-qk0-x233.google.com with SMTP id n204so64458231qke.2 for ; Sat, 12 Nov 2016 22:46:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=dg+U3N2mKrtYY3EEj97pij+we4tks8Go07kEC5YptW4=; b=XdsvuXv7wITrVMPCnoygEBBV8RHwD5mxEi2Xs9N02vFhjDjz+ouD3YLZkH8VYJfsT+ oL7DEI7iU1Q5E7t7iscQ1RhrSL8zDfWf29rD1MybUe640QX2B33e/JjD9J+7NLzD73Ey qBxQFxANWfoiuWGmV1zGPdOGJsgTLCsCYQMZA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=dg+U3N2mKrtYY3EEj97pij+we4tks8Go07kEC5YptW4=; b=aC2gJfNejC07f/9K8tQRcYVJBXv+AlG6sBWgMxzCEXpmbc6k5/EJolZywMX+gi8mRc 7xgmqjQeAKgTFzQozjb2DA1rIN1/rl184d90kJSyPb5+RJc931HTwvtUBCKHgP+29D7R WlMYHoZYaRKEVhL1BVQ4XjIlqsRQmYuVTSG2A3pWVqlsAw81IZk3ews1LN9TPfT8YzOn KR2pAAZ3SeJzj9ApxB2plJrsY38Vt1Ndxji8Yg591I08RSBCZMzbVz/c/yE5Sre7tbNm VJe4IAwSBUKBLecSU2XA2OdalFKpBH5QY57iR6E6DE4GJo5RJRxPged+PVeuvHfKOTth Gfhw== X-Gm-Message-State: ABUngvftx4jONFVQmffEfkOJoe/HYqob/PuJNNm6D1KK4nZoRaCdKsmROBuKjUTB0v3C3KktyOeBhGRXwJ3+M7jE X-Received: by 10.55.146.130 with SMTP id u124mr12523529qkd.176.1479019573531; Sat, 12 Nov 2016 22:46:13 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.85.165 with HTTP; Sat, 12 Nov 2016 22:46:13 -0800 (PST) In-Reply-To: <20161108235950.GM27644@bivouac.eciton.net> References: <1478618476-12608-1-git-send-email-haojian.zhuang@linaro.org> <1478618476-12608-8-git-send-email-haojian.zhuang@linaro.org> <20161108235950.GM27644@bivouac.eciton.net> From: Haojian Zhuang Date: Sun, 13 Nov 2016 14:46:13 +0800 Message-ID: To: Leif Lindholm Cc: Ryan Harkin , edk2-devel-01 , Ard Biesheuvel Subject: Re: [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 13 Nov 2016 06:46:10 -0000 Content-Type: text/plain; charset=UTF-8 On 9 November 2016 at 07:59, Leif Lindholm 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 >> --- >> 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; > } >