Use a lock to prevent transfers being restarted during cancellation.

Fixes bug #916.

Previously, there was a race which could lead to a transfer being left
active after cancel_transfers() completed. This would then cause the
next prepare_transfers() call to fail, because libusb_submit_transfer()
would return an error due to the transfer already being in use.

The sequence of events that could cause this was:

1. Main thread calls hackrf_stop_rx(), which calls cancel_transfers(),
   which iterates through the 4 transfers in use and cancels them one
   by one with libusb_cancel_transfer().

2. During this time, a transfer is completed. The transfer thread calls
   hackrf_libusb_transfer_callback(), which handles the data and then
   calls libusb_submit_transfer() to resubmit that transfer.

3. Now, cancel_transfers() and hackrf_stop_rx() are completed but one
   transfer is still active.

4. The next hackrf_start_rx() call fails, because prepare_transfers()
   tries to submit a transfer which is already in use.

To fix this, we add a lock which must be held to either cancel transfers
or restart them. This ensures that only one of these actions can happen
for a given transfer; it's no longer possible for a transfer to be
cancelled and then immediately restarted.
This commit is contained in:
Martin Ling
2021-12-31 20:10:41 +00:00
parent 8660e44575
commit 837b5ee9c8

View File

@ -126,6 +126,7 @@ struct hackrf_device {
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 */ bool transfers_setup; /* true if the USB transfers have been setup */
pthread_mutex_t transfer_lock; /* must be held to cancel or restart transfers */
}; };
typedef struct { typedef struct {
@ -207,6 +208,11 @@ static int cancel_transfers(hackrf_device* device)
if(transfers_check_setup(device) == true) if(transfers_check_setup(device) == true)
{ {
// Take lock while cancelling transfers. This blocks the
// transfer completion callback from restarting a transfer
// while we're in the middle of trying to cancel them all.
pthread_mutex_lock(&device->transfer_lock);
for(transfer_index=0; transfer_index<TRANSFER_COUNT; transfer_index++) for(transfer_index=0; transfer_index<TRANSFER_COUNT; transfer_index++)
{ {
if( device->transfers[transfer_index] != NULL ) if( device->transfers[transfer_index] != NULL )
@ -214,7 +220,16 @@ 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; device->transfers_setup = false;
// Now release the lock. It's possible that some transfers were
// already complete when we called libusb_cancel_transfer() on
// them, and they may still get a callback. But the callback
// won't restart a transfer now that the transfers_setup flag
// is set to false.
pthread_mutex_unlock(&device->transfer_lock);
return HACKRF_SUCCESS; return HACKRF_SUCCESS;
} else { } else {
return HACKRF_ERROR_OTHER; return HACKRF_ERROR_OTHER;
@ -614,6 +629,15 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d
lib_device->streaming = false; lib_device->streaming = false;
lib_device->do_exit = false; lib_device->do_exit = false;
result = pthread_mutex_init(&lib_device->transfer_lock, NULL);
if( result != 0 )
{
free(lib_device);
libusb_release_interface(usb_device, 0);
libusb_close(usb_device);
return HACKRF_ERROR_THREAD;
}
result = allocate_transfers(lib_device); result = allocate_transfers(lib_device);
if( result != 0 ) if( result != 0 )
{ {
@ -1559,7 +1583,21 @@ static void LIBUSB_CALL hackrf_libusb_transfer_callback(struct libusb_transfer*
if( device->callback(&transfer) == 0 ) if( device->callback(&transfer) == 0 )
{ {
if( libusb_submit_transfer(usb_transfer) < 0) // Take lock to make sure that we don't restart a
// transfer whilst cancel_transfers() is in the middle
// of stopping them.
pthread_mutex_lock(&device->transfer_lock);
int result = 0;
if( device->transfers_setup ) {
result = libusb_submit_transfer(usb_transfer);
}
// Now we can release the lock. Our transfer was either
// cancelled or restarted, not both.
pthread_mutex_unlock(&device->transfer_lock);
if( result < 0)
{ {
request_exit(device); request_exit(device);
}else { }else {
@ -1821,6 +1859,8 @@ int ADDCALL hackrf_close(hackrf_device* device)
free_transfers(device); free_transfers(device);
pthread_mutex_destroy(&device->transfer_lock);
free(device); free(device);
} }
open_devices--; open_devices--;