[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
usb_storage as it wrongly assumes that every transfer can use 4k
per qt_buffer. This is wrong if the start address of the data
is not 4k aligned and leads to 'EHCI timed out on TD' messages
because of 'out of buffer pointers' in ehci_td_buffer function.

Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
---
 common/usb_storage.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index faad237..612a553 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1416,10 +1416,10 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
 
 	/*
-	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a
-	 * transfer without running itself out of qt_buffers.
+	 * The U-Boot EHCI driver cannot handle more than 4096 * 4 bytes in a
+	 * unaligned transfer without running itself out of qt_buffers.
 	 */
-	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
+	ss->max_xfer_blk = (4096 * 4) / dev_desc->blksz;
 
 	init_part(dev_desc);
 
-- 
1.7.9.5

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> usb_storage as it wrongly assumes that every transfer can use 4k
> per qt_buffer. This is wrong if the start address of the data
> is not 4k aligned and leads to 'EHCI timed out on TD' messages
> because of 'out of buffer pointers' in ehci_td_buffer function.

Stefan, thanks a lot for finding this! I'll look into it on 21st I hope.

> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> ---
>  common/usb_storage.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index faad237..612a553 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -1416,10 +1416,10 @@ int usb_stor_get_info(struct usb_device *dev,
> struct us_data *ss, USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
> 
>  	/*
> -	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a
> -	 * transfer without running itself out of qt_buffers.
> +	 * The U-Boot EHCI driver cannot handle more than 4096 * 4 bytes in a
> +	 * unaligned transfer without running itself out of qt_buffers.
>  	 */
> -	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
> +	ss->max_xfer_blk = (4096 * 4) / dev_desc->blksz;
> 
>  	init_part(dev_desc);

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> usb_storage as it wrongly assumes that every transfer can use 4k
> per qt_buffer. This is wrong if the start address of the data
> is not 4k aligned and leads to 'EHCI timed out on TD' messages
> because of 'out of buffer pointers' in ehci_td_buffer function.
> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>

Ok, first I have to admit I broke my promise to look into this ASAP, sorry about 
it :-(

Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 ?

> ---
>  common/usb_storage.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index faad237..612a553 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -1416,10 +1416,10 @@ int usb_stor_get_info(struct usb_device *dev,
> struct us_data *ss, USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
> 
>  	/*
> -	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a
> -	 * transfer without running itself out of qt_buffers.
> +	 * The U-Boot EHCI driver cannot handle more than 4096 * 4 bytes in a
> +	 * unaligned transfer without running itself out of qt_buffers.
>  	 */
> -	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
> +	ss->max_xfer_blk = (4096 * 4) / dev_desc->blksz;
> 
>  	init_part(dev_desc);

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

Am 03.07.2012 20:10, schrieb Marek Vasut:
>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
>> usb_storage as it wrongly assumes that every transfer can use 4k
>> per qt_buffer. This is wrong if the start address of the data
>> is not 4k aligned and leads to 'EHCI timed out on TD' messages
>> because of 'out of buffer pointers' in ehci_td_buffer function.
>>
>> Cc: Marek Vasut <marex@denx.de>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> Ok, first I have to admit I broke my promise to look into this ASAP, sorry about
> it :-(
No problem, as long as we get it into the next release. ;-)
>
> Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 ?
No, because the first blk need to be aligned with the 4096. In worst 
case the blk is at the end of the 4096 range. If we assume that the blk 
is aligned to blk_sz we can change it to ((4096 * 4) / dev_desc->blk_sz) 
+ 1. I skip the last blk (+ 1) because with 4096 aligned first blk we 
unaligned the next transfer and add extra short packages to each ehci 
transfer.

If we want to maximise the usage we need to calculate the max_xfer_blk 
depending on the start address of the first blk.

>> ---
>>   common/usb_storage.c |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index faad237..612a553 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -1416,10 +1416,10 @@ int usb_stor_get_info(struct usb_device *dev,
>> struct us_data *ss, USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
>>
>>   	/*
>> -	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a
>> -	 * transfer without running itself out of qt_buffers.
>> +	 * The U-Boot EHCI driver cannot handle more than 4096 * 4 bytes in a
>> +	 * unaligned transfer without running itself out of qt_buffers.
>>   	 */
>> -	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
>> +	ss->max_xfer_blk = (4096 * 4) / dev_desc->blksz;
>>
>>   	init_part(dev_desc);
>

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Schneider, Kolja]

> Am 03.07.2012 20:10, schrieb Marek Vasut:
> >> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> >> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> >> usb_storage as it wrongly assumes that every transfer can use 4k
> >> per qt_buffer. This is wrong if the start address of the data
> >> is not 4k aligned and leads to 'EHCI timed out on TD' messages
> >> because of 'out of buffer pointers' in ehci_td_buffer function.
> >>
> >> Cc: Marek Vasut <marex@denx.de>
> >> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> > Ok, first I have to admit I broke my promise to look into this ASAP, sorry
> about
> > it :-(
> No problem, as long as we get it into the next release. ;-)
> >
> > Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 ?
> No, because the first blk need to be aligned with the 4096. In worst
> case the blk is at the end of the 4096 range. If we assume that the blk
> is aligned to blk_sz we can change it to ((4096 * 4) / dev_desc->blk_sz)
> + 1. I skip the last blk (+ 1) because with 4096 aligned first blk we
> unaligned the next transfer and add extra short packages to each ehci
> transfer.
> 
> If we want to maximise the usage we need to calculate the max_xfer_blk
> depending on the start address of the first blk.
> 

I admit to not totally getting it. However, there are two things that come to my mind:
 - 	Doesn't the EHCI Specification mention exactly five buffers that can/should/must
   	be used?
 - 	I think I once stumbled across some comment that said as much as the blocks
	always having to be aligned anyway?

:-) Kolja

> >> ---
> >>   common/usb_storage.c |    6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/common/usb_storage.c b/common/usb_storage.c
> >> index faad237..612a553 100644
> >> --- a/common/usb_storage.c
> >> +++ b/common/usb_storage.c
> >> @@ -1416,10 +1416,10 @@ int usb_stor_get_info(struct usb_device
> *dev,
> >> struct us_data *ss, USB_STOR_PRINTF("partype: %d\n", dev_desc-
> >part_type);
> >>
> >>   	/*
> >> -	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in
> a
> >> -	 * transfer without running itself out of qt_buffers.
> >> +	 * The U-Boot EHCI driver cannot handle more than 4096 * 4 bytes in
> a
> >> +	 * unaligned transfer without running itself out of qt_buffers.
> >>   	 */
> >> -	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
> >> +	ss->max_xfer_blk = (4096 * 4) / dev_desc->blksz;
> >>
> >>   	init_part(dev_desc);
> >
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

Am 04.07.2012 08:57, schrieb Schneider, Kolja:
>> Am 03.07.2012 20:10, schrieb Marek Vasut:
>>>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
>>>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
>>>> usb_storage as it wrongly assumes that every transfer can use 4k
>>>> per qt_buffer. This is wrong if the start address of the data
>>>> is not 4k aligned and leads to 'EHCI timed out on TD' messages
>>>> because of 'out of buffer pointers' in ehci_td_buffer function.
>>>>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>> Ok, first I have to admit I broke my promise to look into this ASAP, sorry
>> about
>>> it :-(
>> No problem, as long as we get it into the next release. ;-)
>>> Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 ?
>> No, because the first blk need to be aligned with the 4096. In worst
>> case the blk is at the end of the 4096 range. If we assume that the blk
>> is aligned to blk_sz we can change it to ((4096 * 4) / dev_desc->blk_sz)
>> + 1. I skip the last blk (+ 1) because with 4096 aligned first blk we
>> unaligned the next transfer and add extra short packages to each ehci
>> transfer.
>>
>> If we want to maximise the usage we need to calculate the max_xfer_blk
>> depending on the start address of the first blk.
>>
> I admit to not totally getting it. However, there are two things that come to my mind:
>   - 	Doesn't the EHCI Specification mention exactly five buffers that can/should/must
>     	be used?
Yes, you can use up to five 4096 byte buffers.
>   - 	I think I once stumbled across some comment that said as much as the blocks
> 	always having to be aligned anyway?
The buffers must be aligned to a 4096 byte page. This means that you 
have to use the first and last buffer to align your data to the next or 
previous 4096 byte page boundary.

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> Am 04.07.2012 08:57, schrieb Schneider, Kolja:
> >> Am 03.07.2012 20:10, schrieb Marek Vasut:
> >>>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> >>>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> >>>> usb_storage as it wrongly assumes that every transfer can use 4k
> >>>> per qt_buffer. This is wrong if the start address of the data
> >>>> is not 4k aligned and leads to 'EHCI timed out on TD' messages
> >>>> because of 'out of buffer pointers' in ehci_td_buffer function.
> >>>> 
> >>>> Cc: Marek Vasut <marex@denx.de>
> >>>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> >>> 
> >>> Ok, first I have to admit I broke my promise to look into this ASAP,
> >>> sorry
> >> 
> >> about
> >> 
> >>> it :-(
> >> 
> >> No problem, as long as we get it into the next release. ;-)
> >> 
> >>> Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 ?
> >> 
> >> No, because the first blk need to be aligned with the 4096. In worst
> >> case the blk is at the end of the 4096 range. If we assume that the blk
> >> is aligned to blk_sz we can change it to ((4096 * 4) / dev_desc->blk_sz)
> >> + 1. I skip the last blk (+ 1) because with 4096 aligned first blk we
> >> unaligned the next transfer and add extra short packages to each ehci
> >> transfer.
> >> 
> >> If we want to maximise the usage we need to calculate the max_xfer_blk
> >> depending on the start address of the first blk.
> > 
> > I admit to not totally getting it. However, there are two things that come 
to my mind:
> >   - 	Doesn't the EHCI Specification mention exactly five buffers that
> >   can/should/must
> >   
> >     	be used?
> 
> Yes, you can use up to five 4096 byte buffers.
> 
> >   - 	I think I once stumbled across some comment that said as much as 
the
> >   blocks
> > 	
> > 	always having to be aligned anyway?
> 
> The buffers must be aligned to a 4096 byte page. This means that you
> have to use the first and last buffer to align your data to the next or
> previous 4096 byte page boundary.

All right, I managed to replicate the issue. This (or similar) doesn't work for 
you, right:

usb read 0x42000004 0x0 0x400

The proper solution would be to introduce a bounce buffer for such unaligned 
transfers. A proper, generic bounce buffer that can be configured to bounce on 
specified boundaries and warns about performance penalties.

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

Am 07.07.2012 23:58, schrieb Marek Vasut:
>> Am 04.07.2012 08:57, schrieb Schneider, Kolja:
>>>> Am 03.07.2012 20:10, schrieb Marek Vasut:
>>>>>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
>>>>>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
>>>>>> usb_storage as it wrongly assumes that every transfer can use 4k
>>>>>> per qt_buffer. This is wrong if the start address of the data
>>>>>> is not 4k aligned and leads to 'EHCI timed out on TD' messages
>>>>>> because of 'out of buffer pointers' in ehci_td_buffer function.
>>>>>>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>> Ok, first I have to admit I broke my promise to look into this ASAP,
>>>>> sorry
>>>> about
>>>>
>>>>> it :-(
>>>> No problem, as long as we get it into the next release. ;-)
>>>>
>>>>> Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 ?
>>>> No, because the first blk need to be aligned with the 4096. In worst
>>>> case the blk is at the end of the 4096 range. If we assume that the blk
>>>> is aligned to blk_sz we can change it to ((4096 * 4) / dev_desc->blk_sz)
>>>> + 1. I skip the last blk (+ 1) because with 4096 aligned first blk we
>>>> unaligned the next transfer and add extra short packages to each ehci
>>>> transfer.
>>>>
>>>> If we want to maximise the usage we need to calculate the max_xfer_blk
>>>> depending on the start address of the first blk.
>>> I admit to not totally getting it. However, there are two things that come
> to my mind:
>>>    - 	Doesn't the EHCI Specification mention exactly five buffers that
>>>    can/should/must
>>>    
>>>      	be used?
>> Yes, you can use up to five 4096 byte buffers.
>>
>>>    - 	I think I once stumbled across some comment that said as much as
> the
>>>    blocks
>>> 	
>>> 	always having to be aligned anyway?
>> The buffers must be aligned to a 4096 byte page. This means that you
>> have to use the first and last buffer to align your data to the next or
>> previous 4096 byte page boundary.
> All right, I managed to replicate the issue. This (or similar) doesn't work for
> you, right:
>
> usb read 0x42000004 0x0 0x400
I observe the bug during
fatload usb 0 0x800000 uImage

...
dev=0ffb0440, pipe=c8008283, buffer=00a90000, length=20480, req=(null)
...
dev=0ffb0440, pipe=c8008283, buffer=00a95000, length=20480, req=(null)
...
dev=0ffb0440, pipe=c8008283, buffer=00a9a000, length=18432, req=(null)
...
dev=0ffb0440, pipe=c8008283, buffer=00a9e800, length=20480, req=(null)
out of buffer pointers (2048 bytes left)
...

It looks like the file is fragmented. During load the start address 
becomes unaligned and thereby the code wrongly tries to transfer more 
blocks than possible.

Before the above mentioned patch the max_xfer_blk was much smaller (20), 
so that the problem never appears.
> The proper solution would be to introduce a bounce buffer for such unaligned
> transfers. A proper, generic bounce buffer that can be configured to bounce on
> specified boundaries and warns about performance penalties.
Why not limit the max_xfer_blk to the maximum save count [(4096 * 4) / 
dev_desc->blksz)] or calculate the max_xfer_blk depending on the start 
address of the transfer:

max_xfer_blk = ((4096 * 5) - (start & ~4096) / dev_desc->blksz

Regards,
     Stefan

[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> Am 07.07.2012 23:58, schrieb Marek Vasut:
> >> Am 04.07.2012 08:57, schrieb Schneider, Kolja:
> >>>> Am 03.07.2012 20:10, schrieb Marek Vasut:
> >>>>>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> >>>>>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> >>>>>> usb_storage as it wrongly assumes that every transfer can use 4k
> >>>>>> per qt_buffer. This is wrong if the start address of the data
> >>>>>> is not 4k aligned and leads to 'EHCI timed out on TD' messages
> >>>>>> because of 'out of buffer pointers' in ehci_td_buffer function.
> >>>>>> 
> >>>>>> Cc: Marek Vasut <marex@denx.de>
> >>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> >>>>> 
> >>>>> Ok, first I have to admit I broke my promise to look into this ASAP,
> >>>>> sorry
> >>>> 
> >>>> about
> >>>> 
> >>>>> it :-(
> >>>> 
> >>>> No problem, as long as we get it into the next release. ;-)
> >>>> 
> >>>>> Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1
> >>>>> ?
> >>>> 
> >>>> No, because the first blk need to be aligned with the 4096. In worst
> >>>> case the blk is at the end of the 4096 range. If we assume that the
> >>>> blk is aligned to blk_sz we can change it to ((4096 * 4) /
> >>>> dev_desc->blk_sz) + 1. I skip the last blk (+ 1) because with 4096
> >>>> aligned first blk we unaligned the next transfer and add extra short
> >>>> packages to each ehci transfer.
> >>>> 
> >>>> If we want to maximise the usage we need to calculate the max_xfer_blk
> >>>> depending on the start address of the first blk.
> >>> 
> >>> I admit to not totally getting it. However, there are two things that
> >>> come
> > 
> > to my mind:
> >>>    - 	Doesn't the EHCI Specification mention exactly five buffers that
> >>>    can/should/must
> >>>    
> >>>      	be used?
> >> 
> >> Yes, you can use up to five 4096 byte buffers.
> >> 
> >>>    - 	I think I once stumbled across some comment that said as much as
> > 
> > the
> > 
> >>>    blocks
> >>> 	
> >>> 	always having to be aligned anyway?
> >> 
> >> The buffers must be aligned to a 4096 byte page. This means that you
> >> have to use the first and last buffer to align your data to the next or
> >> previous 4096 byte page boundary.
> > 
> > All right, I managed to replicate the issue. This (or similar) doesn't
> > work for you, right:
> > 
> > usb read 0x42000004 0x0 0x400
> 
> I observe the bug during
> fatload usb 0 0x800000 uImage
> 
> ...
> dev=0ffb0440, pipe=c8008283, buffer=00a90000, length=20480, req=(null)
> ...
> dev=0ffb0440, pipe=c8008283, buffer=00a95000, length=20480, req=(null)
> ...
> dev=0ffb0440, pipe=c8008283, buffer=00a9a000, length=18432, req=(null)
> ...
> dev=0ffb0440, pipe=c8008283, buffer=00a9e800, length=20480, req=(null)
> out of buffer pointers (2048 bytes left)
> ...
> 
> It looks like the file is fragmented. During load the start address
> becomes unaligned and thereby the code wrongly tries to transfer more
> blocks than possible.
> 
> Before the above mentioned patch the max_xfer_blk was much smaller (20),
> so that the problem never appears.
> 
> > The proper solution would be to introduce a bounce buffer for such
> > unaligned transfers. A proper, generic bounce buffer that can be
> > configured to bounce on specified boundaries and warns about performance
> > penalties.
> 
> Why not limit the max_xfer_blk to the maximum save count [(4096 * 4) /
> dev_desc->blksz)] or calculate the max_xfer_blk depending on the start
> address of the transfer:
> 
> max_xfer_blk = ((4096 * 5) - (start & ~4096) / dev_desc->blksz

This looks like a much better solution ;-) Can you redo the patch for that? 
Also, there's some macro for that rounding in include/common.h

> 
> Regards,
>      Stefan

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
usb_storage as it wrongly assumes that every transfer can use
4096 bytes per qt_buffer. This is wrong if the start address of
the data is not page aligned to 4096 bytes and leads to 'EHCI
timed out on TD' messages because of 'out of buffer pointers'
in ehci_td_buffer function.

The bug appears during load of a fragmented file and
read from or write to an unaligned memory address.

Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>

---
Changes for v2:
 - Replace fixed worst case calculation with dynamic
   computation based on start address of transfer

 common/usb_storage.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index faad237..bdc306f 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -150,12 +150,17 @@ struct us_data {
 	unsigned int	irqpipe;	 	/* pipe for release_irq */
 	unsigned char	irqmaxp;		/* max packed for irq Pipe */
 	unsigned char	irqinterval;		/* Intervall for IRQ Pipe */
-	unsigned long	max_xfer_blk;		/* Max blocks per xfer */
 	ccb		*srb;			/* current srb */
 	trans_reset	transport_reset;	/* reset routine */
 	trans_cmnd	transport;		/* transport routine */
 };
 
+/*
+ * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
+ * of 4096 bytes in a transfer without running itself out of qt_buffers
+ */
+#define USB_MAX_XFER_BLK(start, blksz)	(((4096 * 5) - (start % 4096)) / blksz)
+
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
 
 
@@ -1041,7 +1046,7 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor,
 unsigned long usb_stor_read(int device, unsigned long blknr,
 			    unsigned long blkcnt, void *buffer)
 {
-	unsigned long start, blks, buf_addr;
+	unsigned long start, blks, buf_addr, max_xfer_blk;
 	unsigned short smallblks;
 	struct usb_device *dev;
 	struct us_data *ss;
@@ -1083,12 +1088,14 @@ unsigned long usb_stor_read(int device, unsigned long blknr,
 		/* XXX need some comment here */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > ss->max_xfer_blk)
-			smallblks = ss->max_xfer_blk;
+		max_xfer_blk = USB_MAX_XFER_BLK(buf_addr,
+						usb_dev_desc[device].blksz);
+		if (blks > max_xfer_blk)
+			smallblks = (unsigned short) max_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == ss->max_xfer_blk)
+		if (smallblks == max_xfer_blk)
 			usb_show_progress();
 		srb->datalen = usb_dev_desc[device].blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1109,7 +1116,7 @@ retry_it:
 			start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= ss->max_xfer_blk)
+	if (blkcnt >= max_xfer_blk)
 		debug("\n");
 	return blkcnt;
 }
@@ -1117,7 +1124,7 @@ retry_it:
 unsigned long usb_stor_write(int device, unsigned long blknr,
 				unsigned long blkcnt, const void *buffer)
 {
-	unsigned long start, blks, buf_addr;
+	unsigned long start, blks, buf_addr, max_xfer_blk;
 	unsigned short smallblks;
 	struct usb_device *dev;
 	struct us_data *ss;
@@ -1162,12 +1169,14 @@ unsigned long usb_stor_write(int device, unsigned long blknr,
 		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > ss->max_xfer_blk)
-			smallblks = ss->max_xfer_blk;
+		max_xfer_blk = USB_MAX_XFER_BLK(buf_addr,
+						usb_dev_desc[device].blksz);
+		if (blks > max_xfer_blk)
+			smallblks = (unsigned short) max_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == ss->max_xfer_blk)
+		if (smallblks == max_xfer_blk)
 			usb_show_progress();
 		srb->datalen = usb_dev_desc[device].blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1188,7 +1197,7 @@ retry_it:
 			start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= ss->max_xfer_blk)
+	if (blkcnt >= max_xfer_blk)
 		debug("\n");
 	return blkcnt;
 
@@ -1415,12 +1424,6 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	USB_STOR_PRINTF(" address %d\n", dev_desc->target);
 	USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
 
-	/*
-	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a
-	 * transfer without running itself out of qt_buffers.
-	 */
-	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
-
 	init_part(dev_desc);
 
 	USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
-- 
1.7.9.5

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> usb_storage as it wrongly assumes that every transfer can use
> 4096 bytes per qt_buffer. This is wrong if the start address of
> the data is not page aligned to 4096 bytes and leads to 'EHCI
> timed out on TD' messages because of 'out of buffer pointers'
> in ehci_td_buffer function.

Yes, this can be simply confirmed even with USB stick by loading to unaligned 
address. It'll make the buffers overflow too.

> The bug appears during load of a fragmented file and
> read from or write to an unaligned memory address.
> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> 
> ---
> Changes for v2:
>  - Replace fixed worst case calculation with dynamic
>    computation based on start address of transfer
> 
>  common/usb_storage.c |   37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index faad237..bdc306f 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -150,12 +150,17 @@ struct us_data {
>  	unsigned int	irqpipe;	 	/* pipe for release_irq */
>  	unsigned char	irqmaxp;		/* max packed for irq Pipe */
>  	unsigned char	irqinterval;		/* Intervall for IRQ Pipe */
> -	unsigned long	max_xfer_blk;		/* Max blocks per xfer */
>  	ccb		*srb;			/* current srb */
>  	trans_reset	transport_reset;	/* reset routine */
>  	trans_cmnd	transport;		/* transport routine */
>  };
> 
> +/*
> + * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
> + * of 4096 bytes in a transfer without running itself out of qt_buffers
> + */
> +#define USB_MAX_XFER_BLK(start, blksz)	(((4096 * 5) - (start % 4096)) /
> blksz) +

