Skip to content

Commit

Permalink
Use a lock to prevent transfers being restarted during cancellation.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
martinling committed Dec 31, 2021
1 parent bf84a3a commit b346790
Showing 1 changed file with 41 additions and 1 deletion.
42 changes: 41 additions & 1 deletion host/libhackrf/src/hackrf.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ struct hackrf_device {
volatile bool do_exit;
unsigned char buffer[TRANSFER_COUNT * TRANSFER_BUFFER_SIZE];
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 {
Expand Down Expand Up @@ -207,14 +208,28 @@ static int cancel_transfers(hackrf_device* device)

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++)
{
if( device->transfers[transfer_index] != NULL )
{
libusb_cancel_transfer(device->transfers[transfer_index]);
}
}

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;
} else {
return HACKRF_ERROR_OTHER;
Expand Down Expand Up @@ -614,6 +629,15 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d
lib_device->streaming = 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);
if( result != 0 )
{
Expand Down Expand Up @@ -1559,7 +1583,21 @@ static void LIBUSB_CALL hackrf_libusb_transfer_callback(struct libusb_transfer*

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);
}else {
Expand Down Expand Up @@ -1821,6 +1859,8 @@ int ADDCALL hackrf_close(hackrf_device* device)

free_transfers(device);

pthread_mutex_destroy(&device->transfer_lock);

free(device);
}
open_devices--;
Expand Down

0 comments on commit b346790

Please sign in to comment.