Page MenuHomeFreeBSD

Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Needs ReviewPublic

Authored by unitrunker_unitrunker.net on Sun, Dec 14, 9:25 AM.
Tags
Referenced Files
Unknown Object (File)
Fri, Dec 26, 12:49 AM
Unknown Object (File)
Thu, Dec 25, 10:13 PM
Unknown Object (File)
Thu, Dec 25, 9:33 PM
Unknown Object (File)
Thu, Dec 25, 8:00 PM
Unknown Object (File)
Thu, Dec 25, 2:47 AM
Unknown Object (File)
Wed, Dec 24, 10:20 AM
Unknown Object (File)
Wed, Dec 24, 10:11 AM
Unknown Object (File)
Wed, Dec 24, 9:42 AM
Subscribers

Details

Summary

To more closely match behavior of libusb_open on other platforms, place an advisory lock on the device file descriptor.

Currently, multiple processes can open the same USB device. On Linux and Windows, USB device enumeration omits devices currently in use. A number of ports assume this behavior.

This proposed change will cause libusb_open to report an error (LIBUSB_ERROR_BUSY) if it finds an exclusive advisory lock on the device. Likewise libusb20_dev_open will also report an error (LIBUSB20_ERROR_BUSY) if if finds an exclusive advisory lock on the device.

This includes a further change to libusb_open_with_vid_pid to skip over failed attempts to the first successful call to libusb_open.

In an exteme case, current unexpected behavior in the FreeBSD implementation causes two ports to display the same device 32 times (see ports airspy and hydrasdr).

As an example, a FreeBSD host has two USB devices attached that share the same VID/PID but different serial numbers. The intent is ...

a. a first call to libusb_open_with_vid_pid returns a device handle to the first matching device - which the app keeps open.
b. a second call to libusb_open_with_vid_pid returns a device handle to the second matching device (and not the first) - which the app keeps open.
c. a third call to libusb_open_with_vid_pid returns null (and none of the other devices).

libusb_close or process exit remove the advisory lock on the device. The specific advisory lock mechanism is flock(2). A complete patch with test plan and test results is included.

Please see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=291506 which includes source and CMakeLists.txt for the usbgrab utility. Ditto for the unit_test_libusb20_dev_open test program.

Test Plan

Bespoke code to test following (libusb) where Call #1 and Call #2 attempt to open the same connected device.

Call #1#1 ReturnsCall #2#2 ReturnsComment
libusb_openLIBUSB_SUCCESSlibusb_openLIBUSB_ERROR_BUSYon the same struct libusb_device pointer.
libusb_openLIBUSB_SUCCESSlibusb_openLIBUSB_ERROR_BUSYon different struct libusb_device pointers (same or different processes).

Bespoke code to test following (libusb20) where Call #1 and Call #2 attempt to open the same connected device. Note that libusb20_dev_open and libusb20_dev_open_with_flags(EXCLUSIVE) are the equivalent.

Call #1#1 ReturnsCall #2#2 ReturnsComment
libusb20_dev_openLIBUSB20_SUCCESSlibusb20_dev_openLIBUSB20_ERROR_BUSYon the same struct libusb20_device twice.
libusb20_dev_openLIBUSB20_SUCCESSlibusb20_dev_openLIBUSB20_ERROR_BUSYdifferent struct libusb20_device pointers (same or different processes).
libusb20_dev_open_with_flags(SHARED)LIBUSB20_SUCCESSlibusb20_dev_openLIBUSB20_ERROR_BUSYon the same struct libusb20_device twice.
libusb20_dev_open_with_flags(SHARED)LIBUSB20_SUCCESSlibusb20_dev_openLIBUSB20_ERROR_BUSYdifferent struct libusb20_device pointers (same or different processes).
libusb20_dev_open_with_flags(SHARED)LIBUSB20_SUCCESSlibusb20_dev_open_with_flags(SHARED)LIBUSB20_ERROR_BUSYon the same struct libusb20_device twice.
libusb20_dev_open_with_flags(SHARED)LIBUSB20_SUCCESSlibusb20_dev_open_with_flags(SHARED)LIBUSB20_SUCCESSdifferent struct libusb20_device pointers (same or different processes).
libusb20_dev_openLIBUSB20_SUCCESSlibusb20_dev_open_with_flags(SHARED)LIBUSB20_ERROR_BUSYon the same struct libusb20_device twice.
libusb20_dev_openLIBUSB20_SUCCESSlibusb20_dev_open_with_flags(SHARED)LIBUSB20_ERROR_BUSYdifferent struct libusb20_device pointers (same or different processes).

