diff --git a/lib/usb/usb_control.c b/lib/usb/usb_control.c index 1b87057a..69e14bb3 100644 --- a/lib/usb/usb_control.c +++ b/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 { diff --git a/lib/usb/usb_private.h b/lib/usb/usb_private.h index b38c1d67..4a3b9e0b 100644 --- a/lib/usb/usb_private.h +++ b/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 { diff --git a/tests/gadget-zero/test_gadget0.py b/tests/gadget-zero/test_gadget0.py index ddeff532..8aaef414 100644 --- a/tests/gadget-zero/test_gadget0.py +++ b/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") + + diff --git a/tests/gadget-zero/usb-gadget0.c b/tests/gadget-zero/usb-gadget0.c index ac49d0fb..9c119f9e 100644 --- a/tests/gadget-zero/usb-gadget0.c +++ b/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; }