Can't something in include/common.h around line 900 can't be used?

btw put braces around (start) in the macro and around (blksz) .

[...]

The rest is good, thanks! :-)

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

Am 10.07.2012 04:12, schrieb Marek Vasut:
>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
>> usb_storage as it wrongly assumes that every transfer can use
>> 4096 bytes per qt_buffer. This is wrong if the start address of
>> the data is not page aligned to 4096 bytes and leads to 'EHCI
>> timed out on TD' messages because of 'out of buffer pointers'
>> in ehci_td_buffer function.
> Yes, this can be simply confirmed even with USB stick by loading to unaligned
> address. It'll make the buffers overflow too.
>
>> The bug appears during load of a fragmented file and
>> read from or write to an unaligned memory address.
>>
>> Cc: Marek Vasut <marex@denx.de>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>
>> ---
>> Changes for v2:
>>   - Replace fixed worst case calculation with dynamic
>>     computation based on start address of transfer
>>
>>   common/usb_storage.c |   37 ++++++++++++++++++++-----------------
>>   1 file changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index faad237..bdc306f 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -150,12 +150,17 @@ struct us_data {
>>   	unsigned int	irqpipe;	 	/* pipe for release_irq */
>>   	unsigned char	irqmaxp;		/* max packed for irq Pipe */
>>   	unsigned char	irqinterval;		/* Intervall for IRQ Pipe */
>> -	unsigned long	max_xfer_blk;		/* Max blocks per xfer */
>>   	ccb		*srb;			/* current srb */
>>   	trans_reset	transport_reset;	/* reset routine */
>>   	trans_cmnd	transport;		/* transport routine */
>>   };
>>
>> +/*
>> + * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
>> + * of 4096 bytes in a transfer without running itself out of qt_buffers
>> + */
>> +#define USB_MAX_XFER_BLK(start, blksz)	(((4096 * 5) - (start % 4096)) /
>> blksz) +
> Can't something in include/common.h around line 900 can't be used?
If you mean the round functions I don't need them, as I need the
leftover of 4096 and I need to divide round down the count.

