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 <[email protected]>; Ming >> Huang <[email protected]>; Ni, Ruiyu >> <[email protected]>; [email protected]; linaro- >> [email protected]; [email protected]; >> [email protected]; Dong, Eric >> <[email protected]> >> Cc: [email protected]; [email protected]; >> [email protected]; [email protected]; >> Gao, Liming <[email protected]>; [email protected]; >> [email protected]; [email protected]; >> [email protected]; Zeng, Star >> <[email protected]> >> 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 <[email protected]>; Ni, Ruiyu >> <[email protected]>; [email protected]; linaro- >> [email protected]; [email protected]; >> [email protected]; Zeng, Star >> <[email protected]>; Dong, Eric <[email protected]>; >> Kinney, Michael D <[email protected]> >> Cc: [email protected]; [email protected]; >> [email protected]; [email protected]; >> Gao, Liming <[email protected]>; [email protected]; >> [email protected]; [email protected]; >> [email protected] >> 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:[email protected]] >>> Sent: Sunday, March 11, 2018 11:01 PM >>> To: Ni, Ruiyu <[email protected]>; >>> [email protected]; linaro- >> [email protected]; >>> [email protected]; [email protected]; >> Zeng, Star >>> <[email protected]>; Dong, Eric >> <[email protected]> >>> Cc: [email protected]; [email protected]; >>> [email protected]; [email protected]; >> Kinney, Michael D >>> <[email protected]>; Gao, Liming >> <[email protected]>; >>> [email protected]; [email protected]; >> [email protected]; >>> [email protected] >>> 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- >>> [email protected]] On Behalf Of >>>>>>>> Ming Huang >>>>>>>> Sent: Saturday, February 24, 2018 5:30 PM >>>>>>>> To: [email protected]; linaro- >>> [email protected]; edk2- >>>>>>>> [email protected]; [email protected]; >>> Zeng, Star >>>>>>>> <[email protected]>; Dong, Eric >>> <[email protected]> >>>>>>>> Cc: [email protected]; >>> [email protected]; Ming Huang >>>>>>>> <[email protected]>; Gao, Liming >>> <[email protected]>; >>>>>>>> [email protected]; [email protected]; >>>>>>>> [email protected]; Kinney, Michael D >>>>>>>> <[email protected]>; [email protected]; >>>>>>>> [email protected]; >> [email protected] >>>>>>>> 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 >> <[email protected]> >>>>>>>> --- >>>>>>> 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 >>>>>>>> [email protected] >>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>>> _______________________________________________ >>>>>> edk2-devel mailing list >>>>>> [email protected] >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>>> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
