From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>
Subject: Re: [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra data is erased by EraseBlocks
Date: Fri, 11 Aug 2017 04:20:54 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A0931CDA83C@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B918177@shsmsx102.ccr.corp.intel.com>
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, August 11, 2017 10:37 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Wu, Hao A; Ni, Ruiyu; Zeng, Star
> Subject: RE: [edk2] [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra
> data is erased by EraseBlocks
>
> Two minor comments, others are good to me.
>
> 1. Could the code use (LastLba + 1 - EndGroupLba) instead of (LastLba -
> EndGroupLba + 1)?
>
> 2. Could the code have the debug message "DEBUG ((EFI_D_ERROR,
> "EmmcEraseBlocks(): Lba 0x%x BlkNo 0x%x Event %p with %r\n", Lba,
> BlockNum, Token->Event, Status))" for the new return paths the patch added?
> And the debug level should be DEBUG_INFO instead of EFI_D_ERROR?
>
Thanks for the feedbacks. I will submit a new version of the patch.
Best Regards,
Hao Wu
>
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao
> Wu
> Sent: Thursday, August 10, 2017 9:21 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra
> data is erased by EraseBlocks
>
> V2 changes:
>
> The Trim command is not supported on all eMMC devices. For those devices
> that do not support such command, add codes to handle the scenario.
>
> Commit message:
>
> The current implementation of the Erase Block Protocol service
> EraseBlocks() uses the erase command. According to spec eMMC Electrical
> Standard 5.1, Section 6.6.9:
>
> The erasable unit of the eMMC is the "Erase Group"; Erase group is measured
> in write blocks that are the basic writable units of the Device.
> ...
> When the Erase is executed it will apply to all write blocks within an erase
> group.
>
> However, code logic in function EmmcEraseBlocks() does not check whether
> the blocks to be erased form complete erase groups. Missing such checks will
> lead to erasing extra data on the device.
>
> This commit will:
> a. If the device support the Trim command, use the Trim command to perform
> the erase operations for eMMC devices.
>
> According to the spec:
> Unlike the Erase command, the Trim function applies the erase operation to
> write blocks instead of erase groups.
>
> b. If the device does not support the Trim command, use the Erase command to
> erase the data in the erase groups. And write zeros to those blocks that cannot
> form a complete erase group.
>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
> MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c | 127
> +++++++++++++++++++++++++++++-
> 1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> index c432d26801..916f15d429 100644
> --- a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> +++ b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c
> @@ -1851,6 +1851,14 @@ EmmcEraseBlock (
> EraseBlock->SdMmcCmdBlk.CommandIndex = EMMC_ERASE;
> EraseBlock->SdMmcCmdBlk.CommandType = SdMmcCommandTypeAc;
> EraseBlock->SdMmcCmdBlk.ResponseType = SdMmcResponseTypeR1b;
> + if ((Device->ExtCsd.SecFeatureSupport & BIT4) != 0) {
> + //
> + // Perform a Trim operation which applies the erase operation to write
> blocks
> + // instead of erase groups. (Spec JESD84-B51, eMMC Electrical Standard 5.1,
> + // Section 6.6.10 and 6.10.4)
> + //
> + EraseBlock->SdMmcCmdBlk.CommandArgument = 1; }
>
> EraseBlock->IsEnd = IsEnd;
> EraseBlock->Token = Token;
> @@ -1903,6 +1911,43 @@ Error:
> }
>
> /**
> + Write zeros to specified blocks.
> +
> + @param[in] Partition A pointer to the EMMC_PARTITION instance.
> + @param[in] StartLba The starting logical block address to write zeros.
> + @param[in] Size The size in bytes to fill with zeros. This must be a
> multiple of
> + the physical block size of the device.
> +
> + @retval EFI_SUCCESS The request is executed successfully.
> + @retval EFI_OUT_OF_RESOURCES The request could not be executed due to
> a lack of resources.
> + @retval Others The request could not be executed successfully.
> +
> +**/
> +EFI_STATUS
> +EmmcWriteZeros (
> + IN EMMC_PARTITION *Partition,
> + IN EFI_LBA StartLba,
> + IN UINTN Size
> + )
> +{
> + EFI_STATUS Status;
> + UINT8 *Buffer;
> + UINT32 MediaId;
> +
> + Buffer = AllocateZeroPool (Size);
> + if (Buffer == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + MediaId = Partition->BlockMedia.MediaId;
> +
> + Status = EmmcReadWrite (Partition, MediaId, StartLba, Buffer, Size,
> + FALSE, NULL); FreePool (Buffer);
> +
> + return Status;
> +}
> +
> +/**
> Erase a specified number of device blocks.
>
> @param[in] This Indicates a pointer to the calling context.
> @@ -1943,7 +1988,13 @@ EmmcEraseBlocks (
> EFI_BLOCK_IO_MEDIA *Media;
> UINTN BlockSize;
> UINTN BlockNum;
> + EFI_LBA FirstLba;
> EFI_LBA LastLba;
> + EFI_LBA StartGroupLba;
> + EFI_LBA EndGroupLba;
> + UINT32 EraseGroupSize;
> + UINT32 Remainder;
> + UINTN WriteZeroSize;
> UINT8 PartitionConfig;
> EMMC_PARTITION *Partition;
> EMMC_DEVICE *Device;
> @@ -1978,7 +2029,8 @@ EmmcEraseBlocks (
> Token->TransactionStatus = EFI_SUCCESS;
> }
>
> - LastLba = Lba + BlockNum - 1;
> + FirstLba = Lba;
> + LastLba = Lba + BlockNum - 1;
>
> //
> // Check if needs to switch partition access.
> @@ -1994,7 +2046,78 @@ EmmcEraseBlocks (
> Device->ExtCsd.PartitionConfig = PartitionConfig;
> }
>
> - Status = EmmcEraseBlockStart (Partition, Lba,
> (EFI_BLOCK_IO2_TOKEN*)Token, FALSE);
> + if ((Device->ExtCsd.SecFeatureSupport & BIT4) == 0) {
> + //
> + // If the Trim operation is not supported by the device, handle the erase
> + // of blocks that do not form a complete erase group separately.
> + //
> + EraseGroupSize = This->EraseLengthGranularity;
> +
> + DivU64x32Remainder (FirstLba, EraseGroupSize, &Remainder);
> + StartGroupLba = (Remainder == 0) ? FirstLba : (FirstLba +
> + EraseGroupSize - Remainder);
> +
> + DivU64x32Remainder (LastLba + 1, EraseGroupSize, &Remainder);
> + EndGroupLba = LastLba + 1 - Remainder;
> +
> + //
> + // If the size to erase is smaller than the erase group size, the whole
> + // erase operation is performed by writting zeros.
> + //
> + if (BlockNum < EraseGroupSize) {
> + Status = EmmcWriteZeros (Partition, FirstLba, Size);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if ((Token != NULL) && (Token->Event != NULL)) {
> + Token->TransactionStatus = EFI_SUCCESS;
> + gBS->SignalEvent (Token->Event);
> + }
> + return EFI_SUCCESS;
> + }
> +
> + //
> + // If the starting LBA to erase is not aligned with the start of an erase
> + // group, write zeros to erase the data from starting LBA to the end of the
> + // current erase group.
> + //
> + if (StartGroupLba > FirstLba) {
> + WriteZeroSize = (UINTN)(StartGroupLba - FirstLba) * BlockSize;
> + Status = EmmcWriteZeros (Partition, FirstLba, WriteZeroSize);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + }
> +
> + //
> + // If the ending LBA to erase is not aligned with the end of an erase
> + // group, write zeros to erase the data from the start of the erase group
> + // to the ending LBA.
> + //
> + if (EndGroupLba <= LastLba) {
> + WriteZeroSize = (UINTN)(LastLba - EndGroupLba + 1) * BlockSize;
> + Status = EmmcWriteZeros (Partition, EndGroupLba, WriteZeroSize);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + }
> +
> + //
> + // Check whether there is erase group to erase.
> + //
> + if (EndGroupLba <= StartGroupLba) {
> + if ((Token != NULL) && (Token->Event != NULL)) {
> + Token->TransactionStatus = EFI_SUCCESS;
> + gBS->SignalEvent (Token->Event);
> + }
> + return EFI_SUCCESS;
> + }
> +
> + FirstLba = StartGroupLba;
> + LastLba = EndGroupLba - 1;
> + }
> +
> + Status = EmmcEraseBlockStart (Partition, FirstLba,
> + (EFI_BLOCK_IO2_TOKEN*)Token, FALSE);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> --
> 2.12.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
prev parent reply other threads:[~2017-08-11 4:18 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-10 1:21 [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra data is erased by EraseBlocks Hao Wu
2017-08-11 2:36 ` Zeng, Star
2017-08-11 4:20 ` Wu, Hao A [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A0931CDA83C@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox