* [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB
@ 2019-09-15 22:24 Marek Vasut
2019-09-16 6:53 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Marek Vasut @ 2019-09-15 22:24 UTC (permalink / raw)
To: u-boot
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>
---
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 (ret < 0) {
- /* unimplemented, let's use default 20 */
- blk = 20;
+ blk = 240;
} else {
if (size > USHRT_MAX * 512)
size = USHRT_MAX * 512;
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB
2019-09-15 22:24 [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB Marek Vasut
@ 2019-09-16 6:53 ` Bin Meng
2019-09-16 8:10 ` Marek Vasut
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2019-09-16 6:53 UTC (permalink / raw)
To: u-boot
On Mon, Sep 16, 2019 at 6:24 AM 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>
> ---
> 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,
which makes usb_get_max_xfer_size() useless. Should we completely drop
the usb_get_max_xfer_size() call?
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB
2019-09-16 6:53 ` Bin Meng
@ 2019-09-16 8:10 ` Marek Vasut
2019-09-16 8:15 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Marek Vasut @ 2019-09-16 8:10 UTC (permalink / raw)
To: u-boot
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 ?
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB
2019-09-16 8:10 ` Marek Vasut
@ 2019-09-16 8:15 ` Bin Meng
2019-09-16 8:34 ` Marek Vasut
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2019-09-16 8:15 UTC (permalink / raw)
To: u-boot
On Mon, Sep 16, 2019 at 4:10 PM Marek Vasut <marek.vasut@gmail.com> 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.
>
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB
2019-09-16 8:15 ` Bin Meng
@ 2019-09-16 8:34 ` Marek Vasut
0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2019-09-16 8:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-16 8:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-15 22:24 [U-Boot] [PATCH] [RFC] usb: storage: Limit transfer size to 120 kiB Marek Vasut
2019-09-16 6:53 ` Bin Meng
2019-09-16 8:10 ` Marek Vasut
2019-09-16 8:15 ` Bin Meng
2019-09-16 8:34 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.