From 696e8c4b94084bdf0971147d5019d61126c1b553 Mon Sep 17 00:00:00 2001
From: Purdea Andrei <andrei@purdea.ro>
Date: Fri, 27 Sep 2024 00:39:29 +0300
Subject: [PATCH] Add SETUP_EP0_OUT_BUF(), to be used for OUT control
 transfers.

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:

```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.
---
 firmware/library/include/fx2usb.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/firmware/library/include/fx2usb.h b/firmware/library/include/fx2usb.h
index 77745f8..c9ddb0b 100644
--- a/firmware/library/include/fx2usb.h
+++ b/firmware/library/include/fx2usb.h
@@ -53,8 +53,12 @@ void usb_init(bool disconnect);
   } while(0)
 
 /**
- * Configure EP0 for an IN or OUT transfer from or to `EP0BUF`.
- * For an OUT transfer, specify `length` as `0`.
+ * Configure EP0 for an IN transfer from `EP0BUF`.
+ *
+ * Using this for an OUT transfer is deprecated, because it exposes a
+ * race condition. For OUT transfers please use one or more calls to
+ * `SETUP_EP0_OUT_BUF()` instead followed by a single call to `ACK_EP0()`.
+ * Do not call `ACK_EP0()` before processing all pending data in `EP0BUF`
  */
 #define SETUP_EP0_BUF(length) \
   do { \
@@ -64,6 +68,13 @@ void usb_init(bool disconnect);
     EP0CS = _HSNAK; \
   } while(0)
 
+#define SETUP_EP0_OUT_BUF() \
+  do { \
+    SUDPTRCTL = _SDPAUTO; \
+    EP0BCH = 0; \
+    EP0BCL = 0; \
+  } while(0)
+
 /**
  * Acknowledge an EP0 SETUP or OUT transfer.
  */