From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x235.google.com (mail-qk0-x235.google.com [IPv6:2607:f8b0:400d:c09::235]) (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 E0A4F21A00AD5 for ; Thu, 29 Jun 2017 07:32:08 -0700 (PDT) Received: by mail-qk0-x235.google.com with SMTP id r62so77719979qkf.0 for ; Thu, 29 Jun 2017 07:33:41 -0700 (PDT) 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=mQr0UorqZ+76GmCM+T+q71oHcAjxt62t8Qal+Zu0fA4=; b=aQtvKGSE1YVCwbXSvHAs0fPo4nYcpD1zi7i85D1Bo7lX1v4HH09kL/N0BpZPHYwyCc /jwXilltq8JdY6DfRNNXa4hCi+MsHNBBuMRZc3dQi0F2jaQKlOiMp60tDmAF+25LCbGh 1ldvOxWCExvF1aWstiKIXG+HwHm6n+PAOMg68= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=mQr0UorqZ+76GmCM+T+q71oHcAjxt62t8Qal+Zu0fA4=; b=LWAICmYjo8vAelRl14SJn1B8O9GxXqTpfbU2BfrrLj91+9i+nxDL5ugCKekSPJUl5g gY1FPJW+yOFv8wgo4KyXyufBgSoylZSiwRGWH0i9uFPWtr2gTSHaxDY9YZ5EVuJmMgjP jLfvfJtH1/ToR9O/0x0fHXujpaTKi7Omcvo779DE1Ojgb6TsGMrK2LwW5uYaamsP1kLw IL6G+xmGzC8IUtDmQAbzNRpk35N9yZUgerHPiZNPq9JDdZL/V92Ee6XXMoXI1ghxLw8r GnEBxt52FLNb5dEFNtB4l7xclIih5phx6APOQsyhElqZVvZjwMeDUeCtVrUDk3/Z8+ii McSg== X-Gm-Message-State: AKS2vOyYg3VW/hlIjm8keZD6w+87R/WmTc0ozHWW0AItIHT3haDtvPeQ OwfT4Q8+1re8TPJTk3GqBg4jp/7rb+gj X-Received: by 10.55.3.139 with SMTP id 133mr18854703qkd.65.1498746820504; Thu, 29 Jun 2017 07:33:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.55.110.66 with HTTP; Thu, 29 Jun 2017 07:33:40 -0700 (PDT) In-Reply-To: <20170629111114.GR26676@bivouac.eciton.net> References: <1498727030-26410-1-git-send-email-jun.nie@linaro.org> <20170629111114.GR26676@bivouac.eciton.net> From: Jun Nie Date: Thu, 29 Jun 2017 22:33:40 +0800 Message-ID: To: Leif Lindholm Cc: Ard Biesheuvel , Haojian Zhuang , edk2-devel@lists.01.org, Shawn Guo , Jason Liu Subject: Re: [PATCH] EmbeddedPkg/MmcDxe: Align the ExtCSD buffer X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Jun 2017 14:32:09 -0000 Content-Type: text/plain; charset="UTF-8" 2017-06-29 19:11 GMT+08:00 Leif Lindholm : > On Thu, Jun 29, 2017 at 05:03:50PM +0800, Jun Nie wrote: >> ExtCSD structure may be read via DMA. So align it to >> page to avoid data corruption. > > So, this is possibly a valid thing to do, but this is not what you > said in the last version. The last version said you needed 64-bit > alignment, and this is already guaranteed by all of the allocation > functions. Yes, I need 64 bytes alignment. page alignment is better, right? :-) > >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jun Nie >> --- >> EmbeddedPkg/Universal/MmcDxe/Mmc.h | 2 +- >> EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 11 ++++++++--- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h >> index 8a7d5a3..f3e56ff 100644 >> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h >> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h >> @@ -319,7 +319,7 @@ typedef struct { >> OCR OCRData; >> CID CIDData; >> CSD CSDData; >> - ECSD ECSDData; // MMC V4 extended card specific >> + ECSD *ECSDData; // MMC V4 extended card specific >> } CARD_INFO; >> >> typedef struct _MMC_HOST_INSTANCE { >> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c >> index c28207e..6bb65c3 100644 >> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c >> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c >> @@ -13,6 +13,7 @@ >> **/ >> >> #include >> +#include >> #include >> >> #include "Mmc.h" >> @@ -210,12 +211,16 @@ EmmcIdentificationMode ( >> } >> >> // Fetch ECSD >> + MmcHostInstance->CardInfo.ECSDData = AllocatePages (1); > > No magic numbers please. > Suggest EFI_SIZE_TO_PAGES (sizeof (ECSD)); > Or AllocatePool (EFI_SIZE_TO_PAGES (sizeof (ECSD))); You are right. > > I'm not seeing a balancing FreePages call in this patch. > My guess would be that DestroyMmcHostInstance() would be the > correct location for it. Add FreePages shall be more clear. Will do in next version. > >> + if (MmcHostInstance->CardInfo.ECSDData == NULL) { >> + return EFI_BUFFER_TOO_SMALL; > > I think EFI_OUT_OF_RESOURCES would be more appropriate. Will do. > > / > Leif > >> + } >> Status = Host->SendCommand (Host, MMC_CMD8, 0); >> if (EFI_ERROR (Status)) { >> DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status)); >> } >> >> - Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData)); >> + Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)MmcHostInstance->CardInfo.ECSDData); >> if (EFI_ERROR (Status)) { >> DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status)); >> return Status; >> @@ -237,7 +242,7 @@ EmmcIdentificationMode ( >> Media->LogicalBlocksPerPhysicalBlock = 1; >> Media->IoAlign = 4; >> // Compute last block using bits [215:212] of the ECSD >> - Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // eMMC isn't supposed to report this for >> + Media->LastBlock = MmcHostInstance->CardInfo.ECSDData->SECTOR_COUNT - 1; // eMMC isn't supposed to report this for >> // Cards <2GB in size, but the model does. >> >> // Setup card type >> @@ -258,7 +263,7 @@ InitializeEmmcDevice ( >> UINT32 TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, EMMCHS26}; >> >> Host = MmcHostInstance->MmcHost; >> - ECSDData = &MmcHostInstance->CardInfo.ECSDData; >> + ECSDData = MmcHostInstance->CardInfo.ECSDData; >> if (ECSDData->DEVICE_TYPE == EMMCBACKWARD) >> return EFI_SUCCESS; >> >> -- >> 1.9.1 >>