> btw put braces around (start) in the macro and around (blksz) .
I will send a v3 tonight.

Regards,
     Stefan

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> Am 10.07.2012 04:12, schrieb Marek Vasut:
> >> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> >> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> >> usb_storage as it wrongly assumes that every transfer can use
> >> 4096 bytes per qt_buffer. This is wrong if the start address of
> >> the data is not page aligned to 4096 bytes and leads to 'EHCI
> >> timed out on TD' messages because of 'out of buffer pointers'
> >> in ehci_td_buffer function.
> > 
> > Yes, this can be simply confirmed even with USB stick by loading to
> > unaligned address. It'll make the buffers overflow too.
> > 
> >> The bug appears during load of a fragmented file and
> >> read from or write to an unaligned memory address.
> >> 
> >> Cc: Marek Vasut <marex@denx.de>
> >> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> >> 
> >> ---
> >> 
> >> Changes for v2:
> >>   - Replace fixed worst case calculation with dynamic
> >>   
> >>     computation based on start address of transfer
> >>   
> >>   common/usb_storage.c |   37 ++++++++++++++++++++-----------------
> >>   1 file changed, 20 insertions(+), 17 deletions(-)
> >> 
> >> diff --git a/common/usb_storage.c b/common/usb_storage.c
> >> index faad237..bdc306f 100644
> >> --- a/common/usb_storage.c
> >> +++ b/common/usb_storage.c
> >> @@ -150,12 +150,17 @@ struct us_data {
> >> 
> >>   	unsigned int	irqpipe;	 	/* pipe for release_irq */
> >>   	unsigned char	irqmaxp;		/* max packed for irq Pipe */
> >>   	unsigned char	irqinterval;		/* Intervall for IRQ Pipe */
> >> 
> >> -	unsigned long	max_xfer_blk;		/* Max blocks per xfer */
> >> 
> >>   	ccb		*srb;			/* current srb */
> >>   	trans_reset	transport_reset;	/* reset routine */
> >>   	trans_cmnd	transport;		/* transport routine */
> >>   
> >>   };
> >> 
> >> +/*
> >> + * The U-Boot EHCI driver cannot handle more than 5 page aligned
> >> buffers + * of 4096 bytes in a transfer without running itself out of
> >> qt_buffers + */
> >> +#define USB_MAX_XFER_BLK(start, blksz)	(((4096 * 5) - (start % 4096)) /
> >> blksz) +
> > 
> > Can't something in include/common.h around line 900 can't be used?
> 
> If you mean the round functions I don't need them, as I need the
> leftover of 4096 and I need to divide round down the count.
> 
> > btw put braces around (start) in the macro and around (blksz) .
> 
> I will send a v3 tonight.

