Skip to content

Commit

Permalink
core: Introduce atomic type and operations
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@dickens
dickens committed Dec 16, 2020
1 parent f6d2cb5 commit 1a08aa8
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 42 deletions.
76 changes: 43 additions & 33 deletions libusb/core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -1173,9 +1170,11 @@ int API_EXPORTED libusb_get_max_iso_packet_size(libusb_device *dev,
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;
}

Expand All @@ -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);
Expand All @@ -1208,7 +1206,6 @@ void API_EXPORTED libusb_unref_device(libusb_device *dev)
usbi_disconnect_device(dev);
}

usbi_mutex_destroy(&dev->lock);
free(dev);
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);

Expand All @@ -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");

Expand Down
34 changes: 29 additions & 5 deletions libusb/libusbi.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions libusb/os/linux_usbfs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion libusb/version_nano.h
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
#define LIBUSB_NANO 11586
#define LIBUSB_NANO 11587

1 comment on commit 1a08aa8

@hjelmn
Copy link
Member

@hjelmn hjelmn commented on 1a08aa8 Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we should not be using the __atomics. They are not part of any standard. Instead we should be (thought we already were) requiring c11 for atomics.

Please sign in to comment.