Dear Stefan Herbrechtsmeier,
[...]
What do you mean by "partial USB transfers"? As seen from EHCI users like the MSC driver (usb_storage.c), USB transfers either succeed or fail, but they cannot be "segmented".
Segmented -- like multiple transfers being issues with small payload?
Right.
You can not put these together at the USB-level, since it's the issuing code that has to be fixed.
If the segmentation comes from the file system handling we can not avoid this.
If the FS code is shitty or the device is fragmented, noone can help that.
[...]
My code assumes that wMaxPacketSize is a power of 2. This is not always true for interrupt endpoints. Let's talk about these. Their handling is currently broken in U-Boot since their transfers are made asynchronous instead of periodic. Devices shouldn't care too much about that, as long as transfers do not exceed wMaxPacketSize, in which case my code still works because wMaxPacketSize always fits in a single qTD. Interrupt transfers larger than wMaxPacketSize do not seem to be used by U-Boot. If they were used, the current code in U-Boot would have a timing issue because the asynchronous scheme would break the interval requested by devices, which could at worst make them fail in some way. So the only solution would be that such transfers be split by the caller of submit_int_msg, in which case my code still works. What would you think about failing with an error message in submit_int_msg if length is larger than wMaxPacketSize? Marek, what do you think?
Let's do that ... I think the interrupt endpoint is only used for keyboard and if someone needs it for something else, the code will be there, just needing improvement. Comment and error message are OK.
OK. I have thought of another solution for this. You'll tell me which one you prefer.
The ehci_submit_async code currently in U-Boot checks through ehci_td_buffer that length fits in the single qTD reserved for data payload only after work has begun, possibly after a SETUP transfer. With my series, this is checked at the very beginning, before the allocation. We could detect that wMaxPacketSize is not a power of 2 (e.g. with __builtin_popcount), in which case the allocation for the data payload would be restricted to 1 qTD like now, and there would be a check at the very beginning to test if length fits in this qTD. In that way, there could be several packets per interrupt transfer as long as it fits in a single qTD, just like now, contrary to the limitation imposed by the error in submit_int_msg. But I'm not sure it's a good idea to allow this behavior.
I think this is not needed, as there is only one user (keyboard) with max size of 8 byte.
I agree, but for a different reason. Let's aim for a simple implementation first that doesn't change behavior and add this change later _if_ needed.
[...]
So we could perhaps issue a #error in ehci-hcd or in usb_storage if CONFIG_SYS_MALLOC_LEN is not large enough, but I don't think it's a good
idea because:
- the threshold value would have to depend on runtime block sizes
or
something, which could lead to a big worst worst case that would almost never happen in real life, so giving such an unrealistic heap size constraint would be cumbersome,
#warning then?
With which limit if so?
I would expect more than 128kB as this is a common worst case (512 B block size).
Seems to be reasonable.
[..]