Ok then, I think this is just perfect than and it should definitelly hit this 
release :-)

Thank you very much, sorry for pestering you too much and adding delays. Shame 
on my maintaining skills.

> Regards,
>      Stefan

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
usb_storage as it wrongly assumes that every transfer can use
4096 bytes per qt_buffer. This is wrong if the start address of
the data is not page aligned to 4096 bytes and leads to 'EHCI
timed out on TD' messages because of 'out of buffer pointers'
in ehci_td_buffer function.

The bug appears during load of a fragmented file and
read from or write to an unaligned memory address.

Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>

---
Changes for v2:
 - Replace fixed worst case calculation with dynamic
   computation based on start address of transfer
Changes for v3:
 - Add parentheses within macro around parameters

 common/usb_storage.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index faad237..c528979 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -150,12 +150,18 @@ struct us_data {
 	unsigned int	irqpipe;	 	/* pipe for release_irq */
 	unsigned char	irqmaxp;		/* max packed for irq Pipe */
 	unsigned char	irqinterval;		/* Intervall for IRQ Pipe */
-	unsigned long	max_xfer_blk;		/* Max blocks per xfer */
 	ccb		*srb;			/* current srb */
 	trans_reset	transport_reset;	/* reset routine */
 	trans_cmnd	transport;		/* transport routine */
 };
 
