> -----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-boun...@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...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Zeng, > Star <star.z...@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.z...@intel.com> > Cc: Ruiyu Ni <ruiyu...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Hao Wu <hao.a...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
