Re: [edk2 UsbMassStorageDxe v1 1/1] MdeModulePkg/Usb: Replace macro USB_BOOT_IO_BLOCKS

[Ming Huang <ming.huang@linaro.org>]

Star & Mike,

I will fix the issues mentioned below and send v2 patch later.

Thanks,
Ming


On 2018/3/21 22:22, Kinney, Michael D wrote:
> One minor comment.
>
>   #define USB_BOOT_MAX_CARRY_SIZE         0x10000
>
> Should be
>
>   #define USB_BOOT_MAX_CARRY_SIZE         SIZE_64KB
>
> Mike
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Wednesday, March 21, 2018 5:41 AM
>> To: Ming Huang <ming.huang@linaro.org>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; leif.lindholm@linaro.org; linaro-
>> uefi@lists.linaro.org; edk2-devel@lists.01.org; Dong,
>> Eric <eric.dong@intel.com>
>> Cc: ard.biesheuvel@linaro.org; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; guoheyi@huawei.com;
>> wanghuiqiang@huawei.com; huangming23@huawei.com;
>> zhangjinsong2@huawei.com; mengfanrong@huawei.com;
>> huangdaode@hisilicon.com; waip23@126.com
>> Subject: RE: [edk2 UsbMassStorageDxe v1 1/1]
>> MdeModulePkg/Usb: Replace macro USB_BOOT_IO_BLOCKS
>>
>> Ming,
>>
>> Basically, I am ok with the change in this patch. There
>> are two comments.
>>
>> 1. Please add more background information about the
>> specific device in the commit log. For example, you
>> mentioned " like some virtual CD-ROM from BMC " in
>> previous patch.
>> 2. Please make sure building pass with the patch on
>> different tool chains. I tried to build with the patch
>> on VS2015, but met building failure like below.
>> warning C4244: '=': conversion from 'UINT32' to
>> 'UINT16', possible loss of data
>>
>>
>> Ray,
>>
>> Do you have other concern?
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Ming Huang [mailto:ming.huang@linaro.org]
>> Sent: Thursday, March 15, 2018 8:30 PM
>> To: leif.lindholm@linaro.org; linaro-
>> uefi@lists.linaro.org; edk2-devel@lists.01.org; Zeng,
>> Star <star.zeng@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>> Cc: ard.biesheuvel@linaro.org; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; guoheyi@huawei.com;
>> wanghuiqiang@huawei.com; huangming23@huawei.com;
>> zhangjinsong2@huawei.com; mengfanrong@huawei.com;
>> huangdaode@hisilicon.com; waip23@126.com; Ming Huang
>> <ming.huang@linaro.org>
>> Subject: [edk2 UsbMassStorageDxe v1 1/1]
>> MdeModulePkg/Usb: Replace macro USB_BOOT_IO_BLOCKS
>>
>> Booting from USB may fail while the macro
>> USB_BOOT_IO_BLOCKS set to 128 because the block size of
>> some USB devices are exceeded 512. So,the count blocks
>> to transfer should be calculated by block size of the
>> USB devices.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>> ---
>>  MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c |
>> 16 ++++++++++++----
>> MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h |
>> 4 ++--
>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git
>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
>> index b84bfd2d7290..b38cb6116bf4 100644
>> ---
>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
>> +++
>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c
>> @@ -814,11 +814,13 @@ UsbBootReadBlocks (
>>    USB_BOOT_READ10_CMD       ReadCmd;
>>    EFI_STATUS                Status;
>>    UINT16                    Count;
>> +  UINT16                    CountMax;
>>    UINT32                    BlockSize;
>>    UINT32                    ByteSize;
>>    UINT32                    Timeout;
>>
>>    BlockSize = UsbMass->BlockIoMedia.BlockSize;
>> +  CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
>>    Status    = EFI_SUCCESS;
>>
>>    while (TotalBlock > 0) {
>> @@ -827,7 +829,7 @@ UsbBootReadBlocks (
>>      // on the device. We must split the total block
>> because the READ10
>>      // command only has 16 bit transfer length (in the
>> unit of block).
>>      //
>> -    Count     = (UINT16)((TotalBlock <
>> USB_BOOT_IO_BLOCKS) ? TotalBlock : USB_BOOT_IO_BLOCKS);
>> +    Count     = (UINT16)((TotalBlock < CountMax) ?
>> TotalBlock : CountMax);
>>      ByteSize  = (UINT32)Count * BlockSize;
>>
>>      //
>> @@ -890,11 +892,13 @@ UsbBootWriteBlocks (
>>    USB_BOOT_WRITE10_CMD  WriteCmd;
>>    EFI_STATUS            Status;
>>    UINT16                Count;
>> +  UINT16                CountMax;
>>    UINT32                BlockSize;
>>    UINT32                ByteSize;
>>    UINT32                Timeout;
>>
>>    BlockSize = UsbMass->BlockIoMedia.BlockSize;
>> +  CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
>>    Status    = EFI_SUCCESS;
>>
>>    while (TotalBlock > 0) {
>> @@ -903,7 +907,7 @@ UsbBootWriteBlocks (
>>      // 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 <
>> USB_BOOT_IO_BLOCKS) ? TotalBlock : USB_BOOT_IO_BLOCKS);
>> +    Count     = (UINT16)((TotalBlock < CountMax) ?
>> TotalBlock : CountMax);
>>      ByteSize  = (UINT32)Count * BlockSize;
>>
>>      //
>> @@ -966,18 +970,20 @@ UsbBootReadBlocks16 (
>>    UINT8                     ReadCmd[16];
>>    EFI_STATUS                Status;
>>    UINT16                    Count;
>> +  UINT16                    CountMax;
>>    UINT32                    BlockSize;
>>    UINT32                    ByteSize;
>>    UINT32                    Timeout;
>>
>>    BlockSize = UsbMass->BlockIoMedia.BlockSize;
>> +  CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
>>    Status    = EFI_SUCCESS;
>>
>>    while (TotalBlock > 0) {
>>      //
>>      // Split the total blocks into smaller pieces.
>>      //
>> -    Count     = (UINT16)((TotalBlock <
>> USB_BOOT_IO_BLOCKS) ? TotalBlock : USB_BOOT_IO_BLOCKS);
>> +    Count     = (UINT16)((TotalBlock < CountMax) ?
>> TotalBlock : CountMax);
>>      ByteSize  = (UINT32)Count * BlockSize;
>>
>>      //
>> @@ -1040,18 +1046,20 @@ UsbBootWriteBlocks16 (
>>    UINT8                 WriteCmd[16];
>>    EFI_STATUS            Status;
>>    UINT16                Count;
>> +  UINT16                CountMax;
>>    UINT32                BlockSize;
>>    UINT32                ByteSize;
>>    UINT32                Timeout;
>>
>>    BlockSize = UsbMass->BlockIoMedia.BlockSize;
>> +  CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize;
>>    Status    = EFI_SUCCESS;
>>
>>    while (TotalBlock > 0) {
>>      //
>>      // Split the total blocks into smaller pieces.
>>      //
>> -    Count     = (UINT16)((TotalBlock <
>> USB_BOOT_IO_BLOCKS) ? TotalBlock : USB_BOOT_IO_BLOCKS);
>> +    Count     = (UINT16)((TotalBlock < CountMax) ?
>> TotalBlock : CountMax);
>>      ByteSize  = (UINT32)Count * BlockSize;
>>
>>      //
>> diff --git
>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>> index 13a926035ceb..fc5449bde21e 100644
>> ---
>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>> +++
>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>> @@ -65,9 +65,9 @@ WITHOUT WARRANTIES OR REPRESENTATIONS
>> OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  #define USB_PDT_SIMPLE_DIRECT           0x0E
>> ///< Simplified direct access device
>>
>>  //
>> -// Other parameters, Max carried size is 512B * 128 =
>> 64KB
>> +// Other parameters, Max carried size is 64KB.
>>  //
>> -#define USB_BOOT_IO_BLOCKS              128
>> +#define USB_BOOT_MAX_CARRY_SIZE         0x10000
>>
>>  //
>>  // Retry mass command times, set by experience
>> --
>> 1.9.1
>