+/*
+ * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
+ * of 4096 bytes in a transfer without running itself out of qt_buffers
+ */
+#define USB_MAX_XFER_BLK(start, blksz) \
+	(((4096 * 5) - ((start) % 4096)) / (blksz))
+
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
 
 
@@ -1041,7 +1047,7 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor,
 unsigned long usb_stor_read(int device, unsigned long blknr,
 			    unsigned long blkcnt, void *buffer)
 {
-	unsigned long start, blks, buf_addr;
+	unsigned long start, blks, buf_addr, max_xfer_blk;
 	unsigned short smallblks;
 	struct usb_device *dev;
 	struct us_data *ss;
@@ -1083,12 +1089,14 @@ unsigned long usb_stor_read(int device, unsigned long blknr,
 		/* XXX need some comment here */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > ss->max_xfer_blk)
-			smallblks = ss->max_xfer_blk;
+		max_xfer_blk = USB_MAX_XFER_BLK(buf_addr,
+						usb_dev_desc[device].blksz);
+		if (blks > max_xfer_blk)
+			smallblks = (unsigned short) max_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == ss->max_xfer_blk)
+		if (smallblks == max_xfer_blk)
 			usb_show_progress();
 		srb->datalen = usb_dev_desc[device].blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1109,7 +1117,7 @@ retry_it:
 			start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= ss->max_xfer_blk)
