Merge pull request #1180 from martinling/fix-transfer-locking

Improve transfer locking
This commit is contained in:
Michael Ossmann
2022-09-28 05:06:07 -04:00
committed by GitHub

View File

@ -144,7 +144,6 @@ struct hackrf_device {
pthread_mutex_t transfer_lock; /* must be held to cancel or restart transfers */ pthread_mutex_t transfer_lock; /* must be held to cancel or restart transfers */
volatile int active_transfers; /* number of active transfers */ volatile int active_transfers; /* number of active transfers */
pthread_cond_t all_finished_cv; /* signalled when all transfers have finished */ pthread_cond_t all_finished_cv; /* signalled when all transfers have finished */
pthread_mutex_t all_finished_lock; /* used to protect all_finished */
bool flush; bool flush;
struct libusb_transfer* flush_transfer; struct libusb_transfer* flush_transfer;
hackrf_flush_cb_fn flush_callback; hackrf_flush_cb_fn flush_callback;
@ -245,22 +244,14 @@ static int cancel_transfers(hackrf_device* device)
device->transfers_setup = false; device->transfers_setup = false;
device->flush = false; device->flush = 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);
// Now wait for the transfer thread to signal that all transfers // Now wait for the transfer thread to signal that all transfers
// have finished, either by completing or being fully cancelled. // have finished, either by completing or being fully cancelled.
pthread_mutex_lock(&device->all_finished_lock);
while (device->active_transfers > 0) { while (device->active_transfers > 0) {
pthread_cond_wait( pthread_cond_wait(
&device->all_finished_cv, &device->all_finished_cv,
&device->all_finished_lock); &device->transfer_lock);
} }
pthread_mutex_unlock(&device->all_finished_lock); pthread_mutex_unlock(&device->transfer_lock);
return HACKRF_SUCCESS; return HACKRF_SUCCESS;
} else { } else {
@ -382,7 +373,11 @@ static int prepare_transfers(
ready_transfers = TRANSFER_COUNT; ready_transfers = TRANSFER_COUNT;
} }
// Now everything is ready, go ahead and submit the ready transfers. // Now everything is ready, go ahead and submit the ready transfers. We must hold
// the transfer lock whilst doing this, so that completion callbacks cannot resubmit
// any transfers until all transfers have been initially submitted.
pthread_mutex_lock(&device->transfer_lock);
for (transfer_index = 0; transfer_index < ready_transfers; transfer_index++) { for (transfer_index = 0; transfer_index < ready_transfers; transfer_index++) {
struct libusb_transfer* transfer = device->transfers[transfer_index]; struct libusb_transfer* transfer = device->transfers[transfer_index];
transfer->endpoint = endpoint_address; transfer->endpoint = endpoint_address;
@ -397,27 +392,35 @@ static int prepare_transfers(
error = libusb_submit_transfer(transfer); error = libusb_submit_transfer(transfer);
if (error != 0) { if (error != 0) {
last_libusb_error = error; last_libusb_error = error;
return HACKRF_ERROR_LIBUSB; break;
} }
device->active_transfers++; device->active_transfers++;
} }
// We should only continue streaming if all transfers were made ready if (error == 0) {
// and submitted above. Otherwise, set streaming to false so that the // We should only continue streaming if all transfers were made ready
// libusb completion callback won't submit further transfers. // and submitted above. Otherwise, set streaming to false so that the
device->streaming = (ready_transfers == TRANSFER_COUNT); // libusb completion callback won't submit further transfers.
device->transfers_setup = true; device->streaming = (ready_transfers == TRANSFER_COUNT);
device->transfers_setup = true;
// If we're not continuing streaming, follow up with a flush if needed. // If we're not continuing streaming, follow up with a flush if needed.
if (!device->streaming && device->flush) { if (!device->streaming && device->flush) {
error = libusb_submit_transfer(device->flush_transfer); error = libusb_submit_transfer(device->flush_transfer);
if (error != 0) { if (error != 0) {
last_libusb_error = error; last_libusb_error = error;
return HACKRF_ERROR_LIBUSB; }
} }
} }
return HACKRF_SUCCESS; // Now we can release the transfer lock.
pthread_mutex_unlock(&device->transfer_lock);
if (error == 0) {
return HACKRF_SUCCESS;
} else {
return HACKRF_ERROR_LIBUSB;
}
} }
static int detach_kernel_drivers(libusb_device_handle* usb_device_handle) static int detach_kernel_drivers(libusb_device_handle* usb_device_handle)
@ -743,14 +746,6 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d
return HACKRF_ERROR_THREAD; return HACKRF_ERROR_THREAD;
} }
result = pthread_mutex_init(&lib_device->all_finished_lock, NULL);
if (result != 0) {
free(lib_device);
libusb_release_interface(usb_device, 0);
libusb_close(usb_device);
return HACKRF_ERROR_THREAD;
}
result = pthread_cond_init(&lib_device->all_finished_cv, NULL); result = pthread_cond_init(&lib_device->all_finished_cv, NULL);
if (result != 0) { if (result != 0) {
free(lib_device); free(lib_device);
@ -1775,11 +1770,11 @@ static void LIBUSB_CALL hackrf_libusb_flush_callback(struct libusb_transfer* usb
// All transfers have now ended, so proceed with signalling completion. // All transfers have now ended, so proceed with signalling completion.
hackrf_device* device = (hackrf_device*) usb_transfer->user_data; hackrf_device* device = (hackrf_device*) usb_transfer->user_data;
pthread_mutex_lock(&device->all_finished_lock); pthread_mutex_lock(&device->transfer_lock);
device->flush = false; device->flush = false;
device->active_transfers = 0; device->active_transfers = 0;
pthread_cond_broadcast(&device->all_finished_cv); pthread_cond_broadcast(&device->all_finished_cv);
pthread_mutex_unlock(&device->all_finished_lock); pthread_mutex_unlock(&device->transfer_lock);
if (device->flush_callback) if (device->flush_callback)
device->flush_callback(device->flush_ctx, success); device->flush_callback(device->flush_ctx, success);
@ -1834,28 +1829,26 @@ hackrf_libusb_transfer_callback(struct libusb_transfer* usb_transfer)
device->streaming = false; device->streaming = false;
device->flush = false; device->flush = false;
} }
// If a data transfer was resubmitted successfully, we're done.
if (!resubmit || result != LIBUSB_SUCCESS) {
// No further calls should be made to the TX callback.
device->streaming = false;
// If this is the last transfer, signal that all are now finished.
if (device->active_transfers == 1) {
if (!device->flush) {
device->active_transfers = 0;
pthread_cond_broadcast(&device->all_finished_cv);
}
} else {
device->active_transfers--;
}
}
// Now we can release the lock. Our transfer was either // Now we can release the lock. Our transfer was either
// cancelled or restarted, not both. // cancelled or restarted, not both.
pthread_mutex_unlock(&device->transfer_lock); pthread_mutex_unlock(&device->transfer_lock);
// If a data transfer was resubmitted successfully, we're done.
if (resubmit && result == LIBUSB_SUCCESS)
return;
// Otherwise, no further calls should be made to the TX callback.
device->streaming = false;
// If this is the last transfer, signal that all are now finished.
pthread_mutex_lock(&device->all_finished_lock);
if (device->active_transfers == 1) {
if (!device->flush) {
device->active_transfers = 0;
pthread_cond_broadcast(&device->all_finished_cv);
}
} else {
device->active_transfers--;
}
pthread_mutex_unlock(&device->all_finished_lock);
} }
static int kill_transfer_thread(hackrf_device* device) static int kill_transfer_thread(hackrf_device* device)
@ -2109,7 +2102,6 @@ int ADDCALL hackrf_close(hackrf_device* device)
pthread_mutex_destroy(&device->transfer_lock); pthread_mutex_destroy(&device->transfer_lock);
pthread_cond_destroy(&device->all_finished_cv); pthread_cond_destroy(&device->all_finished_cv);
pthread_mutex_destroy(&device->all_finished_lock);
free(device); free(device);
} }