From 0961a76f26bb670f808b77172fe72f70bd5ccf52 Mon Sep 17 00:00:00 2001 From: Adrian Chadd Date: Sun, 8 Nov 2020 21:38:39 -0800 Subject: [PATCH] Fix libusb usage for at least freebsd around the worker thread and transfer cancellation On at least freebsd-13 trying to cancel a transfer whilst the libusb thread is not running results in the transfers not completing cancellation. The next time they're attempted to be re-added the libusb code thinks they're still active, and returns BUSY on the buffers. This causes gqrx to error out when one makes DSP changes or stops/starts it. You have to restart gqrx to fix it. After digging into it a bit, the libusb code expects that you're actively running the main loop in order to have some deferred actions run in the context of said main loop thread. This includes processing cancelled transfers - the callbacks have to be run (if they exist) before the buffers are properly cancelled and have their tracking metadata (a couple of private pointers and state) removed from inside of libusb. This patch does the following: * separate out adding and cancelling transfers from the libusb worker thread create/destroy path * create the libusb worker thread when opening the device * destroy the libusb worker thread when closing the device * only add and cancel transfers when starting and stopping tx/rx * handle cancelled transfers gracefully in the USB callback Whilst here, also make the libusb device memory zeroed by using calloc instead of malloc. This fixes all of the weird libusb related buffer management problems on FreeBSD. --- host/libhackrf/src/hackrf.c | 108 ++++++++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/host/libhackrf/src/hackrf.c b/host/libhackrf/src/hackrf.c index bc1d5fe8..adeaa059 100644 --- a/host/libhackrf/src/hackrf.c +++ b/host/libhackrf/src/hackrf.c @@ -119,6 +119,7 @@ struct hackrf_device { void* tx_ctx; volatile bool do_exit; unsigned char buffer[TRANSFER_COUNT * TRANSFER_BUFFER_SIZE]; + bool transfers_setup; /* true if the USB transfers have been setup */ }; typedef struct { @@ -157,6 +158,8 @@ static const uint16_t hackrf_one_usb_pid = 0x6089; static const uint16_t rad1o_usb_pid = 0xcc15; static uint16_t open_devices = 0; +static int create_transfer_thread(hackrf_device* device); + static libusb_context* g_libusb_context = NULL; int last_libusb_error = LIBUSB_SUCCESS; @@ -169,7 +172,7 @@ static int cancel_transfers(hackrf_device* device) { uint32_t transfer_index; - if( device->transfers != NULL ) + if( (device->transfers != NULL) && (device->transfers_setup == true) ) { for(transfer_index=0; transfer_indextransfers[transfer_index]); } } + device->transfers_setup = false; return HACKRF_SUCCESS; } else { return HACKRF_ERROR_OTHER; @@ -269,6 +273,7 @@ static int prepare_transfers( return HACKRF_ERROR_LIBUSB; } } + device->transfers_setup = true; return HACKRF_SUCCESS; } else { // This shouldn't happen. @@ -560,7 +565,7 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d } lib_device = NULL; - lib_device = (hackrf_device*)malloc(sizeof(*lib_device)); + lib_device = (hackrf_device*)calloc(1, sizeof(*lib_device)); if( lib_device == NULL ) { libusb_release_interface(usb_device, 0); @@ -584,6 +589,14 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d return HACKRF_ERROR_NO_MEM; } + result = create_transfer_thread(lib_device); + if (result != 0) { + free(lib_device); + libusb_release_interface(usb_device, 0); + libusb_close(usb_device); + return result; + } + *device = lib_device; open_devices++; @@ -1521,11 +1534,12 @@ static void LIBUSB_CALL hackrf_libusb_transfer_callback(struct libusb_transfer* }else { request_exit(device); } + } else if(usb_transfer->status == LIBUSB_TRANSFER_CANCELLED) { + /* Nothing; this will happen during shutdown */ } else { /* Other cases LIBUSB_TRANSFER_NO_DEVICE LIBUSB_TRANSFER_ERROR, LIBUSB_TRANSFER_TIMED_OUT - LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW - LIBUSB_TRANSFER_CANCELLED ... + LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW .... */ request_exit(device); /* Fatal error stop transfer */ } @@ -1535,11 +1549,25 @@ static int kill_transfer_thread(hackrf_device* device) { void* value; int result; - - request_exit(device); if( device->transfer_thread_started != false ) { + /* + * Schedule cancelling transfers before halting the + * libusb thread. This should result in the transfers + * being properly marked as cancelled. + * + * Ideally this would wait for the cancellations to + * complete with the callback but for now that + * isn't super easy to do. + */ + cancel_transfers(device); + + /* + * Now call request_exit() to halt the main loop. + */ + request_exit(device); + value = NULL; result = pthread_join(device->transfer_thread, &value); if( result != 0 ) @@ -1548,16 +1576,43 @@ static int kill_transfer_thread(hackrf_device* device) } device->transfer_thread_started = false; - /* Cancel all transfers */ - cancel_transfers(device); + } + + /* + * Reset do_exit; we're now done here and the thread was + * already dead or is now dead. + */ + device->do_exit = false; + + return HACKRF_SUCCESS; +} + +static int prepare_setup_transfers(hackrf_device* device, + const uint8_t endpoint_address, + hackrf_sample_block_cb_fn callback) +{ + int result; + + if( device->transfers_setup == true ) + { + return HACKRF_ERROR_BUSY; + } + + device->callback = callback; + result = prepare_transfers( + device, endpoint_address, + hackrf_libusb_transfer_callback + ); + + if( result != HACKRF_SUCCESS ) + { + return result; } return HACKRF_SUCCESS; } -static int create_transfer_thread(hackrf_device* device, - const uint8_t endpoint_address, - hackrf_sample_block_cb_fn callback) +static int create_transfer_thread(hackrf_device* device) { int result; @@ -1566,18 +1621,7 @@ static int create_transfer_thread(hackrf_device* device, device->streaming = false; device->do_exit = false; - result = prepare_transfers( - device, endpoint_address, - hackrf_libusb_transfer_callback - ); - - if( result != HACKRF_SUCCESS ) - { - return result; - } - device->streaming = true; - device->callback = callback; result = pthread_create(&device->transfer_thread, 0, transfer_threadproc, device); if( result == 0 ) { @@ -1625,7 +1669,7 @@ int ADDCALL hackrf_start_rx(hackrf_device* device, hackrf_sample_block_cb_fn cal if( result == HACKRF_SUCCESS ) { device->rx_ctx = rx_ctx; - result = create_transfer_thread(device, endpoint_address, callback); + result = prepare_setup_transfers(device, endpoint_address, callback); } return result; } @@ -1633,7 +1677,7 @@ int ADDCALL hackrf_start_rx(hackrf_device* device, hackrf_sample_block_cb_fn cal int ADDCALL hackrf_stop_rx(hackrf_device* device) { int result; - result = kill_transfer_thread(device); + result = cancel_transfers(device); if (result != HACKRF_SUCCESS) { return result; @@ -1649,7 +1693,7 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal if( result == HACKRF_SUCCESS ) { device->tx_ctx = tx_ctx; - result = create_transfer_thread(device, endpoint_address, callback); + result = prepare_setup_transfers(device, endpoint_address, callback); } return result; } @@ -1657,7 +1701,7 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal int ADDCALL hackrf_stop_tx(hackrf_device* device) { int result; - result = kill_transfer_thread(device); + result = cancel_transfers(device); if (result != HACKRF_SUCCESS) { return result; @@ -1668,13 +1712,15 @@ int ADDCALL hackrf_stop_tx(hackrf_device* device) int ADDCALL hackrf_close(hackrf_device* device) { - int result1, result2; + int result1, result2, result3; result1 = HACKRF_SUCCESS; result2 = HACKRF_SUCCESS; - + result3 = HACKRF_SUCCESS; + if( device != NULL ) { + result3 = kill_transfer_thread(device); result1 = hackrf_stop_rx(device); result2 = hackrf_stop_tx(device); if( device->usb_device != NULL ) @@ -1690,6 +1736,10 @@ int ADDCALL hackrf_close(hackrf_device* device) } open_devices--; + if (result3 != HACKRF_SUCCESS) + { + return result3; + } if (result2 != HACKRF_SUCCESS) { return result2; @@ -2174,7 +2224,7 @@ int ADDCALL hackrf_start_rx_sweep(hackrf_device* device, hackrf_sample_block_cb_ if (HACKRF_SUCCESS == result) { device->rx_ctx = rx_ctx; - result = create_transfer_thread(device, endpoint_address, callback); + result = prepare_setup_transfers(device, endpoint_address, callback); } return result; }