+	if (blkcnt >= max_xfer_blk)
 		debug("\n");
 	return blkcnt;
 }
@@ -1117,7 +1125,7 @@ retry_it:
 unsigned long usb_stor_write(int device, unsigned long blknr,
 				unsigned long blkcnt, const void *buffer)
 {
-	unsigned long start, blks, buf_addr;
+	unsigned long start, blks, buf_addr, max_xfer_blk;
 	unsigned short smallblks;
 	struct usb_device *dev;
 	struct us_data *ss;
@@ -1162,12 +1170,14 @@ unsigned long usb_stor_write(int device, unsigned long blknr,
 		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > ss->max_xfer_blk)
-			smallblks = ss->max_xfer_blk;
+		max_xfer_blk = USB_MAX_XFER_BLK(buf_addr,
+						usb_dev_desc[device].blksz);
+		if (blks > max_xfer_blk)
+			smallblks = (unsigned short) max_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == ss->max_xfer_blk)
+		if (smallblks == max_xfer_blk)
 			usb_show_progress();
 		srb->datalen = usb_dev_desc[device].blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1188,7 +1198,7 @@ retry_it:
 			start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= ss->max_xfer_blk)
+	if (blkcnt >= max_xfer_blk)
 		debug("\n");
 	return blkcnt;
 
