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

[Bin Meng]

On Wed, Sep 18, 2019 at 6:07 PM Marek Vasut <marek.vasut at 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.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.

Agree. I was wondering how Linux managed to set such limit for all usb
storage devices and no performance degradation reported?

BTW: it looks you missed adding my "Reviewed-by"
https://gitlab.denx.de/u-boot/custodians/u-boot-usb/commit/85a0cb96dabf7572db3c6726a276a48c5c77a9da

Regards,
Bin