[U-Boot] [PATCH V2] [RFC] usb: storage: Limit transfer size to 120 kiB

[Marek Vasut]

On 9/18/19 4:13 AM, Bin Meng wrote:
> On Tue, Sep 17, 2019 at 10:43 PM Marek Vasut <marek.vasut at 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+renesas at gmail.com>
>> Cc: Bin Meng <bmeng.cn at gmail.com>
>> Cc: Simon Glass <sjg at 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.cn at gmail.com>

Fixed and added to next . This really needs a LOT of testing.
I am worried about performance here.

-- 
Best regards,
Marek Vasut