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

[Marek Vasut]

On 9/16/19 10:15 AM, Bin Meng wrote:
> On Mon, Sep 16, 2019 at 4:10 PM Marek Vasut wrote:
>>
>> On 9/16/19 8:53 AM, Bin Meng wrote:
>>> On Mon, Sep 16, 2019 at 6:24 AM Marek Vasut 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.
>>>>
>>>> ---
>>>>  common/usb_storage.c | 27 +++++++++++++++++----------
>>>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>>>> index 8c889bb1a6..130eab7832 100644
>>>> --- a/common/usb_storage.c
>>>> +++ b/common/usb_storage.c
>>>> @@ -943,21 +943,28 @@ static void usb_stor_set_max_xfer_blk(struct usb_device *udev,
>>>>         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
>>>> +       blk = 240;
>>>>  #else
>>>>         ret = usb_get_max_xfer_size(udev, (size_t *)&size);
>>>
>>> If we blindly set blk to 240 for both non-dm usb and dm usb here,
>>
>> Blindly ?
> 
> So based on the comments you added (also in kernel commit
> 779b457f66e1), it looks to me we have to set 240 for whatever usb
> storage devices. That suggests if usb_get_max_xfer_size() is
> implemented by DM USB drivers (so far only EHCI and xHCI implemented
> it) and return a value larger than 120KiB, some usb storage devices is
> subject to break.

I think we need to clamp the result of that to 240 blocks too.

>>> which makes usb_get_max_xfer_size() useless. Should we completely drop
>>> the usb_get_max_xfer_size() call?
>>
>> No, I think we should keep it if we ever decided to start implementing
>> quirks like US_FL_MAX_SECTORS_64 (cfr. Linux scsiglue.c).
>>
>>>>         if (ret < 0) {
>>>> -               /* unimplemented, let's use default 20 */
>>>> -               blk = 20;
>>>> +               blk = 240;
>>>>         } else {
>>>>                 if (size > USHRT_MAX * 512)
>>>>                         size = USHRT_MAX * 512;
>>>> --
> 
> Regards,
> Bin
> 


-- 
Best regards,
Marek Vasut