On 9/18/19 12:27 PM, Bin Meng wrote:
> On Wed, Sep 18, 2019 at 6:07 PM Marek Vasut <marek.va...@gmail.com> wrote:
>>
>> On 9/18/19 4:13 AM, Bin Meng wrote:
>>> On Tue, Sep 17, 2019 at 10:43 PM Marek Vasut <marek.va...@gmail.com> wrote:
>>>>
>>>> Due to constant influx of more and more weird and broken USB sticks,
>>>> do as Linux does in commit 779b457f66e10de3471479373463b27fd308dc85
>>>>
>>>> usb: storage: scsiglue: further describe our 240 sector limit
>>>>
>>>> Just so we have some sort of documentation as to why
>>>> we limit our Mass Storage transfers to 240 sectors,
>>>> let's update the comment to make clearer that
>>>> devices were found that would choke with larger
>>>> transfers.
>>>>
>>>> While at that, also make sure to clarify that other
>>>> operating systems have similar, albeit different,
>>>> limits on mass storage transfers.
>>>>
>>>> And reduce the maximum transfer length of USB storage to 120 kiB.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com>
>>>> Cc: Bin Meng <bmeng...@gmail.com>
>>>> Cc: Simon Glass <s...@chromium.org>
>>>> ---
>>>> V2: Reshuffle the code a bit, always clamp the transfer size to 240 blocks
>>>> ---
>>>> common/usb_storage.c | 43 ++++++++++++++++++++++---------------------
>>>> 1 file changed, 22 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>>>> index 8c889bb1a6..e1b539a082 100644
>>>> --- a/common/usb_storage.c
>>>> +++ b/common/usb_storage.c
>>>> @@ -938,31 +938,32 @@ do_retry:
>>>> static void usb_stor_set_max_xfer_blk(struct usb_device *udev,
>>>> struct us_data *us)
>>>> {
>>>> - unsigned short blk;
>>>> - size_t __maybe_unused size;
>>>> - int __maybe_unused ret;
>>>> -
>>>> -#if !CONFIG_IS_ENABLED(DM_USB)
>>>> -#ifdef CONFIG_USB_EHCI_HCD
>>>> /*
>>>> - * The U-Boot EHCI driver can handle any transfer length as long as
>>>> - * there is enough free heap space left, but the SCSI READ(10) and
>>>> - * WRITE(10) commands are limited to 65535 blocks.
>>>> + * Limit the total size of a transfer to 120 KB.
>>>> + *
>>>> + * Some devices are known to choke with anything larger. It seems
>>>> like
>>>> + * the problem stems from the fact that original IDE controllers
>>>> had
>>>> + * only an 8-bit register to hold the number of sectors in one
>>>> transfer
>>>> + * and even those couldn't handle a full 256 sectors.
>>>> + *
>>>> + * Because we want to make sure we interoperate with as many
>>>> devices as
>>>> + * possible, we will maintain a 240 sector transfer size limit for
>>>> USB
>>>> + * Mass Storage devices.
>>>> + *
>>>> + * Tests show that other operating have similar limits with
>>>> Microsoft
>>>> + * Windows 7 limiting transfers to 128 sectors for both USB2 and
>>>> USB3
>>>> + * and Apple Mac OS X 10.11 limiting transfers to 256 sectors for
>>>> USB2
>>>> + * and 2048 for USB3 devices.
>>>> */
>>>> - blk = USHRT_MAX;
>>>> -#else
>>>> - blk = 20;
>>>> -#endif
>>>> -#else
>>>> + unsigned short blk = 240;
>>>> +
>>>> +#if CONFIG_IS_ENABLED(DM_USB)
>>>> + size_t size;
>>>> + int ret;
>>>> +
>>>> ret = usb_get_max_xfer_size(udev, (size_t *)&size);
>>>> - if (ret < 0) {
>>>> - /* unimplemented, let's use default 20 */
>>>> - blk = 20;
>>>> - } else {
>>>> - if (size > USHRT_MAX * 512)
>>>> - size = USHRT_MAX * 512;
>>>> + if ((ret >= 0) && (size < 240 * 512))
>>>
>>> size < blk * 512
>>>
>>>> blk = size / 512;
>>>> - }
>>>> #endif
>>>>
>>>> us->max_xfer_blk = blk;
>>>> --
>>>
>>> Looks good otherwise
>>>
>>> Reviewed-by: Bin Meng <bmeng...@gmail.com>
>>
>> Fixed and added to next . This really needs a LOT of testing.
>> I am worried about performance here.
>
> Agree. I was wondering how Linux managed to set such limit for all usb
> storage devices and no performance degradation reported?
I suspect they create a new QH/qTD chain during the transfer and load it
into the controller right when the previous one finishes.
> BTW: it looks you missed adding my "Reviewed-by"
> https://gitlab.denx.de/u-boot/custodians/u-boot-usb/commit/85a0cb96dabf7572db3c6726a276a48c5c77a9da
Should be fixed now.
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
