Give the USB mutex back on every path out of control_exchange (#822, problem 5) - #915
Conversation
control_exchange() takes the mutex on entry and gives it back on every path out except one: a setup length other than 8 returns -10 while still holding it. Every later USB operation then waits out xSemaphoreTake and fails, which on this device means the keyboard, mass storage and the Ethernet adapter all stop at once -- and each subsequent operation pays the wait again, not just the first. That wait is 25 seconds, not 5. xSemaphoreTake() counts ticks, and configTICK_RATE_HZ is 200 (software/FreeRTOS/Source/FreeRTOSConfig.h:21), so the 5000 in this file is 25 s. Measured on the device rather than read off: 24997 ms. Earlier descriptions of this defect, mine included, said five seconds. Latent today, because no caller passes anything but 8. Found by reading, in the sweep in GideonZ#822, problem 5.
The commit before this one adds the fifth xSemaphoreGive to control_exchange, because the fourth path out had been missed. That is the shape of the defect: the release is written once per return, so a return added later is a return that holds the mutex forever, and nothing in the code says otherwise. control_exchange now takes the mutex, calls control_exchange_locked() and gives it back on the single way out. The body moves into that function unchanged -- no USB logic is touched, only the four releases it no longer needs. A path added inside it cannot leave the mutex held, whoever writes it. Measured on a C64 Ultimate, not argued. A debug build calls control_exchange once with a setup length of 9 and then reads a device descriptor, timing it: without this pair (b24b06fcd) wrong length -> -10, next transfer -> -9 after 24997 ms with it (3799d4f6b) wrong length -> -10, next transfer -> 18 after 0 ms Both images are the same firmware, built from the same tree with the same ESP32 part; they differ in the eleven lines below and nothing else. The probe is not in this branch: it belongs to the measurement, not to the fix.
|
@chrisgleissner — this PR and my two others, #843 and #916, are based on Could you confirm? If they should land on |
| uint16_t getSplitControl(int addr, int port, int speed, int type); | ||
| int getReceivedLength(int index); | ||
| int control_exchange(struct t_pipe *pipe, void *out, int outlen, void *in, int inlen); | ||
| int control_exchange_locked(struct t_pipe *pipe, void *out, int outlen, void *in, int inlen); |
There was a problem hiding this comment.
control_exchange_locked() is declared in the public section, so this change creates a callable path that skips the mutex entirely. A caller using that method can concurrently reuse descriptor 0 and setupBuffer, recreating the USB-transfer race that this wrapper is intended to prevent. Keep the locked helper private (alongside the other internal transfer helpers) so control_exchange() remains the only public entry point.
There was a problem hiding this comment.
You are right, and the reasoning is the same one the PR rests on: a second public entry that does not take the mutex is exactly the hole the wrapper is meant to close. Fixed in 2ff8e7359 — the declaration moves into the private section beside doPing() and the other internal helpers, so control_exchange() is again the only way in from outside. usb_base.cc:570 is the sole caller; there is no other in the whole software/ tree, so nothing else changes.
Built clean afterwards: make u64ii_no_esp in ghcr.io/gideonz/riscv, from a tree with every output/ and result/ under target/ removed first — 409 files compiled, exit 0, usb_base.cc translated in all three trees (ultimate, factorytest, update). The only warnings are the three in u64ii_tester.cc/u64ii_programmer.cc that master already emits.
And thanks for confirming master as the target branch — #843, #915 and #916 all sat on it already, so nothing to rebase.
|
Hi @enver-haase , thanks for this PR. I reviewed and left a comment. I confirm that the target branch going forward should be More details: #616 (comment) Thanks |
control_exchange_locked() was declared public, which makes it a second way into the transfer path -- one that does not take the mutex. A caller using it can reuse descriptor 0 and setupBuffer while another transfer is in flight, which is the race control_exchange() exists to prevent. Declared private, beside the other internal transfer helpers, control_exchange() is again the only public entry. Only usb_base.cc:570 calls it, so nothing else moves. Raised in review on GideonZ#915. Built clean: make u64ii_no_esp in ghcr.io/gideonz/riscv, every output/ and result/ under target/ removed first, 409 files compiled, MAKE_EXIT=0, and usb_base.cc translated in all three trees (ultimate, factorytest, update). The only warnings are the three u64ii_tester/u64ii_programmer ones that master already emits.
2 participants
control_exchange()takes the USB mutex on entry and gives it back on every path out except one: a setup length other than 8 returns-10while still holding it. Every later USB operation then waits outxSemaphoreTakeand fails — on this device that is the keyboard, mass storage and the Ethernet adapter at once, and each subsequent operation pays the wait again rather than just the first.Latent today, because no caller passes anything but 8. Found by reading, in the sweep in #822.
The wait is 25 seconds, not 5
This is worth saying plainly because I got it wrong myself, in #843 and in the commit message this PR now carries corrected.
xSemaphoreTake()counts ticks, andconfigTICK_RATE_HZis 200 (software/FreeRTOS/Source/FreeRTOSConfig.h:21), so the5000in this file is 25 s.Measured on the device rather than read off: 24997 ms.
Two commits, and the second is the point
The first adds the missing
xSemaphoreGive. The second removes the need to remember it:control_exchangenow takes the mutex, callscontrol_exchange_locked()and releases it on the single way out. The body moves into that function unchanged — no USB logic is touched, only the four releases it no longer needs.That is the shape the defect had. The release was written once per return, so a return added later is a return that holds the mutex forever, and nothing in the code said otherwise. Now a path added inside cannot leave it held, whoever writes it.
Measured on hardware, red and green
A debug build calls
control_exchangeonce with a setup length of 9 and then reads a device descriptor, timing it. Two images from the same tree, same ESP32 part, differing only in the eleven lines of this PR:Device: C64 Ultimate, FPGA 122, core 1.49, both images flashed from a stick and read back over the syslog service. The probe is deliberately not in this branch — it belongs to the measurement, not to the fix — and it releases the leaked mutex after measuring, so the device stays usable and the next image can be flashed from the same stick.
What is not here
No host test.
usb_base.ccreaches the Nano core through about fifty hardware symbols, and faking them to get the file onto a build host fakes away the thing under test. The property this PR buys is meant to be read rather than measured — one release, one exit, three lines apart — and the number above is what backs it.Split out of #843, which keeps the two network findings. The one-line commit that travelled there is reverted in that PR rather than dropped, so it stays a fast-forward and its review keeps its anchors.