nut icon indicating copy to clipboard operation
nut copied to clipboard

Extend libusb(1?) usage to specify `port=...` as a devfs path to the device node

Open jimklimov opened this issue 2 years ago • 3 comments

Currently our USB-capable drivers use port=auto (because ups.conf syntax requires some value) and a number of matchers to select the device at run-time. Discussion in #1273 and other issues suggested that libusb1 may have a way to actually specify a devfs path to the device node instead, to be on par with serial drivers using port=/dev/cuaa, or port=/dev/ttyS0 or port=/dev/ttyUSB1 (for USB->serial converter dongles) instead.

This ticket is a reminder to investigate and hopefully implement this ability, with a caveat that depending on OS setup (use of dynamic enumeration, re-plugging of same or neighboring devices, port resets and sleeps, etc.) such paths might be or not be volatile.

jimklimov avatar Sep 19 '23 07:09 jimklimov

FWIW, libusb1 as of 1.0.16 sports a libusb_get_port_numbers() API that returns the entire device path, rather than just the terminal port number. That would be a stable way to refer to a unique point in the USB topology, and is stable against enumeration order and unplug/replug events (so long as the replug is into the same port).

But I agree that port is probably a more useful, less bespoke approach and probably involves less code. In particular, one can use udev (or devd or similar) to give a symbolic link to a particular point in the USB topology. In udev, for example,

SUBSYSTEM=="usb", KERNEL=="5-1.1", OWNER="nut", SYMLINK+="ups0"

will create

$ ls /dev/ups0
lrwxrwxrwx 1 root root 15 Jun 26 09:37 /dev/ups0 -> bus/usb/005/011

as well as set the owner of the raw bus/usb device. Since 1.0.23, libusb1 has had libusb_wrap_sys_device to allow the use of such device nodes to directly access a given device without enumeration. See https://github.com/libusb/libusb/blob/e678b3fad58a508cbd0a6e6dc777fb48346f948b/examples/testlibusb.c#L242-L261 for an example of its use.

(ETA: it is purely a coincidence that device path "1.1" has device number "11" as of the commands above. There is no relation between these digit strings.)

Would you like me to try to raise a PR?

nwf avatar Jun 26 '24 09:06 nwf

Yes, thanks - I suppose that would be welcome (keeping port=auto for guessing as is done now). Just in case, keep in mind that not all world is Linux - though in this case it is rather for libusb1 project's headache to do things right. Just that wordings of such devfs paths can differ a lot.

In any case, if these methods only appeared from some intermediate version of the library, it sounds right to check for their existence in m4/nut_check_libusb.m4 and ifdef the new feature based on that (and also log a message that port!=auto is ignored in libusb0.c). If the provided path does not seem like an expected UPS in builds that support the feature, driver should fail to start; if the feature is not supported - log but ignore (do port=auto as we always did).

jimklimov avatar Jun 26 '24 14:06 jimklimov

OK, I tried and I'm sorry, but I don't think I'm going to be able to raise a PR in the near term.

  • libusb1 requires the client to continue to manage the system fd that gets wrapped by libusb_wrap_sys_device, and to close it after closing the USB device, which is not particularly pleasant and would require violence to the somewhat inconsistent abstraction layer around USBDevice_t and usb_communication_subdriver_t objects.

    • IMHO, it would be much better if the library dup()'d the descriptor and close()d the one they had when the device got closed; then the client could optionally retain the fd or not, as desired. Oh well.

    • Also IMHO, it's a bit odd to have both USBDevice_t-s and usb_dev_handle-s as part of the usb_communication_subdriver_t interface. The use of the latter basically ensures that this interface is a thin shim around libusb[01] and requires that all callers understand the internals (and proper destruction) of USBDevice_t.

  • The use of libusb_get_port_numbers might seem more enticing, but the violence required for the USBDeviceMatcher objects is also more than I want to take on right now; the other annoying bit, formatting a temporary string from the array contents for the matchers' use, at least occurs just once. (In particular, USBMATCHER_REGEXP_ARRAY_LIMIT is now defined by two nested conditionals and there's not just one optional slot in the resulting array. It's tempting to make all slots non-optional and just leave the regexp as NULL if the feature isn't supported, but even that is... tedious, but probably the easiest answer.)

For my case, busport happens to work, because the two UPSes are siblings on the same hub, but that's not a particularly fantastic requirement. So it goes.

nwf avatar Jul 01 '24 04:07 nwf