[U-Boot] [PATCH V2] [RFC] usb: storage: Limit transfer size to 120 kiB
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@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@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)) blk = size / 512; - } #endif
us->max_xfer_blk = blk;
On Tue, Sep 17, 2019 at 10:43 PM Marek Vasut marek.vasut@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@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@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@gmail.com
On 9/18/19 4:13 AM, Bin Meng wrote:
On Tue, Sep 17, 2019 at 10:43 PM Marek Vasut marek.vasut@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@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@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@gmail.com
Fixed and added to next . This really needs a LOT of testing. I am worried about performance here.
On Wed, Sep 18, 2019 at 6:07 PM Marek Vasut marek.vasut@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@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@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@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@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/85a0cb96dabf7572d...
Regards, Bin
On 9/18/19 12:27 PM, Bin Meng wrote:
On Wed, Sep 18, 2019 at 6:07 PM Marek Vasut marek.vasut@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@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@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@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@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/85a0cb96dabf7572d...
Should be fixed now.
participants (2)
-
Bin Meng -
Marek Vasut