From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22f.google.com (mail-wm0-x22f.google.com [IPv6:2a00:1450:400c:c09::22f]) (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 5804B81D4F for ; Tue, 8 Nov 2016 15:59:51 -0800 (PST) Received: by mail-wm0-x22f.google.com with SMTP id f82so211964211wmf.1 for ; Tue, 08 Nov 2016 15:59:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=wle5WBL16q+p6zQIcSZhCPlxmJ9tjlUg/zu2nFLji/c=; b=d6tJ8B2TuNPne/A0+6I7P2GVhPjqP39qIwAYapoxzq0wlBNuzPtCdj3MCqkSl3xPHb b7vHOrhP3F/84V6PA5+vZrbvXeR437yv/fv/AqNxEnvi5ntAiAB+SgmPcsaEYUO3tCWH a7Fl/kJxNdKEYjsO6OcVi0d6pCWyxg0LtCYm0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=wle5WBL16q+p6zQIcSZhCPlxmJ9tjlUg/zu2nFLji/c=; b=KtFKBLnMB3+yXulgekks+1JwT+acdvV98QMWwEwXx093WpowojvXq9zq9o9BU9wSvF yzLA4G4rLbkrmQZ17QBYHc0eWYpuKjAaLdiMmF9hfRzB6fqb3HRd7tPm8eZziRxXVDU+ ZcpJNjIOUAHCbl66LcE/v9mFmW9L5cQAlVwBTho4fy/3VGtCn3vL3ZDWI/mfbQ+9HolX T+RbO0KEzuB09aaLCAvCMzoHGlaJ1kDsR+nXIqErh9UdwdIjyyvogooS0/PepNm7bGNL uJIMErnyv2C+Tg2QcyRsSo7lg2xWVDOj9cD51ZhBxlp/mNUl1Wvowmkm2HoCRwmYWTJW B+bg== X-Gm-Message-State: ABUngvc0sF0avw7RJaL7dO9tglSCIEZw2hECB5WcfwpnCvNhxdKM4qaUAKOQqwhrsGUulgSw X-Received: by 10.28.168.137 with SMTP id r131mr18351564wme.16.1478649593194; Tue, 08 Nov 2016 15:59:53 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id z6sm30780007wjt.24.2016.11.08.15.59.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Nov 2016 15:59:52 -0800 (PST) Date: Tue, 8 Nov 2016 23:59:50 +0000 From: Leif Lindholm To: Haojian Zhuang Cc: ryan.harkin@linaro.org, edk2-devel@lists.01.org, ard.biesheuvel@linaro.org Message-ID: <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> MIME-Version: 1.0 In-Reply-To: <1478618476-12608-8-git-send-email-haojian.zhuang@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) 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: Tue, 08 Nov 2016 23:59:51 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? 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 >