Add SETUP_EP0_OUT_BUF(), to be used for OUT control transfers. #18
Conversation
|
MORE PRs to follow to fix this for glasgow, and some examples in libfx2 |
|
Thanks so much for handling this, I'm really happy that you are looking into it! Right now I'm feeling too sick to review the code but I'll do it as soon as I can. I've known about what I think is this exact issue for a while but never found the time to handle it. |
Before this change, the EP0BUF vuffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
I hope you will be better soon! |
|
I'm disabled; it does not really get better. But I may be able to work out some time for my OSS projects, since I want to work on them still. |
Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
|
Thank you, the analysis and the fix look all correct to me. Could you please add a pair of macros For deprecation, I propose making it rely on a data symbol called something like |
|
Also, I would strongly prefer to upgrade all of the examples as well, since in practice half of the people who use the library at all will just copy&paste example code. |
purdeaandrei
force-pushed
the
f_add_SETUP_EP0_OUT_BUF
branch
from
696e8c4 to
7b0ed57
Compare
DONE
Could you clarify why we'd need an actual symbol here, instead of just a macro? #define SETUP_EP0_BUF(length) \
do { \
if (SETUP_EP0_BUF_is_deprecated_use_SETUP_EP0_IN_BUF_or_SETUP_EP0_OUT_BUF_instead) { \
SUDPTRCTL = _SDPAUTO; \
EP0BCH = 0; \
EP0BCL = length; \
EP0CS = _HSNAK; \
}\
} while(0)Then the error message would be And code size would remain unchanged. However if I were to do something like: extern uint8_t SETUP_EP0_BUF_is_deprecated_use_SETUP_EP0_IN_BUF_or_SETUP_EP0_OUT_BUF_instead;
#define SETUP_EP0_BUF(length) \
do { \
SUDPTRCTL = _SDPAUTO; \
EP0BCH = 0; \
EP0BCL = length + SETUP_EP0_BUF_is_deprecated_use_SETUP_EP0_IN_BUF_or_SETUP_EP0_OUT_BUF_instead; \
EP0CS = _HSNAK; \
} while(0)Then the error message would be: Then you could make it go away by adding something like this to one of your .c files: uint8_t SETUP_EP0_BUF_is_deprecated_use_SETUP_EP0_IN_BUF_or_SETUP_EP0_OUT_BUF_instead;But this would result in added code size, not only to store the symbol, but to also access it every time Wouldn't it be better to just use a missing identifier?
Of coure. I presume then, that if libfx2 will error out by default with the deprecated function, you'd prefer the fixes to the examples as part of the same PR, right? |
When processing OUT control transfers, the EP0 buffer is armed when
wriging EP0BCL, while SDPAUTO is high. At this stage it's not
necessary to set HSNAK, because that bit has an effect on the status
stage, not on the data stage.
Beyond it being not necessary to write HSNAK early, it introduces a
race condition, because the host sees the status stage complete early,
before we have fully processed the content of EP0BUF, and as such it
thinks it can send more control transfers. If the host actually sends
more control transfers, then EP0BUF might be overwritten before we have
completed processing EP0BUF, and this can cause data corruption.
The clean way to handle this, is to force the host to wait in the status
stage until we have fully processed the EP0BUF, by NAK-ink the status
stage. This does not mean that the packets in the Data stage will be
NAK'd because, that is controlled by whether the buffer is armed or not.
This commit deprecates `SETUP_EP0_BUF(length)`, and introduces:
- `SETUP_EP0_IN_BUF(length)` as a direct replacement for IN transfers
- `SETUP_EP0_OUT_BUF()` as a new macro to perform OUT transfers
The new recommended way to do OUT control transfers is:
```c
for (each_expected_packet) {
SETUP_EP0_OUT_BUF();
handle_packet(EP0BUF, EP0BCL);
// EP0BCL will have the size of the received packet
}
ACK_EP0();
```
That is: only call `ACK_EP0()`, which clears HSNAK by writing 1 to it,
after we know it's safe to overwrite EP0BUF.
Note: a simple way to reproduce the issue when not following this
procedure, is to run:
```bash
while true; do lsusb -v -d $(DEVICE_VID):$(DEVICE_PID) > /dev/null; done
```
In a terminal. (replacing DEVICE_VID/DEVICE_PID with correct values)
While this loop is running, we expected most applications doing control
out transfers with the old method to become unusable.
Technically speaking a single run of `lsusb` could even cause trouble if
it happes at the wrong time, and I think this could be triggered in
other situations as well, this is just the easiest way to reproduce the
problem.
This is now used instead of SETUP_EP0_BUF whenever setting up an IN transfer. This commit doesn't introduce any kind of functional change.
This commit doesn't introduce any kind of functional change. The cdc-acm example wasn't vulnerable to the race condition caused by using `SETUP_EP0_BUF(0)`. I have tested that both the original and new versions of cdc-acm work correctly.
I have tested the following boot-cypress writing eeprom, while executing the following in the background: `while true; do lsusb -v -d 04b4:8613 > /dev/null; done` With the old version of boot-cypress this results in corrupted data. With the new version of boot-cypress this works correctly.
I have tested boot-dfu downloading firmware with dfu-util, while executing the following in the background, as root: `while true; do lsusb -v -d 04b4:8613 > /dev/null; done` With old version of boot-dfu this results in corrupted data. With the new version of boot-dfu this works correctly. I had to allocate an additonal scratch2 buffer. We cannot use the normal scratch buffer, because it's used when getting descriptors, and the OS may send GET_DESCRIPTOR requests at any time. I have also tested the new version of boot-dfu-spiflash, but only flashing the eeprom. I have also tested the new version of boot-uf2-dfu in dfu-mode only.
purdeaandrei
force-pushed
the
f_add_SETUP_EP0_OUT_BUF
branch
from
7b0ed57 to
7a68b79
Compare
|
Hello,
This PR is ready for review |
It's not a big deal IMO; I value correctness much more than space saving. If this becomes an issue in the future we can always find a solution later; it's perfectly fine for the examples.
I think your proposal makes more sense than mine; let's do that! |
… TODO MEASURE AGAIN). Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
|
(I haven't merged the PR until now because GitHub doesn't automatically send an e-mail if you update a past commit via force-push.) |
Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
Especially to include fixes from whitequark#18 and whitequark#25 This time boot-cypress is built with 4.5.0 #15242. For future builds please run from the root of the repository: `./software/deploy-bootloader.sh` The deploy script was adapted from the one used in glasgow firmware.
Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
Before this change, the EP0BUF buffer used by control out transfers could be overwritten by new control transfers before the firmware is finished processing the previous control transfer. The easiest way to illustrate this problem would be to run in a different terminal the following: ```bash while true; do lsusb -v -d 20b7:9db1 > /dev/null; done ``` While this is running, glasglow is completely unusable. Presumably even a single lsusb run could cause corruption, if it happens to be issued at the wrong time. With this change glasgow is now usable, even if the above loop is running. Please see whitequark/libfx2#18 for more details.
2 participants
When processing OUT control transfers, the EP0 buffer is armed when wriging EP0BCL, while SDPAUTO is high. At this stage it's not necessary to set HSNAK, because that bit has an effect on the status stage, not on the data stage.
Beyond it being not necessary to write HSNAK early, it introduces a race condition, because the host sees the status stage complete early, before we have fully processed the content of EP0BUF, and as such it thinks it can send more control transfers. If the host actually sends more control transfers, then EP0BUF might be overwritten before we have completed processing EP0BUF, and this can cause data corruption.
The clean way to handle this, is to force the host to wait in the status stage until we have fully processed the EP0BUF, by NAK-ink the status stage. This does not mean that the packets in the Data stage will be NAK'd because, that is controlled by whether the buffer is armed or not.
Firmware-wise the new recommended way to the OUT control transfers is:
That is: only call
ACK_EP0(), which clears HSNAK by writing 1 to it, after we know it's safe to overwrite EP0BUF.Note: a simple way to reproduce the issue when not following this procedure, is to run:
In a terminal. (replacing DEVICE_VID/DEVICE_PID with correct values) While this loop is running, we expected most applications doing control out transfers with the old method to become unusable.
Technically speaking a single run of
lsusbcould even cause trouble if it happes at the wrong time, and I think this could be triggered in other situations as well, this is just the easiest way to reproduce the problem.