core: Introduce atomic type and operations An atomic variable is useful for reference counting and is much less overhead than accessing such a variable while holding a lock. To that end, replace the libusb_device 'refcnt' variable with an atomic and use the atomic operations to manipulate it. This removes the need for the mutex in the libusb_device. Also convert the 'attached' variable to an atomic as well. This variable was previously accessed both while holding the libusb_device mutex and not. Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>
diff --git a/libusb/core.c b/libusb/core.c index 07d459c..051b4f7 100644 --- a/libusb/core.c +++ b/libusb/core.c
@@ -704,10 +704,9 @@ if (!dev) return NULL; - usbi_mutex_init(&dev->lock); + usbi_atomic_store(&dev->refcnt, 1); dev->ctx = ctx; - dev->refcnt = 1; dev->session_data = session_id; dev->speed = LIBUSB_SPEED_UNKNOWN; @@ -722,7 +721,7 @@ { struct libusb_context *ctx = DEVICE_CTX(dev); - dev->attached = 1; + usbi_atomic_store(&dev->attached, 1); usbi_mutex_lock(&dev->ctx->usb_devs_lock); list_add(&dev->list, &dev->ctx->usb_devs); @@ -740,9 +739,7 @@ { struct libusb_context *ctx = DEVICE_CTX(dev); - usbi_mutex_lock(&dev->lock); - dev->attached = 0; - usbi_mutex_unlock(&dev->lock); + usbi_atomic_store(&dev->attached, 0); usbi_mutex_lock(&ctx->usb_devs_lock); list_del(&dev->list); @@ -1173,9 +1170,11 @@ DEFAULT_VISIBILITY libusb_device * LIBUSB_CALL libusb_ref_device(libusb_device *dev) { - usbi_mutex_lock(&dev->lock); - dev->refcnt++; - usbi_mutex_unlock(&dev->lock); + long refcnt; + + refcnt = usbi_atomic_inc(&dev->refcnt); + assert(refcnt >= 2); + return dev; } @@ -1186,14 +1185,13 @@ */ void API_EXPORTED libusb_unref_device(libusb_device *dev) { - int refcnt; + long refcnt; if (!dev) return; - usbi_mutex_lock(&dev->lock); - refcnt = --dev->refcnt; - usbi_mutex_unlock(&dev->lock); + refcnt = usbi_atomic_dec(&dev->refcnt); + assert(refcnt >= 0); if (refcnt == 0) { usbi_dbg("destroy device %d.%d", dev->bus_number, dev->device_address); @@ -1208,7 +1206,6 @@ usbi_disconnect_device(dev); } - usbi_mutex_destroy(&dev->lock); free(dev); } } @@ -1306,11 +1303,11 @@ struct libusb_device_handle *_dev_handle; size_t priv_size = usbi_backend.device_handle_priv_size; int r; + usbi_dbg("open %d.%d", dev->bus_number, dev->device_address); - if (!dev->attached) { + if (!usbi_atomic_load(&dev->attached)) return LIBUSB_ERROR_NO_DEVICE; - } _dev_handle = calloc(1, PTR_ALIGN(sizeof(*_dev_handle)) + priv_size); if (!_dev_handle) @@ -1673,7 +1670,7 @@ if (interface_number < 0 || interface_number >= USB_MAXINTERFACES) return LIBUSB_ERROR_INVALID_PARAM; - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; usbi_mutex_lock(&dev_handle->lock); @@ -1763,12 +1760,12 @@ if (alternate_setting < 0 || alternate_setting > (int)UINT8_MAX) return LIBUSB_ERROR_INVALID_PARAM; - usbi_mutex_lock(&dev_handle->lock); - if (!dev_handle->dev->attached) { + if (!usbi_atomic_load(&dev_handle->dev->attached)) { usbi_mutex_unlock(&dev_handle->lock); return LIBUSB_ERROR_NO_DEVICE; } + usbi_mutex_lock(&dev_handle->lock); if (!(dev_handle->claimed_interfaces & (1U << interface_number))) { usbi_mutex_unlock(&dev_handle->lock); return LIBUSB_ERROR_NOT_FOUND; @@ -1799,7 +1796,7 @@ unsigned char endpoint) { usbi_dbg("endpoint %x", endpoint); - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; return usbi_backend.clear_halt(dev_handle, endpoint); @@ -1827,7 +1824,7 @@ int API_EXPORTED libusb_reset_device(libusb_device_handle *dev_handle) { usbi_dbg(" "); - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; if (usbi_backend.reset_device) @@ -1865,7 +1862,7 @@ if (!num_streams || !endpoints || num_endpoints <= 0) return LIBUSB_ERROR_INVALID_PARAM; - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; if (usbi_backend.alloc_streams) @@ -1895,7 +1892,7 @@ if (!endpoints || num_endpoints <= 0) return LIBUSB_ERROR_INVALID_PARAM; - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; if (usbi_backend.free_streams) @@ -1933,7 +1930,7 @@ unsigned char * LIBUSB_CALL libusb_dev_mem_alloc(libusb_device_handle *dev_handle, size_t length) { - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return NULL; if (usbi_backend.dev_mem_alloc) @@ -1984,7 +1981,7 @@ if (interface_number < 0 || interface_number >= USB_MAXINTERFACES) return LIBUSB_ERROR_INVALID_PARAM; - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; if (usbi_backend.kernel_driver_active) @@ -2022,7 +2019,7 @@ if (interface_number < 0 || interface_number >= USB_MAXINTERFACES) return LIBUSB_ERROR_INVALID_PARAM; - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; if (usbi_backend.detach_kernel_driver) @@ -2059,7 +2056,7 @@ if (interface_number < 0 || interface_number >= USB_MAXINTERFACES) return LIBUSB_ERROR_INVALID_PARAM; - if (!dev_handle->dev->attached) + if (!usbi_atomic_load(&dev_handle->dev->attached)) return LIBUSB_ERROR_NO_DEVICE; if (usbi_backend.attach_kernel_driver) @@ -2407,6 +2404,9 @@ list_del(&ctx->list); usbi_mutex_static_unlock(&active_contexts_lock); + /* Don't bother with locking after this point because unless there is + * an application bug, nobody will be accessing these. */ + if (libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) { usbi_hotplug_deregister(ctx, 1); @@ -2422,18 +2422,28 @@ if (list_empty(&ctx->open_devs)) libusb_handle_events_timeout(ctx, &tv); - usbi_mutex_lock(&ctx->usb_devs_lock); for_each_device_safe(ctx, dev, next) { + if (usbi_atomic_load(&dev->refcnt) > 1) + usbi_warn(ctx, "device %d.%d still referenced", + dev->bus_number, dev->device_address); list_del(&dev->list); libusb_unref_device(dev); } - usbi_mutex_unlock(&ctx->usb_devs_lock); + } else { + /* + * Backends without hotplug store enumerated devices on the + * usb_devs list when libusb_get_device_list() is called. + * These devices are removed from the list when the last + * reference is dropped, typically when the device list is + * freed. Any device still on the list has a reference held + * by the app, which is a bug. + */ + for_each_device(ctx, dev) { + usbi_warn(ctx, "device %d.%d still referenced", + dev->bus_number, dev->device_address); + } } - /* a few sanity checks. don't bother with locking because unless - * there is an application bug, nobody will be accessing these. */ - if (!list_empty(&ctx->usb_devs)) - usbi_warn(ctx, "some libusb_devices were leaked"); if (!list_empty(&ctx->open_devs)) usbi_warn(ctx, "application left some devices open");
diff --git a/libusb/libusbi.h b/libusb/libusbi.h index 491114b..aea1bac 100644 --- a/libusb/libusbi.h +++ b/libusb/libusbi.h
@@ -83,6 +83,33 @@ #define PTR_ALIGN(v) \ (((v) + (sizeof(void *) - 1)) & ~(sizeof(void *) - 1)) +/* Atomic operations + * + * Useful for reference counting or when accessing a value without a lock + * + * The following atomic operations are defined: + * usbi_atomic_load() - Atomically read a variable's value + * usbi_atomic_store() - Atomically write a new value value to a variable + * usbi_atomic_inc() - Atomically increment a variable's value and return the new value + * usbi_atomic_dec() - Atomically decrement a variable's value and return the new value + * + * All of these operations are ordered with each other, thus the effects of + * any one operation is guaranteed to be seen by any other operation. + */ +#ifdef _MSC_VER +typedef volatile LONG usbi_atomic_t; +#define usbi_atomic_load(a) (*(a)) +#define usbi_atomic_store(a, v) (*(a)) = (v) +#define usbi_atomic_inc(a) InterlockedIncrement((a)) +#define usbi_atomic_dec(a) InterlockedDecrement((a)) +#else +typedef long usbi_atomic_t; +#define usbi_atomic_load(a) __atomic_load_n((a), __ATOMIC_SEQ_CST) +#define usbi_atomic_store(a, v) __atomic_store_n((a), (v), __ATOMIC_SEQ_CST) +#define usbi_atomic_inc(a) __atomic_add_fetch((a), 1, __ATOMIC_SEQ_CST) +#define usbi_atomic_dec(a) __atomic_sub_fetch((a), 1, __ATOMIC_SEQ_CST) +#endif + /* Internal abstractions for event handling and thread synchronization */ #if defined(PLATFORM_POSIX) #include "os/events_posix.h" @@ -450,10 +477,7 @@ } struct libusb_device { - /* lock protects refcnt, everything else is finalized at initialization - * time */ - usbi_mutex_t lock; - int refcnt; + usbi_atomic_t refcnt; struct libusb_context *ctx; struct libusb_device *parent_dev; @@ -467,7 +491,7 @@ unsigned long session_data; struct libusb_device_descriptor device_descriptor; - int attached; + usbi_atomic_t attached; }; struct libusb_device_handle {
diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c index 4d2dc8d..7175b35 100644 --- a/libusb/os/linux_usbfs.c +++ b/libusb/os/linux_usbfs.c
@@ -1365,7 +1365,7 @@ goto out; /* Consider the device as connected, but do not add it to the managed * device list. */ - dev->attached = 1; + usbi_atomic_store(&dev->attached, 1); handle->dev = dev; r = initialize_handle(handle, fd); @@ -1387,7 +1387,7 @@ /* device will still be marked as attached if hotplug monitor thread * hasn't processed remove event yet */ usbi_mutex_static_lock(&linux_hotplug_lock); - if (handle->dev->attached) { + if (usbi_atomic_load(&handle->dev->attached)) { usbi_dbg("open failed with no device, but device still attached"); linux_device_disconnected(handle->dev->bus_number, handle->dev->device_address); @@ -2711,7 +2711,7 @@ /* device will still be marked as attached if hotplug monitor thread * hasn't processed remove event yet */ usbi_mutex_static_lock(&linux_hotplug_lock); - if (handle->dev->attached) + if (usbi_atomic_load(&handle->dev->attached)) linux_device_disconnected(handle->dev->bus_number, handle->dev->device_address); usbi_mutex_static_unlock(&linux_hotplug_lock);
diff --git a/libusb/version_nano.h b/libusb/version_nano.h index 578b097..6926971 100644 --- a/libusb/version_nano.h +++ b/libusb/version_nano.h
@@ -1 +1 @@ -#define LIBUSB_NANO 11586 +#define LIBUSB_NANO 11587