Additional testing: installed comms/sdr++ and ran with a suitable USB SDR device. This program has a Refresh button. With the program open but not attached to the device - the serial number is displayed. Run usbgrab <vid> <pid> -p in a shell, click Refresh in sdr++, the serial number disappears. Press enter in the shell window to exit usbgrab. Click Refresh again in sdr++ and the serial number re-appears. This is exactly the desired behavior from a libusb client. Clicking the Run button on sdr++ opens the USB device and begins streaming of bulk transfers from the device.

Also, now airspy_info and hydrasdr_info accurately report the correct number of compatible USB devices. Likewise, airspy_rx, airspyhd_rx and hydrasdr_rx also open and stream bulk transfers from compatible USB devices.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 69461
Build 66344: arc lint + arc unit

Event Timeline

lib/libusb/libusb20.h
264

Two proposed names for the new function. Please pick one and I'll remove the other.

The advantage of this review over D54173 is D54218 provides a consistent advisory lock for both libusb20 and libusb clients.

D54218: Introduce libusb20_dev_open_with_flags for exclusive access.
Non-exclusive access also checks for presence of an advisory lock.

D54218: Introduce libusb20_dev_open_with_flags for exclusive access.
Report LIBUSB20_ERROR_BUSY in place of LIBUSB20_ERROR_ACCESS for advisory lock conflicts.

unitrunker_unitrunker.net edited the test plan for this revision. (Show Details)

D54218: Introduce libusb20_dev_open_with_flags for exclusive access.
libusb20_dev_open is exclusive. Use libusb20_dev_open_with_flags for shared access.
This makes the unit test matrix simpler and is easier to document since libusb_dev_open already returns LIBUSB20_ERROR_BUSY to two calls to open the same libusb20_device pointer.

unitrunker_unitrunker.net retitled this revision from Introduce libusb20_dev_open_with_flags for exclusive access. to Introduce libusb20_dev_open_with_flags for shared vs. exclusive access..Tue, Dec 16, 12:12 PM
unitrunker_unitrunker.net edited the summary of this revision. (Show Details)
unitrunker_unitrunker.net edited the test plan for this revision. (Show Details)
unitrunker_unitrunker.net edited the test plan for this revision. (Show Details)

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Also offer LIBUSB20_OPEN_SHARED.

Compiling and running: ~/work/unit_test_libusb20_dev_open/build/unit_test 0BDA 2838
[ 50%] Building CXX object CMakeFiles/unit_test.dir/unit_test.cpp.o
[100%] Linking CXX executable unit_test
[100%] Built target unit_test
A(1 vs. 1 same) passes.
B(1 vs. 1 differs) passes.
C(1 vs. 0 same) passes.
D(1 vs. 0 differs) passes.
E(0 vs. 1 same) passes.
F(0 vs. 1 differs) passes.
G(0 vs. 0 same) passes.
H(0 vs. 0 differs) passes.

Should we document this error in libusb_open and libusb20_dev_open manuals? If so, is better to do it in this commit.

Should we document this error in libusb_open and libusb20_dev_open manuals? If so, is better to do it in this commit.

The BUSY error is documented in the other review for libusb20_dev_open.3 and already exists in libusb20.3. BUSY is NOT mentioned in libusb.3 for libusb_open.

Actually this is a great question because "BUSY" is only used for attach/detach kernel driver errors in libusb. Let me confirm what is the norm for opening a device already-in-use in libusb.

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
simplify libusb_open

Should we document this error in libusb_open and libusb20_dev_open manuals? If so, is better to do it in this commit.

Today - libusb_open returns LIBUSB_ERROR_NOMEM for any and all errors - which does not match behavior described in libusb.3. For example, it should return LIBUSB_ERROR_NO_DEVICE - but doesn't. I can add this case to the switch statement.

I should check @aokblast's other reviews to make sure I'm not going to step on something.

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Include LIBUSB_ERROR_NO_DEVICE as possible error for libusb_open.

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Include libusb.3 and libusb20.3 man pages.

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Removing manpage changes because

  1. clean up libusb.3 and libusb20.3 represents a lot of work that deserves a separate review review and
  2. I don't want to invest that effort until I have buy-in that this change will go forward.

Smaller changesets are easier to review and merge.

The doc should be changed in the commit that changes the behavior for many reasons, such as bisecting and downstream consumers.

The doc should be changed in the commit that changes the behavior for many reasons, such as bisecting and downstream consumers.

I'll update the commit to avoid new return values. Note that LIBUSB20_ERROR_BUSY isn't a new result for libusb_dev_open. I should not have added that translation to libusb_open as that is out of scope.

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Narrow scope of the review.

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
style(9) fix

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
Cloned libusb20_dev_open.3 from D54231

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
format example

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
punctuation

D54218: Introduce libusb20_dev_open_with_flags for shared vs. exclusive access.
RETURN VALUES