From 4a48d4662919e701ab6b3a2262cfdd58b848283d Mon Sep 17 00:00:00 2001 From: Andrey Konovalov Date: Sat, 15 Jun 2024 00:50:14 +0200 Subject: [PATCH] Interrupt endpoint threads on interface altsetting change Proxyied devices that switch between interface altsettings do not work properly in certain cases: the endpoint handling threads might fail to be joined, as they might be blocked on libusb or Raw Gadget transfers. To interrupt Raw Gadget transfers, send the SIGUSR1 signal to the endpoint threads via pthread_kill and add a no-op handler for this signal. To avoid the endpoints threads being blocked forever on libusb sync I/O functions, add a timeout of 1 second for them. As a side-effect, this change makes the proxy print "Operation timed out" messages every second when there's no activity on an endpoint. We can remove them, but I think they might be valuable for getting more visibility into what's happening. Fixes #13. --- device-libusb.cpp | 7 +++---- device-libusb.h | 4 +++- host-raw-gadget.cpp | 10 ++++++++++ proxy.cpp | 34 ++++++++++++++++++++++++++++++---- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/device-libusb.cpp b/device-libusb.cpp index 683cc9a..8c868ed 100644 --- a/device-libusb.cpp +++ b/device-libusb.cpp @@ -245,7 +245,7 @@ int control_request(const usb_ctrlrequest *setup_packet, int *nbytes, } int send_data(uint8_t endpoint, uint8_t attributes, uint8_t *dataptr, - int length) { + int length, int timeout) { int transferred; int attempt = 0; int result = LIBUSB_SUCCESS; @@ -262,7 +262,7 @@ int send_data(uint8_t endpoint, uint8_t attributes, uint8_t *dataptr, break; case USB_ENDPOINT_XFER_BULK: do { - result = libusb_bulk_transfer(dev_handle, endpoint, dataptr, length, &transferred, 0); + result = libusb_bulk_transfer(dev_handle, endpoint, dataptr, length, &transferred, timeout); //TODO retry transfer if incomplete if (transferred != length) { fprintf(stderr, "Incomplete Bulk transfer on EP%02x for attempt %d. length(%d), transferred(%d)\n", @@ -284,7 +284,7 @@ int send_data(uint8_t endpoint, uint8_t attributes, uint8_t *dataptr, && attempt < MAX_ATTEMPTS); break; case USB_ENDPOINT_XFER_INT: - result = libusb_interrupt_transfer(dev_handle, endpoint, dataptr, length, &transferred, 0); + result = libusb_interrupt_transfer(dev_handle, endpoint, dataptr, length, &transferred, timeout); if (transferred != length) fprintf(stderr, "Incomplete Interrupt transfer on EP%02x\n", endpoint); @@ -302,7 +302,6 @@ int send_data(uint8_t endpoint, uint8_t attributes, uint8_t *dataptr, int receive_data(uint8_t endpoint, uint8_t attributes, uint16_t maxPacketSize, uint8_t **dataptr, int *length, int timeout) { int result = LIBUSB_SUCCESS; - timeout = 0; int attempt = 0; switch (attributes & USB_ENDPOINT_XFERTYPE_MASK) { diff --git a/device-libusb.h b/device-libusb.h index bf2ccd4..d91e76a 100644 --- a/device-libusb.h +++ b/device-libusb.h @@ -2,6 +2,8 @@ #include "misc.h" +#define USB_REQUEST_TIMEOUT 1000 + #define MAX_ATTEMPTS 5 extern libusb_device **devs; @@ -23,6 +25,6 @@ void set_interface_alt_setting(int interface, int altsetting); int control_request(const usb_ctrlrequest *setup_packet, int *nbytes, unsigned char **dataptr, int timeout); int send_data(uint8_t endpoint, uint8_t attributes, uint8_t *dataptr, - int length); + int length, int timeout); int receive_data(uint8_t endpoint, uint8_t attributes, uint16_t maxPacketSize, uint8_t **dataptr, int *length, int timeout); diff --git a/host-raw-gadget.cpp b/host-raw-gadget.cpp index 0f11ee2..14a9a7f 100644 --- a/host-raw-gadget.cpp +++ b/host-raw-gadget.cpp @@ -111,6 +111,11 @@ int usb_raw_ep_read(int fd, struct usb_raw_ep_io *io) { // Ignore failures caused by device reset. return rv; } + else if (errno == EINTR) { + // Ignore failures caused by sending a signal to the + // endpoint threads when shutting them down. + return rv; + } else if (errno == EBUSY) return rv; perror("ioctl(USB_RAW_IOCTL_EP_READ)"); @@ -130,6 +135,11 @@ int usb_raw_ep_write(int fd, struct usb_raw_ep_io *io) { // Ignore failures caused by device reset. return rv; } + else if (errno == EINTR) { + // Ignore failures caused by sending a signal to the + // endpoint threads when shutting them down. + return rv; + } else if (errno == EBUSY) return rv; perror("ioctl(USB_RAW_IOCTL_EP_WRITE)"); diff --git a/proxy.cpp b/proxy.cpp index 93aa956..86cc3ca 100644 --- a/proxy.cpp +++ b/proxy.cpp @@ -99,6 +99,8 @@ void printData(struct usb_raw_transfer_io io, __u8 bEndpointAddress, std::string printf("\n"); } +void noop_signal_handler(int) { } + void *ep_loop_write(void *arg) { struct thread_info thread_info = *((struct thread_info*) arg); int fd = thread_info.fd; @@ -112,6 +114,10 @@ void *ep_loop_write(void *arg) { printf("Start writing thread for EP%02x, thread id(%d)\n", ep.bEndpointAddress, gettid()); + // Set a no-op handler for SIGUSR1. Sending this signal to the thread + // will thus interrupt a blocking ioctl call without other side-effects. + signal(SIGUSR1, noop_signal_handler); + while (!please_stop_eps) { assert(ep_num != -1); if (data_queue->size() == 0) { @@ -134,6 +140,11 @@ void *ep_loop_write(void *arg) { ep.bEndpointAddress, transfer_type.c_str(), dir.c_str()); break; } + if (rv < 0 && errno == EINTR) { + printf("EP%x(%s_%s): interface likely changing, stopping thread\n", + ep.bEndpointAddress, transfer_type.c_str(), dir.c_str()); + break; + } else if (rv < 0) { perror("usb_raw_ep_write()"); exit(EXIT_FAILURE); @@ -147,7 +158,7 @@ void *ep_loop_write(void *arg) { int length = io.inner.length; unsigned char *data = new unsigned char[length]; memcpy(data, io.data, length); - int rv = send_data(ep.bEndpointAddress, ep.bmAttributes, data, length); + int rv = send_data(ep.bEndpointAddress, ep.bmAttributes, data, length, USB_REQUEST_TIMEOUT); if (rv == LIBUSB_ERROR_NO_DEVICE) { printf("EP%x(%s_%s): device likely reset, stopping thread\n", ep.bEndpointAddress, transfer_type.c_str(), dir.c_str()); @@ -177,6 +188,10 @@ void *ep_loop_read(void *arg) { printf("Start reading thread for EP%02x, thread id(%d)\n", ep.bEndpointAddress, gettid()); + // Set a no-op handler for SIGUSR1. Sending this signal to the thread + // will thus interrupt a blocking ioctl call without other side-effects. + signal(SIGUSR1, noop_signal_handler); + while (!please_stop_eps) { assert(ep_num != -1); struct usb_raw_transfer_io io; @@ -190,7 +205,8 @@ void *ep_loop_read(void *arg) { continue; } - int rv = receive_data(ep.bEndpointAddress, ep.bmAttributes, ep.wMaxPacketSize, &data, &nbytes, 0); + int rv = receive_data(ep.bEndpointAddress, ep.bmAttributes, usb_endpoint_maxp(&ep), + &data, &nbytes, USB_REQUEST_TIMEOUT); if (rv == LIBUSB_ERROR_NO_DEVICE) { printf("EP%x(%s_%s): device likely reset, stopping thread\n", ep.bEndpointAddress, transfer_type.c_str(), dir.c_str()); @@ -319,6 +335,16 @@ void terminate_eps(int fd, int config, int interface, int altsetting) { for (int i = 0; i < alt->interface.bNumEndpoints; i++) { struct raw_gadget_endpoint *ep = &alt->endpoints[i]; + // Endpoint threads might be blocked either on a Raw Gadget + // ioctl or on a libusb transfer handling. To interrupt the + // former, we send the SIGUSR1 signal to the threads. The + // threads have a no-op handler set for this signal, so the + // ioctl gets interrupted with no other side-effects. + // The libusb transfer handling does get interrupted directly + // and instead times out. + pthread_kill(ep->thread_read, SIGUSR1); + pthread_kill(ep->thread_write, SIGUSR1); + if (ep->thread_read && pthread_join(ep->thread_read, NULL)) { fprintf(stderr, "Error join thread_read\n"); } @@ -405,7 +431,7 @@ void ep0_loop(int fd) { int rv = -1; if (event.ctrl.bRequestType & USB_DIR_IN) { - result = control_request(&event.ctrl, &nbytes, &control_data, 1000); + result = control_request(&event.ctrl, &nbytes, &control_data, USB_REQUEST_TIMEOUT); if (result == 0) { memcpy(&io.data[0], control_data, nbytes); io.inner.length = nbytes; @@ -580,7 +606,7 @@ void ep0_loop(int fd) { if (verbose_level >= 2) printData(io, 0x00, "control", "out"); - result = control_request(&event.ctrl, &nbytes, &control_data, 1000); + result = control_request(&event.ctrl, &nbytes, &control_data, USB_REQUEST_TIMEOUT); if (result == 0) { printf("ep0: transferred %d bytes (out)\n", rv); }