From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.20; helo=mga02.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4C25F2116325D for ; Mon, 15 Oct 2018 20:06:58 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Oct 2018 20:06:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,386,1534834800"; d="scan'208";a="100553683" Received: from shzintpr03.sh.intel.com (HELO [10.7.209.21]) ([10.239.4.100]) by orsmga002.jf.intel.com with ESMTP; 15 Oct 2018 20:06:56 -0700 To: Ruiyu Ni , edk2-devel@lists.01.org References: <20181015063833.61304-1-ruiyu.ni@intel.com> <20181015063833.61304-2-ruiyu.ni@intel.com> Cc: star.zeng@intel.com From: "Zeng, Star" Message-ID: <3bdcea71-d8d7-6acf-8aa9-7ad845f17cf9@intel.com> Date: Tue, 16 Oct 2018 11:06:26 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181015063833.61304-2-ruiyu.ni@intel.com> Subject: Re: [PATCH 01/11] MdeModulePkg/UsbMass: Merge UsbBoot(Read|Write)Blocks(16) X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Oct 2018 03:06:58 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2018/10/15 14:38, Ruiyu Ni wrote: > The change doesn't have functionality impact. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Star Zeng Ray, Thanks for the patch. I have a very very minor comment below. Reviewed-by: Star Zeng . > --- > .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 248 +++++---------------- > .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h | 76 ++----- > .../Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c | 8 +- > 3 files changed, 80 insertions(+), 252 deletions(-) > > diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c > index dd5950c54c..581571ab45 100644 > --- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c > +++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c > @@ -792,32 +792,34 @@ UsbBootDetectMedia ( > > > /** > - Read some blocks from the device. > + Read or write some blocks from the device. > > - @param UsbMass The USB mass storage device to read from > + @param UsbMass The USB mass storage device to access > + @param Write TRUE for write operation. > @param Lba The start block number > - @param TotalBlock Total block number to read > - @param Buffer The buffer to read to > + @param TotalBlock Total block number to read or write > + @param Buffer The buffer to read to or write from > > - @retval EFI_SUCCESS Data are read into the buffer > - @retval Others Failed to read all the data > + @retval EFI_SUCCESS Data are read into the buffer or writen into the device. > + @retval Others Failed to read or write all the data > > **/ > EFI_STATUS > -UsbBootReadBlocks ( > +UsbBootReadWriteBlocks ( > IN USB_MASS_DEVICE *UsbMass, > + IN BOOLEAN Write, > IN UINT32 Lba, > IN UINTN TotalBlock, > - OUT UINT8 *Buffer > + IN OUT UINT8 *Buffer > ) > { > - USB_BOOT_READ10_CMD ReadCmd; > - EFI_STATUS Status; > - UINT16 Count; > - UINT16 CountMax; > - UINT32 BlockSize; > - UINT32 ByteSize; > - UINT32 Timeout; > + USB_BOOT_READ_WRITE_10_CMD Cmd; > + EFI_STATUS Status; > + UINT16 Count; > + UINT16 CountMax; > + UINT32 BlockSize; > + UINT32 ByteSize; > + UINT32 Timeout; > > BlockSize = UsbMass->BlockIoMedia.BlockSize; > CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize); > @@ -840,17 +842,17 @@ UsbBootReadBlocks ( > // > // Fill in the command then execute > // > - ZeroMem (&ReadCmd, sizeof (USB_BOOT_READ10_CMD)); > + ZeroMem (&Cmd, sizeof (USB_BOOT_READ_WRITE_10_CMD)); > > - ReadCmd.OpCode = USB_BOOT_READ10_OPCODE; > - ReadCmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun)); > - WriteUnaligned32 ((UINT32 *) ReadCmd.Lba, SwapBytes32 (Lba)); > - WriteUnaligned16 ((UINT16 *) ReadCmd.TransferLen, SwapBytes16 (Count)); > + Cmd.OpCode = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE; > + Cmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun)); > + WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba)); > + WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count)); > > Status = UsbBootExecCmdWithRetry ( > UsbMass, > - &ReadCmd, > - (UINT8) sizeof (USB_BOOT_READ10_CMD), > + &Cmd, > + (UINT8) sizeof (USB_BOOT_READ_WRITE_10_CMD), > EfiUsbDataIn, > Buffer, > ByteSize, > @@ -859,86 +861,11 @@ UsbBootReadBlocks ( > if (EFI_ERROR (Status)) { > return Status; > } > - DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks: LBA (0x%x), Blk (0x%x)\n", Lba, Count)); > - Lba += Count; > - Buffer += Count * BlockSize; > - TotalBlock -= Count; > - } > - > - return Status; > -} > - > - > -/** > - Write some blocks to the device. > - > - @param UsbMass The USB mass storage device to write to > - @param Lba The start block number > - @param TotalBlock Total block number to write > - @param Buffer Pointer to the source buffer for the data. > - > - @retval EFI_SUCCESS Data are written into the buffer > - @retval Others Failed to write all the data > - > -**/ > -EFI_STATUS > -UsbBootWriteBlocks ( > - IN USB_MASS_DEVICE *UsbMass, > - IN UINT32 Lba, > - IN UINTN TotalBlock, > - IN UINT8 *Buffer > - ) > -{ > - USB_BOOT_WRITE10_CMD WriteCmd; > - EFI_STATUS Status; > - UINT16 Count; > - UINT16 CountMax; > - UINT32 BlockSize; > - UINT32 ByteSize; > - UINT32 Timeout; > - > - BlockSize = UsbMass->BlockIoMedia.BlockSize; > - CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize); > - Status = EFI_SUCCESS; > - > - while (TotalBlock > 0) { > - // > - // Split the total blocks into smaller pieces to ease the pressure > - // on the device. We must split the total block because the WRITE10 > - // command only has 16 bit transfer length (in the unit of block). > - // > - Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax); > - ByteSize = (UINT32)Count * BlockSize; > - > - // > - // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1] > - // > - Timeout = (UINT32) USB_BOOT_GENERAL_CMD_TIMEOUT; > - > - // > - // Fill in the write10 command block > - // > - ZeroMem (&WriteCmd, sizeof (USB_BOOT_WRITE10_CMD)); > - > - WriteCmd.OpCode = USB_BOOT_WRITE10_OPCODE; > - WriteCmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun)); > - WriteUnaligned32 ((UINT32 *) WriteCmd.Lba, SwapBytes32 (Lba)); > - WriteUnaligned16 ((UINT16 *) WriteCmd.TransferLen, SwapBytes16 (Count)); > - > - Status = UsbBootExecCmdWithRetry ( > - UsbMass, > - &WriteCmd, > - (UINT8) sizeof (USB_BOOT_WRITE10_CMD), > - EfiUsbDataOut, > - Buffer, > - ByteSize, > - Timeout > - ); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - DEBUG ((EFI_D_BLKIO, "UsbBootWriteBlocks: LBA (0x%x), Blk (0x%x)\n", Lba, Count)); > - > + DEBUG (( > + DEBUG_BLKIO, "UsbBoot%sBlocks: LBA (0x%lx), Blk (0x%x)\n", > + Write ? L"Write" : L"Read", > + Lba, Count > + )); > Lba += Count; > Buffer += Count * BlockSize; > TotalBlock -= Count; > @@ -948,26 +875,27 @@ UsbBootWriteBlocks ( > } > > /** > - Read some blocks from the device by SCSI 16 byte cmd. > + Read or write some blocks from the device by SCSI 16 byte cmd I saw the patch kept the ending '.' for UsbBootReadWriteBlocks, but removed the ending '.' for this function. Is it better to be consistent? :) Thanks, Star > > - @param UsbMass The USB mass storage device to read from > + @param UsbMass The USB mass storage device to access > + @param Write TRUE for write operation. > @param Lba The start block number > - @param TotalBlock Total block number to read > - @param Buffer The buffer to read to > - > - @retval EFI_SUCCESS Data are read into the buffer > - @retval Others Failed to read all the data > + @param TotalBlock Total block number to read or write > + @param Buffer The buffer to read to or write from > > + @retval EFI_SUCCESS Data are read into the buffer or writen into the device. > + @retval Others Failed to read or write all the data > **/ > EFI_STATUS > -UsbBootReadBlocks16 ( > +UsbBootReadWriteBlocks16 ( > IN USB_MASS_DEVICE *UsbMass, > + IN BOOLEAN Write, > IN UINT64 Lba, > IN UINTN TotalBlock, > - OUT UINT8 *Buffer > + IN OUT UINT8 *Buffer > ) > { > - UINT8 ReadCmd[16]; > + UINT8 Cmd[16]; > EFI_STATUS Status; > UINT16 Count; > UINT16 CountMax; > @@ -994,17 +922,17 @@ UsbBootReadBlocks16 ( > // > // Fill in the command then execute > // > - ZeroMem (ReadCmd, sizeof (ReadCmd)); > + ZeroMem (Cmd, sizeof (Cmd)); > > - ReadCmd[0] = EFI_SCSI_OP_READ16; > - ReadCmd[1] = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0)); > - WriteUnaligned64 ((UINT64 *) &ReadCmd[2], SwapBytes64 (Lba)); > - WriteUnaligned32 ((UINT32 *) &ReadCmd[10], SwapBytes32 (Count)); > + Cmd[0] = Write ? EFI_SCSI_OP_WRITE16 : EFI_SCSI_OP_READ16; > + Cmd[1] = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0)); > + WriteUnaligned64 ((UINT64 *) &Cmd[2], SwapBytes64 (Lba)); > + WriteUnaligned32 ((UINT32 *) &Cmd[10], SwapBytes32 (Count)); > > Status = UsbBootExecCmdWithRetry ( > UsbMass, > - ReadCmd, > - (UINT8) sizeof (ReadCmd), > + Cmd, > + (UINT8) sizeof (Cmd), > EfiUsbDataIn, > Buffer, > ByteSize, > @@ -1013,83 +941,11 @@ UsbBootReadBlocks16 ( > if (EFI_ERROR (Status)) { > return Status; > } > - DEBUG ((EFI_D_BLKIO, "UsbBootReadBlocks16: LBA (0x%lx), Blk (0x%x)\n", Lba, Count)); > - Lba += Count; > - Buffer += Count * BlockSize; > - TotalBlock -= Count; > - } > - > - return Status; > -} > - > - > -/** > - Write some blocks to the device by SCSI 16 byte cmd. > - > - @param UsbMass The USB mass storage device to write to > - @param Lba The start block number > - @param TotalBlock Total block number to write > - @param Buffer Pointer to the source buffer for the data. > - > - @retval EFI_SUCCESS Data are written into the buffer > - @retval Others Failed to write all the data > - > -**/ > -EFI_STATUS > -UsbBootWriteBlocks16 ( > - IN USB_MASS_DEVICE *UsbMass, > - IN UINT64 Lba, > - IN UINTN TotalBlock, > - IN UINT8 *Buffer > - ) > -{ > - UINT8 WriteCmd[16]; > - EFI_STATUS Status; > - UINT16 Count; > - UINT16 CountMax; > - UINT32 BlockSize; > - UINT32 ByteSize; > - UINT32 Timeout; > - > - BlockSize = UsbMass->BlockIoMedia.BlockSize; > - CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize); > - Status = EFI_SUCCESS; > - > - while (TotalBlock > 0) { > - // > - // Split the total blocks into smaller pieces. > - // > - Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax); > - ByteSize = (UINT32)Count * BlockSize; > - > - // > - // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1] > - // > - Timeout = (UINT32) USB_BOOT_GENERAL_CMD_TIMEOUT; > - > - // > - // Fill in the write16 command block > - // > - ZeroMem (WriteCmd, sizeof (WriteCmd)); > - > - WriteCmd[0] = EFI_SCSI_OP_WRITE16; > - WriteCmd[1] = (UINT8) ((USB_BOOT_LUN (UsbMass->Lun) & 0xE0)); > - WriteUnaligned64 ((UINT64 *) &WriteCmd[2], SwapBytes64 (Lba)); > - WriteUnaligned32 ((UINT32 *) &WriteCmd[10], SwapBytes32 (Count)); > - > - Status = UsbBootExecCmdWithRetry ( > - UsbMass, > - WriteCmd, > - (UINT8) sizeof (WriteCmd), > - EfiUsbDataOut, > - Buffer, > - ByteSize, > - Timeout > - ); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - DEBUG ((EFI_D_BLKIO, "UsbBootWriteBlocks: LBA (0x%lx), Blk (0x%x)\n", Lba, Count)); > + DEBUG (( > + DEBUG_BLKIO, "UsbBoot%sBlocks16: LBA (0x%lx), Blk (0x%x)\n", > + Write ? L"Write" : L"Read", > + Lba, Count > + )); > Lba += Count; > Buffer += Count * BlockSize; > TotalBlock -= Count; > diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h > index d4042e320d..3f8213b3d7 100644 > --- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h > +++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h > @@ -161,17 +161,7 @@ typedef struct { > UINT8 TransferLen[2]; ///< Transfer length > UINT8 Reserverd1; > UINT8 Pad[2]; > -} USB_BOOT_READ10_CMD; > - > -typedef struct { > - UINT8 OpCode; > - UINT8 Lun; > - UINT8 Lba[4]; > - UINT8 Reserved0; > - UINT8 TransferLen[2]; > - UINT8 Reserverd1; > - UINT8 Pad[2]; > -} USB_BOOT_WRITE10_CMD; > +} USB_BOOT_READ_WRITE_10_CMD; > > typedef struct { > UINT8 OpCode; > @@ -292,66 +282,48 @@ UsbBootReadBlocks ( > ); > > /** > - Write some blocks to the device. > + Read or write some blocks from the device. > > - @param UsbMass The USB mass storage device to write to > + @param UsbMass The USB mass storage device to access > + @param Write TRUE for write operation. > @param Lba The start block number > - @param TotalBlock Total block number to write > - @param Buffer Pointer to the source buffer for the data. > + @param TotalBlock Total block number to read or write > + @param Buffer The buffer to read to or write from > > - @retval EFI_SUCCESS Data are written into the buffer > - @retval Others Failed to write all the data > + @retval EFI_SUCCESS Data are read into the buffer or writen into the device. > + @retval Others Failed to read or write all the data > > **/ > EFI_STATUS > -UsbBootWriteBlocks ( > - IN USB_MASS_DEVICE *UsbMass, > - IN UINT32 Lba, > - IN UINTN TotalBlock, > - IN UINT8 *Buffer > +UsbBootReadWriteBlocks ( > + IN USB_MASS_DEVICE *UsbMass, > + IN BOOLEAN Write, > + IN UINT32 Lba, > + IN UINTN TotalBlock, > + IN OUT UINT8 *Buffer > ); > > /** > - Read some blocks from the device by SCSI 16 byte cmd. > + Read or write some blocks from the device by SCSI 16 byte cmd > > - @param UsbMass The USB mass storage device to read from > + @param UsbMass The USB mass storage device to access > + @param Write TRUE for write operation. > @param Lba The start block number > - @param TotalBlock Total block number to read > - @param Buffer The buffer to read to > - > - @retval EFI_SUCCESS Data are read into the buffer > - @retval Others Failed to read all the data > + @param TotalBlock Total block number to read or write > + @param Buffer The buffer to read to or write from > > + @retval EFI_SUCCESS Data are read into the buffer or writen into the device. > + @retval Others Failed to read or write all the data > **/ > EFI_STATUS > -UsbBootReadBlocks16 ( > +UsbBootReadWriteBlocks16 ( > IN USB_MASS_DEVICE *UsbMass, > + IN BOOLEAN Write, > IN UINT64 Lba, > IN UINTN TotalBlock, > - OUT UINT8 *Buffer > + IN OUT UINT8 *Buffer > ); > > -/** > - Write some blocks to the device by SCSI 16 byte cmd. > - > - @param UsbMass The USB mass storage device to write to > - @param Lba The start block number > - @param TotalBlock Total block number to write > - @param Buffer Pointer to the source buffer for the data. > - > - @retval EFI_SUCCESS Data are written into the buffer > - @retval Others Failed to write all the data > - > -**/ > -EFI_STATUS > -UsbBootWriteBlocks16 ( > - IN USB_MASS_DEVICE *UsbMass, > - IN UINT64 Lba, > - IN UINTN TotalBlock, > - IN UINT8 *Buffer > - ); > - > - > /** > Use the USB clear feature control transfer to clear the endpoint stall condition. > > diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c > index 62bf3c5883..9413b00680 100644 > --- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c > +++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c > @@ -172,9 +172,9 @@ UsbMassReadBlocks ( > } > > if (UsbMass->Cdb16Byte) { > - Status = UsbBootReadBlocks16 (UsbMass, Lba, TotalBlock, Buffer); > + Status = UsbBootReadWriteBlocks16 (UsbMass, FALSE, Lba, TotalBlock, Buffer); > } else { > - Status = UsbBootReadBlocks (UsbMass, (UINT32) Lba, TotalBlock, Buffer); > + Status = UsbBootReadWriteBlocks (UsbMass, FALSE, (UINT32) Lba, TotalBlock, Buffer); > } > > if (EFI_ERROR (Status)) { > @@ -292,9 +292,9 @@ UsbMassWriteBlocks ( > // and clear the status should the write succeed. > // > if (UsbMass->Cdb16Byte) { > - Status = UsbBootWriteBlocks16 (UsbMass, Lba, TotalBlock, Buffer); > + Status = UsbBootReadWriteBlocks16 (UsbMass, TRUE, Lba, TotalBlock, Buffer); > } else { > - Status = UsbBootWriteBlocks (UsbMass, (UINT32) Lba, TotalBlock, Buffer); > + Status = UsbBootReadWriteBlocks (UsbMass, TRUE, (UINT32) Lba, TotalBlock, Buffer); > } > > if (EFI_ERROR (Status)) { >