Re: [RFC v1 1/1] MdeModulePkg/Usb: Use Pcd for UsbBootIoBlocks

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

Mike,

Evaluating block size is a good idea!
The block size of the target USB device is 2048 in my platform.
I modify the UsbMassBoot.c with evaluating block size and it works.
I will send the patch later.

Thanks,
Ming

On 2018/3/13 9:38, Kinney, Michael D wrote:

> Star,
>
> Maybe we should evaluate block size of the target USB
> device and limit the total transfer size to 64KB.
>
> This would continue to use 128 for 512B blocks and 
> 32 for 2K blocks.
>
> Mike
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Monday, March 12, 2018 5:44 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; 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;
>> graeme.gregory@linaro.org; Dong, Eric
>> <eric.dong@intel.com>
>> Cc: huangming23@huawei.com; wanghuiqiang@huawei.com;
>> ard.biesheuvel@linaro.org; zhangjinsong2@huawei.com;
>> Gao, Liming <liming.gao@intel.com>; guoheyi@huawei.com;
>> waip23@126.com; mengfanrong@huawei.com;
>> huangdaode@hisilicon.com; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: RE: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use
>> Pcd for UsbBootIoBlocks
>>
>> Mike,
>>
>> There was some discussion at
>> https://lists.01.org/pipermail/edk2-devel/2017-
>> January/006707.html.
>> Seeming, there is no detailed information about why it
>> was changed to 128 according to the commit log.
>>
>>
>> Thanks,
>> Star
>>
>> -----Original Message-----
>> From: Kinney, Michael D
>> Sent: Monday, March 12, 2018 11:47 PM
>> 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;
>> graeme.gregory@linaro.org; Zeng, Star
>> <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: huangming23@huawei.com; wanghuiqiang@huawei.com;
>> ard.biesheuvel@linaro.org; zhangjinsong2@huawei.com;
>> Gao, Liming <liming.gao@intel.com>; guoheyi@huawei.com;
>> waip23@126.com; mengfanrong@huawei.com;
>> huangdaode@hisilicon.com
>> Subject: RE: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use
>> Pcd for UsbBootIoBlocks
>>
>> Does anyone know wow was the original #define of 128
>> selected?
>>
>> If we are seeing compatibility issues with 128 and we
>> have greater compatibility with 64, what would be the
>> impact of just changing the #define to 64?
>>
>> Thanks,
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Ming Huang [mailto:ming.huang@linaro.org]
>>> Sent: Sunday, March 11, 2018 11:01 PM
>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>;
>>> leif.lindholm@linaro.org; linaro-
>> uefi@lists.linaro.org;
>>> edk2-devel@lists.01.org; graeme.gregory@linaro.org;
>> Zeng, Star
>>> <star.zeng@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>>> Cc: huangming23@huawei.com; wanghuiqiang@huawei.com;
>>> ard.biesheuvel@linaro.org; zhangjinsong2@huawei.com;
>> Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>;
>>> guoheyi@huawei.com; waip23@126.com;
>> mengfanrong@huawei.com;
>>> huangdaode@hisilicon.com
>>> Subject: Re: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use
>> Pcd for
>>> UsbBootIoBlocks
>>>
>>> Any comments about this patch?
>>>
>>>
>>> On 2018/3/2 14:36, Ming Huang wrote:
>>>> On 2018/2/27 18:01, Ni, Ruiyu wrote:
>>>>> On 2/27/2018 5:25 PM, Ming Huang wrote:
>>>>>> On 2018/2/27 13:25, Ni, Ruiyu wrote:
>>>>>>> I don't prefer to add PCD, unless we cannot find:
>>>>>>> 1. spec content  to describe the max/min blocks
>>>>>> There is no spec about the max/min blocks in my
>>> mind. I had checked this in
>>>>>> several pdf document like
>>>>>> Universal Serial Bus Mass Storage Class
>>> Specification Overview,
>>>>>> Universal Serial Bus Mass Storage Specification
>> For
>>> Bootability,
>>>>>> Universal Serial Bus Mass Storage Class Bulk-Only
>>> Transport.
>>>>>>> 2. error handling when the blocks number is
>> bigger
>>> than HW expects.
>>>>>> Where should error handling add to?  Error handing
>>> can't add to HW (end-point device),
>>>>>> because HW is not in our control scope.
>>>>> I mean maybe spec describes an error status could
>> be
>>> returned from HW when using 128. So that we can use
>> 64, 32, and
>>> smaller value until HW is happy.
>>>>> I am curious how the other USB storage drivers
>> handle
>>> this.
>>>>> PCD is a static way. Dynamic way is more preferred.
>>>>>
>>>> When using 128,  after waiting
>>> 5x5(USB_BOOT_COMMAND_RETRY=5,
>>> USB_BOOT_GENERAL_CMD_TIMEOUT=5) seconds,
>>>> the UsbBootReadBlocks ->UsbBootExecCmdWithRetry
>> retrun
>>> TimeOut. I don't know why HW return Timeout.
>>>> Booting time is to long if using Dynamic way to fix
>>> the issue.
>>>> When using 64, It works and booting from HW succeed.
>>>> May be using PCD is a simple and effective mean.
>>>>
>>>> Thanks
>>>> Ming
>>>>>>> Thanks/Ray
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: edk2-devel [mailto:edk2-devel-
>>> bounces@lists.01.org] On Behalf Of
>>>>>>>> Ming Huang
>>>>>>>> Sent: Saturday, February 24, 2018 5:30 PM
>>>>>>>> To: leif.lindholm@linaro.org; linaro-
>>> uefi@lists.linaro.org; edk2-
>>>>>>>> devel@lists.01.org; graeme.gregory@linaro.org;
>>> Zeng, Star
>>>>>>>> <star.zeng@intel.com>; Dong, Eric
>>> <eric.dong@intel.com>
>>>>>>>> Cc: huangming23@huawei.com;
>>> ard.biesheuvel@linaro.org; Ming Huang
>>>>>>>> <ming.huang@linaro.org>; Gao, Liming
>>> <liming.gao@intel.com>;
>>>>>>>> mengfanrong@huawei.com; guoheyi@huawei.com;
>>>>>>>> zhangjinsong2@huawei.com; Kinney, Michael D
>>>>>>>> <michael.d.kinney@intel.com>; waip23@126.com;
>>>>>>>> wanghuiqiang@huawei.com;
>> huangdaode@hisilicon.com
>>>>>>>> Subject: [edk2] [RFC v1 1/1] MdeModulePkg/Usb:
>> Use
>>> Pcd for
>>>>>>>> UsbBootIoBlocks
>>>>>>>>
>>>>>>>> Booting from USB may fail while the macro
>>> USB_BOOT_IO_BLOCKS set to 128
>>>>>>>> because 128 blocks is exceeded the maximun
>> blocks
>>> of some USB
>>>>>>>> devices,like some virtual CD-ROM from BMC. So,
>>> give a chance to set the
>>>>>>>> value of USB_BOOT_IO_BLOCKS by adding a Pcd.
>>>>>>>>
>>>>>>>> Contributed-under: TianoCore Contribution
>>> Agreement 1.1
>>>>>>>> Signed-off-by: Ming Huang
>> <ming.huang@linaro.org>
>>>>>>>> ---
>>>>>>>   MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBo
>>> ot.h         | 7
>>>>>>>> +++++--
>>>>>>>   MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassSt
>>> orageDxe.inf | 4
>>>>>>>> ++++
>>>>>>>>   MdeModulePkg/MdeModulePkg.dec
>>>              | 4 ++++
>>>>>>>>   MdeModulePkg/MdeModulePkg.uni
>>>              | 4 ++++
>>>>>>>>   4 files changed, 17 insertions(+), 2
>> deletions(-
>>> )
>>>>>>>> diff --git
>>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>>>>>>>> index 5ee50ac52a21..ca9240adbd5f 100644
>>>>>>>> ---
>>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>>>>>>>> +++
>>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h
>>>>>>>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR
>>> REPRESENTATIONS OF ANY
>>>>>>>> KIND, EITHER EXPRESS OR IMPLIED.
>>>>>>>>   #ifndef _EFI_USB_MASS_BOOT_H_
>>>>>>>>   #define _EFI_USB_MASS_BOOT_H_
>>>>>>>>
>>>>>>>> +#include <Library/PcdLib.h>
>>>>>>>> +
>>>>>>>>   //
>>>>>>>>   // The opcodes of various USB boot commands:
>>>>>>>>   // INQUIRY/REQUEST_SENSE are "No Timeout
>>> Commands" as specified @@
>>>>>>>> -66,9 +68,10 @@ 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
>> depanded
>>> on Pcd.
>>>>>>>> +// The default of PcdUsbBootIoBlocks is 128.
>> 512B
>>> * 128 = 64KB
>>>>>>>>   //
>>>>>>>> -#define USB_BOOT_IO_BLOCKS              128
>>>>>>>> +#define
>>> USB_BOOT_IO_BLOCKS              (FixedPcdGet32
>>>>>>>> (PcdUsbBootIoBlocks))
>>>>>>>>
>>>>>>>>   //
>>>>>>>>   // Retry mass command times, set by experience
>>> diff --git
>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageD
>>> xe.inf
>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageD
>>> xe.inf
>>>>>>>> index 26d15c7679bf..40426512f884 100644
>>>>>>>> ---
>>>>>>>>
>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageD
>>> xe.inf
>>>>>>>> +++
>>>>>>>>
>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageD
>>> xe.inf
>>>>>>>> @@ -60,6 +60,7 @@ [Sources]
>>>>>>>>     UsbMassDiskInfo.c
>>>>>>>>
>>>>>>>>   [Packages]
>>>>>>>> +  MdeModulePkg/MdeModulePkg.dec
>>>>>>>>     MdePkg/MdePkg.dec
>>>>>>>>
>>>>>>>>   [LibraryClasses]
>>>>>>>> @@ -83,5 +84,8 @@ [Protocols]
>>>>>>>>   # EVENT_TYPE_RELATIVE_TIMER        ## CONSUMES
>>>>>>>>   #
>>>>>>>>
>>>>>>>> +[FixedPcd]
>>>>>>>>
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUsbBootIoBlocks
>>>>>>>> +
>>>>>>>>   [UserExtensions.TianoCore."ExtraFiles"]
>>>>>>>>     UsbMassStorageDxeExtra.uni
>>>>>>>> diff --git a/MdeModulePkg/MdeModulePkg.dec
>>>>>>>> b/MdeModulePkg/MdeModulePkg.dec index
>>> 455979386e3f..fc40745315a0
>>>>>>>> 100644
>>>>>>>> --- a/MdeModulePkg/MdeModulePkg.dec
>>>>>>>> +++ b/MdeModulePkg/MdeModulePkg.dec
>>>>>>>> @@ -999,6 +999,10 @@ [PcdsFixedAtBuild]
>>>>>>>>     # @Prompt Enable UEFI Stack Guard.
>>>>>>>>
>>>>>>>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BO
>>> OLEAN|0
>>>>>>>> x30001055
>>>>>>>>
>>>>>>>> +## The Max blocks of usb transfer. The default
>> is
>>> 128.
>>>>>>>> +# @Prompt The Max blocks of usb transfer
>>>>>>>>
>> +gEfiMdeModulePkgTokenSpaceGuid.PcdUsbBootIoBlocks|128|U
>>> INT32|0x0
>>>>>>>> 000010B
>>>>>>>> +
>>>>>>>>   [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>>>>>>     ## Dynamic type PCD can be registered
>> callback
>>> function for Pcd setting
>>>>>>>> action.
>>>>>>>>     #  PcdMaxPeiPcdCallBackNumberPerPcdEntry
>>> indicates the maximum
>>>>>>>> number of callback function diff --git
>>> a/MdeModulePkg/MdeModulePkg.uni
>>>>>>>> b/MdeModulePkg/MdeModulePkg.uni index
>>> f3fa616438b0..c996d6b4ebe0
>>>>>>>> 100644
>>>>>>>> --- a/MdeModulePkg/MdeModulePkg.uni
>>>>>>>> +++ b/MdeModulePkg/MdeModulePkg.uni
>>>>>>>> @@ -1243,3 +1243,7 @@
>>>>>>>>   #string
>>>>>>>>
>> STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEdkiiFpdtStringRec
>>> ordEnableO
>>>>>>>> nly_HELP    #language en-US "Control which FPDT
>>> record format will be used
>>>>>>>> to store the performance entry.\n"
>>>>>>>>
>> "O n TRUE, the
>>> string FPDT
>>>>>>>> record will be used to store every performance
>>> entry.\n"
>> "O n FALSE, the
>>> different
>>>>>>>> FPDT record will be used to store the different
>>> performance entries."
>>>>>>>> +
>>>>>>>> +#string
>>>>>>>>
>> STR_gEfiMdeModulePkgTokenSpaceGuid_PcdUsbBootIoBlocks_PR
>>> OMPT
>>>>>>>> #language en-US "The Max blocks of usb
>> transfer."
>>>>>>>> +
>>>>>>>> +#string
>>>>>>>>
>> STR_gEfiMdeModulePkgTokenSpaceGuid_PcdUsbBootIoBlocks_HE
>>> LP
>>>>>>>> #language en-US "The Max blocks of usb transfer.
>>> The default is 128."
>>>>>>>> --
>>>>>>>> 1.9.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
>>>>>>