usb: short control IN might need a ZLP

Control transfers can transfer less than was requested by the host in the
wLength field.  if this short transfer is a multiple of the endpoint's packet
size, a zero length packet must be sent.

Adds tests for a range of control transfer IN requests, and properly supports
this in the core.  Based heavily on work by Kuldeep Dhaka.

See https://github.com/libopencm3/libopencm3/pull/505
and https://github.com/libopencm3/libopencm3/pull/194 for original discussion.

Tested with stm32f4, stm32f103 and stm32l053.

lib/usb/usb_control.c

@@ -50,6 +50,24 @@ static void stall_transaction(usbd_device *usbd_dev)
 	usbd_dev->control_state.state = IDLE;
 }
 
+/**
+ * If we're replying with _some_ data, but less than the host is expecting,
+ * then we normally just do a short transfer.  But if it's short, but a
+ * multiple of the endpoint max packet size, we need an explicit ZLP.
+ * @param len how much data we want to transfer
+ * @param wLength how much the host asked for
+ * @param ep_size
+ * @return
+ */
+static bool needs_zlp(uint16_t len, uint16_t wLength, uint8_t ep_size) {
+	if (len < wLength) {
+		if (len && (len % ep_size == 0)) {
+			return true;
+		}
+	}
+	return false;
+}
+
 /* Register application callback function for handling USB control requests. */
 int usbd_register_control_callback(usbd_device *usbd_dev, uint8_t type,
 				   uint8_t type_mask,
@@ -89,7 +107,10 @@ static void usb_control_send_chunk(usbd_device *usbd_dev)
 		usbd_ep_write_packet(usbd_dev, 0,
 				     usbd_dev->control_state.ctrl_buf,
 				     usbd_dev->control_state.ctrl_len);
-		usbd_dev->control_state.state = LAST_DATA_IN;
+
+		usbd_dev->control_state.state =
+			usbd_dev->control_state.needs_zlp ? DATA_IN : LAST_DATA_IN;
+		usbd_dev->control_state.needs_zlp = false;
 		usbd_dev->control_state.ctrl_len = 0;
 		usbd_dev->control_state.ctrl_buf = NULL;
 	}