@@ -1415,12 +1425,6 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	USB_STOR_PRINTF(" address %d\n", dev_desc->target);
 	USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
 
-	/*
-	 * The U-Boot EHCI driver cannot handle more than 4096 * 5 bytes in a
-	 * transfer without running itself out of qt_buffers.
-	 */
-	ss->max_xfer_blk = (4096 * 5) / dev_desc->blksz;
-
 	init_part(dev_desc);
 
 	USB_STOR_PRINTF("partype: %d\n", dev_desc->part_type);
-- 
1.7.9.5

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> usb_storage as it wrongly assumes that every transfer can use
> 4096 bytes per qt_buffer. This is wrong if the start address of
> the data is not page aligned to 4096 bytes and leads to 'EHCI
> timed out on TD' messages because of 'out of buffer pointers'
> in ehci_td_buffer function.
> 
> The bug appears during load of a fragmented file and
> read from or write to an unaligned memory address.
> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>

This one is good :-)

Thank you, Stefan! For your contribution and your patience.

I'll test it on friday and I'll talk to WD to include it in current release. The 
fix looks very reasonable. Though I'd like to get some tests done. Tom, Ilya, 
can you possibly try this on your toys please?
[...]

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Ilya Yanok]

Hi,

The patch is good in the sense it does fix the real problem. But I wonder
if it's a good idea to expose lower layer details (like size/number of
buffers per EHCI TD) to upper layer driver? I know EHCI is most common USB
HCD but we have drivers for a bunch of others... How about them?

