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>

libusb/core.c

@@ -704,10 +704,9 @@ struct libusb_device *usbi_alloc_device(struct libusb_context *ctx,
 	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 @@ void usbi_connect_device(struct libusb_device *dev)
 {
 	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 @@ void usbi_disconnect_device(struct libusb_device *dev)
 {
 	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 @@ out:
 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 @@ libusb_device * LIBUSB_CALL libusb_ref_device(libusb_device *dev)
  */
 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 @@ void API_EXPORTED libusb_unref_device(libusb_device *dev)
 			usbi_disconnect_device(dev);
 		}
 
-		usbi_mutex_destroy(&dev->lock);
 		free(dev);
 	}
 }
@@ -1306,11 +1303,11 @@ int API_EXPORTED libusb_open(libusb_device *dev,
 	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 @@ int API_EXPORTED libusb_claim_interface(libusb_device_handle *dev_handle,
 	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 @@ int API_EXPORTED libusb_set_interface_alt_setting(libusb_device_handle *dev_hand
 	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 @@ int API_EXPORTED libusb_clear_halt(libusb_device_handle *dev_handle,
 	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_clear_halt(libusb_device_handle *dev_handle,
 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 @@ int API_EXPORTED libusb_alloc_streams(libusb_device_handle *dev_handle,
 	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 @@ int API_EXPORTED libusb_free_streams(libusb_device_handle *dev_handle,
 	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 @@ DEFAULT_VISIBILITY
 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 @@ int API_EXPORTED libusb_kernel_driver_active(libusb_device_handle *dev_handle,
 	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 @@ int API_EXPORTED libusb_detach_kernel_driver(libusb_device_handle *dev_handle,
 	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 @@ int API_EXPORTED libusb_attach_kernel_driver(libusb_device_handle *dev_handle,
 	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 @@ void API_EXPORTED libusb_exit(libusb_context *ctx)
 	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 @@ void API_EXPORTED libusb_exit(libusb_context *ctx)
 		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");
 

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 @@ static inline void usbi_end_event_handling(struct libusb_context *ctx)
 }
 
 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 @@ struct libusb_device {
 	unsigned long session_data;
 
 	struct libusb_device_descriptor device_descriptor;
-	int attached;
+	usbi_atomic_t attached;
 };
 
 struct libusb_device_handle {

libusb/os/linux_usbfs.c

@@ -1365,7 +1365,7 @@ static int op_wrap_sys_device(struct libusb_context *ctx,
 		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 @@ static int op_open(struct libusb_device_handle *handle)
 			/* 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 @@ static int op_handle_events(struct libusb_context *ctx,
 			/* 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);

libusb/version_nano.h

@@ -1 +1 @@
-#define LIBUSB_NANO 11586
+#define LIBUSB_NANO 11587