@@ -153,7 +174,11 @@ static void usb_control_setup_read(usbd_device *usbd_dev,
 	usbd_dev->control_state.ctrl_len = req->wLength;
 
 	if (usb_control_request_dispatch(usbd_dev, req)) {
-		if (usbd_dev->control_state.ctrl_len) {
+		if (req->wLength) {
+			usbd_dev->control_state.needs_zlp =
+				needs_zlp(usbd_dev->control_state.ctrl_len,
+					req->wLength,
+					usbd_dev->desc->bMaxPacketSize0);
 			/* Go to data out stage if handled. */
 			usb_control_send_chunk(usbd_dev);
 		} else {

lib/usb/usb_private.h

@@ -74,6 +74,7 @@ struct _usbd_device {
 		uint8_t *ctrl_buf;
 		uint16_t ctrl_len;
 		usbd_control_complete_callback complete;
+		bool needs_zlp;
 	} control_state;
 
 	struct user_control_callback {

tests/gadget-zero/test_gadget0.py

@@ -209,3 +209,104 @@ class TestConfigSourceSinkPerformance(unittest.TestCase):
             txc += w
         te = datetime.datetime.now() - ts
         print("wrote %s bytes in %s for %s kps" % (txc, te, self.tput(txc, te)))
+
+
+class TestControlTransfer_Reads(unittest.TestCase):
+    """
+    https://github.com/libopencm3/libopencm3/pull/194
+    and
+    https://github.com/libopencm3/libopencm3/pull/505
+    """
+
+    def setUp(self):
+        self.dev = usb.core.find(idVendor=0xcafe, idProduct=0xcafe, custom_match=find_by_serial(DUT_SERIAL))
+        self.assertIsNotNone(self.dev, "Couldn't find locm3 gadget0 device")
+
+        self.cfg = uu.find_descriptor(self.dev, bConfigurationValue=2)
+        self.assertIsNotNone(self.cfg, "Config 2 should exist")
+        self.dev.set_configuration(self.cfg);
+        self.req = uu.CTRL_IN | uu.CTRL_TYPE_VENDOR | uu.CTRL_RECIPIENT_INTERFACE
+
+    def inner_t(self, wVal, read_len):
+        wVal = int(wVal)
+        read_len = int(read_len)
+        q = self.dev.ctrl_transfer(self.req, 2, wVal, 0, read_len)
+        self.assertEqual(len(q), wVal, "Should have read as much as we asked for?")
+
+    def tearDown(self):
+        uu.dispose_resources(self.dev)
+
+    def test_basic(self):
+        x = self.dev.ctrl_transfer(self.req, 2, 32, 0, 32)
+        self.assertEqual(32, len(x))
+
+    def test_matching_sizes(self):
+        """
+        Can we request x control in when we tell the device to produce x?
+        :return:
+        """
+        def inner(x):
+            x = int(x)
+            q = self.dev.ctrl_transfer(self.req, 2, x, 0, x)
+            self.assertEqual(len(q), x, "Should have read as much as we asked for")
+
+        ep0_size = self.dev.bMaxPacketSize0
+        inner(ep0_size)
+        inner(ep0_size * 3)
+        inner(ep0_size / 3)
+        inner(ep0_size - 7)
+        inner(ep0_size + 11)
+        inner(ep0_size * 4 + 11)
+
+    def test_waytoobig(self):
+        """
+        monster reads should fail, but not fatally.
+        (Don't make them too, big, or libusb will reject you outright, see MAX_CTRL_BUFFER_LENGTH in libusb sources)
+        """
+        try:
+            self.dev.ctrl_transfer(self.req, 2, 10 * self.dev.bMaxPacketSize0, 0, 10 * self.dev.bMaxPacketSize0)
+            self.fail("Should have got a stall")
+        except usb.core.USBError as e:
+            # Note, this might not be as portable as we'd like.
+            self.assertIn("Pipe", e.strerror)
+
+    def test_read_longer(self):
+        """
+        Attempt to read more than the device replied with.
+        This is explicitly allowed by spec:
+        "On an input request, a device must never return more data than is indicated
+        by the wLength value; it may return less"
+        """
+
+        ep0_size = self.dev.bMaxPacketSize0
+        self.inner_t(ep0_size / 2, ep0_size)
+        self.inner_t(ep0_size / 2, ep0_size * 2)
+        self.inner_t(ep0_size + 31, ep0_size * 5)
+
+    def test_read_needs_zlp(self):
+        ep0_size = self.dev.bMaxPacketSize0
+        self.inner_t(ep0_size, ep0_size + 10)
+        self.inner_t(ep0_size * 2, ep0_size * 5)
+
+    def test_read_zero(self):
+        """
+        try and read > 0, but have the device only produce 0
+        """
+        self.inner_t(0, self.dev.bMaxPacketSize0)
+        self.inner_t(0, 200)
+
+    def test_read_nothing(self):
+        """
+        Don't read anything, don't create anything (no data stage)
+        """
+        self.inner_t(0, 0)
+
+    def test_mean_limits(self):
+        """
+        tell the device to produce more than we ask for.
+        Note, this doesn't test the usb stack, it tests the application code behaves.
+        """
+        q = self.dev.ctrl_transfer(self.req, 2, 100, 0, 10)
+        self.assertEqual(len(q), 10, "In this case, should have gotten wLen back")
+
+

tests/gadget-zero/usb-gadget0.c

@@ -44,6 +44,7 @@
  * USB Vendor:Interface control requests.
  */
 #define GZ_REQ_SET_PATTERN	1
+#define GZ_REQ_PRODUCE		2
 #define INTEL_COMPLIANCE_WRITE 0x5b
 #define INTEL_COMPLIANCE_READ 0x5c
 
@@ -168,7 +169,7 @@ static const char * usb_strings[] = {
 };
 
 /* Buffer to be used for control requests. */
-static uint8_t usbd_control_buffer[128];
+static uint8_t usbd_control_buffer[5*64];
 static usbd_device *our_dev;
 
 /* Private global for state */
@@ -253,6 +254,21 @@ static int gadget0_control_request(usbd_device *usbd_dev,
 	case INTEL_COMPLIANCE_READ:
 		ER_DPRINTF("unimplemented!");
 		return USBD_REQ_NOTSUPP;
+	case GZ_REQ_PRODUCE:
+		ER_DPRINTF("fake loopback of %d\n", req->wValue);
+		if (req->wValue > sizeof(usbd_control_buffer)) {
+			ER_DPRINTF("Can't write more than out control buffer! %d > %d\n",
+				req->wValue, sizeof(usbd_control_buffer));
+			return USBD_REQ_NOTSUPP;
+		}
+		/* Don't produce more than asked for! */
+		if (req->wValue > req->wLength) {
+			ER_DPRINTF("Truncating reply to match wLen\n");
+			*len = req->wLength;
+		} else {
+			*len = req->wValue;
+		}
+		return USBD_REQ_HANDLED;
 	}
 	return USBD_REQ_NEXT_CALLBACK;
 }