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.
This commit is contained in:
Adrian Chadd
2020-11-08 21:38:39 -08:00
parent 52851eeb08
commit 0961a76f26

View File

@ -119,6 +119,7 @@ struct hackrf_device {
void* tx_ctx; void* tx_ctx;
volatile bool do_exit; volatile bool do_exit;
unsigned char buffer[TRANSFER_COUNT * TRANSFER_BUFFER_SIZE]; unsigned char buffer[TRANSFER_COUNT * TRANSFER_BUFFER_SIZE];
bool transfers_setup; /* true if the USB transfers have been setup */
}; };
typedef struct { typedef struct {
@ -157,6 +158,8 @@ static const uint16_t hackrf_one_usb_pid = 0x6089;
static const uint16_t rad1o_usb_pid = 0xcc15; static const uint16_t rad1o_usb_pid = 0xcc15;
static uint16_t open_devices = 0; static uint16_t open_devices = 0;
static int create_transfer_thread(hackrf_device* device);
static libusb_context* g_libusb_context = NULL; static libusb_context* g_libusb_context = NULL;
int last_libusb_error = LIBUSB_SUCCESS; int last_libusb_error = LIBUSB_SUCCESS;
@ -169,7 +172,7 @@ static int cancel_transfers(hackrf_device* device)
{ {
uint32_t transfer_index; uint32_t transfer_index;
if( device->transfers != NULL ) if( (device->transfers != NULL) && (device->transfers_setup == true) )
{ {
for(transfer_index=0; transfer_index<TRANSFER_COUNT; transfer_index++) for(transfer_index=0; transfer_index<TRANSFER_COUNT; transfer_index++)
{ {
@ -178,6 +181,7 @@ static int cancel_transfers(hackrf_device* device)
libusb_cancel_transfer(device->transfers[transfer_index]); libusb_cancel_transfer(device->transfers[transfer_index]);
} }
} }
device->transfers_setup = false;
return HACKRF_SUCCESS; return HACKRF_SUCCESS;
} else { } else {
return HACKRF_ERROR_OTHER; return HACKRF_ERROR_OTHER;
@ -269,6 +273,7 @@ static int prepare_transfers(
return HACKRF_ERROR_LIBUSB; return HACKRF_ERROR_LIBUSB;
} }
} }
device->transfers_setup = true;
return HACKRF_SUCCESS; return HACKRF_SUCCESS;
} else { } else {
// This shouldn't happen. // 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 = NULL;
lib_device = (hackrf_device*)malloc(sizeof(*lib_device)); lib_device = (hackrf_device*)calloc(1, sizeof(*lib_device));
if( lib_device == NULL ) if( lib_device == NULL )
{ {
libusb_release_interface(usb_device, 0); 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; 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; *device = lib_device;
open_devices++; open_devices++;
@ -1521,11 +1534,12 @@ static void LIBUSB_CALL hackrf_libusb_transfer_callback(struct libusb_transfer*
}else { }else {
request_exit(device); request_exit(device);
} }
} else if(usb_transfer->status == LIBUSB_TRANSFER_CANCELLED) {
/* Nothing; this will happen during shutdown */
} else { } else {
/* Other cases LIBUSB_TRANSFER_NO_DEVICE /* Other cases LIBUSB_TRANSFER_NO_DEVICE
LIBUSB_TRANSFER_ERROR, LIBUSB_TRANSFER_TIMED_OUT LIBUSB_TRANSFER_ERROR, LIBUSB_TRANSFER_TIMED_OUT
LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW ....
LIBUSB_TRANSFER_CANCELLED ...
*/ */
request_exit(device); /* Fatal error stop transfer */ request_exit(device); /* Fatal error stop transfer */
} }
@ -1535,11 +1549,25 @@ static int kill_transfer_thread(hackrf_device* device)
{ {
void* value; void* value;
int result; int result;
request_exit(device);
if( device->transfer_thread_started != false ) 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; value = NULL;
result = pthread_join(device->transfer_thread, &value); result = pthread_join(device->transfer_thread, &value);
if( result != 0 ) if( result != 0 )
@ -1548,16 +1576,43 @@ static int kill_transfer_thread(hackrf_device* device)
} }
device->transfer_thread_started = false; 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; return HACKRF_SUCCESS;
} }
static int create_transfer_thread(hackrf_device* device, static int create_transfer_thread(hackrf_device* device)
const uint8_t endpoint_address,
hackrf_sample_block_cb_fn callback)
{ {
int result; int result;
@ -1566,18 +1621,7 @@ static int create_transfer_thread(hackrf_device* device,
device->streaming = false; device->streaming = false;
device->do_exit = 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->streaming = true;
device->callback = callback;
result = pthread_create(&device->transfer_thread, 0, transfer_threadproc, device); result = pthread_create(&device->transfer_thread, 0, transfer_threadproc, device);
if( result == 0 ) 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 ) if( result == HACKRF_SUCCESS )
{ {
device->rx_ctx = rx_ctx; device->rx_ctx = rx_ctx;
result = create_transfer_thread(device, endpoint_address, callback); result = prepare_setup_transfers(device, endpoint_address, callback);
} }
return result; 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 ADDCALL hackrf_stop_rx(hackrf_device* device)
{ {
int result; int result;
result = kill_transfer_thread(device); result = cancel_transfers(device);
if (result != HACKRF_SUCCESS) if (result != HACKRF_SUCCESS)
{ {
return result; return result;
@ -1649,7 +1693,7 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal
if( result == HACKRF_SUCCESS ) if( result == HACKRF_SUCCESS )
{ {
device->tx_ctx = tx_ctx; device->tx_ctx = tx_ctx;
result = create_transfer_thread(device, endpoint_address, callback); result = prepare_setup_transfers(device, endpoint_address, callback);
} }
return result; 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 ADDCALL hackrf_stop_tx(hackrf_device* device)
{ {
int result; int result;
result = kill_transfer_thread(device); result = cancel_transfers(device);
if (result != HACKRF_SUCCESS) if (result != HACKRF_SUCCESS)
{ {
return result; return result;
@ -1668,13 +1712,15 @@ int ADDCALL hackrf_stop_tx(hackrf_device* device)
int ADDCALL hackrf_close(hackrf_device* device) int ADDCALL hackrf_close(hackrf_device* device)
{ {
int result1, result2; int result1, result2, result3;
result1 = HACKRF_SUCCESS; result1 = HACKRF_SUCCESS;
result2 = HACKRF_SUCCESS; result2 = HACKRF_SUCCESS;
result3 = HACKRF_SUCCESS;
if( device != NULL ) if( device != NULL )
{ {
result3 = kill_transfer_thread(device);
result1 = hackrf_stop_rx(device); result1 = hackrf_stop_rx(device);
result2 = hackrf_stop_tx(device); result2 = hackrf_stop_tx(device);
if( device->usb_device != NULL ) if( device->usb_device != NULL )
@ -1690,6 +1736,10 @@ int ADDCALL hackrf_close(hackrf_device* device)
} }
open_devices--; open_devices--;
if (result3 != HACKRF_SUCCESS)
{
return result3;
}
if (result2 != HACKRF_SUCCESS) if (result2 != HACKRF_SUCCESS)
{ {
return result2; return result2;
@ -2174,7 +2224,7 @@ int ADDCALL hackrf_start_rx_sweep(hackrf_device* device, hackrf_sample_block_cb_
if (HACKRF_SUCCESS == result) if (HACKRF_SUCCESS == result)
{ {
device->rx_ctx = rx_ctx; device->rx_ctx = rx_ctx;
result = create_transfer_thread(device, endpoint_address, callback); result = prepare_setup_transfers(device, endpoint_address, callback);
} }
return result; return result;
} }