Regards, Ilya.

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Ilya Yanok,

> Hi,
> 
> The patch is good in the sense it does fix the real problem. But I wonder
> if it's a good idea to expose lower layer details (like size/number of
> buffers per EHCI TD) to upper layer driver? I know EHCI is most common USB
> HCD but we have drivers for a bunch of others... How about them?

No, it's not ... it's yet another usb-is-full-of-crap-in-uboot kind of bugs :-(

I'd like to queue this for the current release, we can work on proper rework for 
-next. Agreed?

> Regards, Ilya.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Ilya Yanok]

Dear Marek,

On Sun, Jul 15, 2012 at 12:14 PM, Marek Vasut <marex@denx.de> wrote:

> > The patch is good in the sense it does fix the real problem. But I wonder
> > if it's a good idea to expose lower layer details (like size/number of
> > buffers per EHCI TD) to upper layer driver? I know EHCI is most common
> USB
> > HCD but we have drivers for a bunch of others... How about them?
>
>
> No, it's not ... it's yet another usb-is-full-of-crap-in-uboot kind of
> bugs :-(
>
> I'd like to queue this for the current release, we can work on proper
> rework for
> -next. Agreed?
>

Agreed,

Regards, Ilya.

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Wolfgang Denk,

> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> usb_storage as it wrongly assumes that every transfer can use
> 4096 bytes per qt_buffer. This is wrong if the start address of
> the data is not page aligned to 4096 bytes and leads to 'EHCI
> timed out on TD' messages because of 'out of buffer pointers'
> in ehci_td_buffer function.
> 
> The bug appears during load of a fragmented file and
> read from or write to an unaligned memory address.
> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>

I'd like to get this patch:

http://patchwork.ozlabs.org/patch/170256/

applied into current release. If you consider a pullRQ is better, I'll prepare 
one.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Ilya Yanok,

> Dear Marek,
> 
> On Sun, Jul 15, 2012 at 12:14 PM, Marek Vasut <marex@denx.de> wrote:
> > > The patch is good in the sense it does fix the real problem. But I
> > > wonder if it's a good idea to expose lower layer details (like
> > > size/number of buffers per EHCI TD) to upper layer driver? I know EHCI
> > > is most common
> > 
> > USB
> > 
> > > HCD but we have drivers for a bunch of others... How about them?
> > 
> > No, it's not ... it's yet another usb-is-full-of-crap-in-uboot kind of
> > bugs :-(
> > 
> > I'd like to queue this for the current release, we can work on proper
> > rework for
> > -next. Agreed?
> 
> Agreed,

Thanks! I'm marking this important in my mailbox to avoid missing that we need 
to fix this. Ilya, can you check the driver model papers and see if we can 
somehow integrate this into that?

> Regards, Ilya.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Ilya Yanok]

Dear Marek,

On Sun, Jul 15, 2012 at 1:51 PM, Marek Vasut <marex@denx.de> wrote:

>
> Thanks! I'm marking this important in my mailbox to avoid missing that we
> need
> to fix this. Ilya, can you check the driver model papers and see if we can
> somehow integrate this into that?
>
>
I will try to.

Regards, Ilya.

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Marek Vasut]

Dear Stefan Herbrechtsmeier,

> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
> usb_storage as it wrongly assumes that every transfer can use
> 4096 bytes per qt_buffer. This is wrong if the start address of
> the data is not page aligned to 4096 bytes and leads to 'EHCI
> timed out on TD' messages because of 'out of buffer pointers'
> in ehci_td_buffer function.
> 
> The bug appears during load of a fragmented file and
> read from or write to an unaligned memory address.
> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>

[...]

I applied this and pushed into u-boot-usb. Thanks again for finding and fixing 
this.

I'm still uncertain if we should add it into current release so late, probably 
not though. Therefore I'll queue it for next, ok?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] usb_storage: fix ehci driver max transfer size

[Stefan Herbrechtsmeier]

Am 18.07.2012 14:50, schrieb Marek Vasut:
>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage:
>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in
>> usb_storage as it wrongly assumes that every transfer can use
>> 4096 bytes per qt_buffer. This is wrong if the start address of
>> the data is not page aligned to 4096 bytes and leads to 'EHCI
>> timed out on TD' messages because of 'out of buffer pointers'
>> in ehci_td_buffer function.
>>
>> The bug appears during load of a fragmented file and
>> read from or write to an unaligned memory address.
>>
>> Cc: Marek Vasut <marex@denx.de>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> [...]
>
> I applied this and pushed into u-boot-usb. Thanks again for finding and fixing
> this.
>
> I'm still uncertain if we should add it into current release so late, probably
> not though. Therefore I'll queue it for next, ok?
Given that there was nobody other than me with the problem until now, it 
should be okay.

Regards,
     Stefan