[Libusbx-devel] Improving linux hotplug disconnect
Hi, I have noticed some behavior that I believe most users will find undesirable. The situation is this: 1) The application opens a libusb device, which adds this to the poll fds 2) While the device is opened and IO is occurring, the device reboots or otherwise disappears from USB 3) When the device disappears, the device's fd immediately triggers a POLLERR condition, which is immediately seen by any active event handlers 4) At some later point in time, which varies greatly from hundreds of microseconds to several seconds, the udev monitor will get the disconnect notification and will then process the hotplug disconnect, removing the device from all contexts' device list and calling registers hotplug callbacks 5) Between the time that the POLLERR condition is seen and the udev monitor is notified, the application gets the list of devices, which still contains the device that has been disconnected (because it only gets removed by the udev monitor thread, which hasn't seen the removal yet). The application tries to open the device, but it fails with LIBUSB_ERROR_NO_DEVICE when the open() call to the fd fails. It continues to show up in the device list and fail to open up to the point that the udev monitor thread receives the notification of removal. It occurs to me that if a usb device is open and in the poll fd set, this will provide relatively immediate notification of device removal, because of the POLLERR condition. Once this condition is seen, immediate action can be taken to remove the device from the device lists and notify the hotplug callbacks. I don't know why udev can sometimes take up to several seconds to send the event, but it is undesirable to have a device listed while it is known to be unusable. Prior to hotplug, the described scenario would not have occurred since the usb device tree was scanned upon every get_device_list() call. I have played around and have come with an improvement for this situation. With my patch, any open devices that are disconnected will immediately be removed from all contexts' device list and have all hotplug callbacks called. Devices that are not currently open will still be subject to the speed of the udev monitor and may still show up in a context's device list, but this can also be improved to only happen at most one time. If an attempt to open a device that has been disconnected is made, it can be handled just like the POLLERR condition. In both of these situations, when the udev monitor thread finally gets the removal message, it will try to remove the device (by its session ID) but will find it has already been removed. Regards, Chris -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [PATCH] WinCE: Fix device reference leak which caused crash on libusb_exit().
Hi, Thanks for the patch, pushed to master. On 06/24/2013 01:31 PM, Toby Gray wrote: > The Windows CE device allocation code has always had a bug where it would > leak references to devices when they are allocated. This commit removes the > reference leak. > > This leak was highlighted by the new hotplug code which now triggers a NULL > pointer dereference if not all devices are unreferenced before libusb_exit > is called. > --- > libusb/os/wince_usb.c | 10 -- > 1 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/libusb/os/wince_usb.c b/libusb/os/wince_usb.c > index d8e8f55..76b559d 100644 > --- a/libusb/os/wince_usb.c > +++ b/libusb/os/wince_usb.c > @@ -341,10 +341,10 @@ static int wince_get_device_list( > UKW_DEVICE devices[MAX_DEVICE_COUNT]; > struct discovered_devs * new_devices = *discdevs; > DWORD count = 0, i; > - struct libusb_device *dev; > + struct libusb_device *dev = NULL; > unsigned char bus_addr, dev_addr; > unsigned long session_id; > - BOOL success, need_unref = FALSE; > + BOOL success; > DWORD release_list_offset = 0; > int r = LIBUSB_SUCCESS; > > @@ -378,7 +378,6 @@ static int wince_get_device_list( > r = LIBUSB_ERROR_NO_MEM; > goto err_out; > } > - need_unref = TRUE; > r = init_device(dev, devices[i], bus_addr, dev_addr); > if (r < 0) > goto err_out; > @@ -391,14 +390,13 @@ static int wince_get_device_list( > r = LIBUSB_ERROR_NO_MEM; > goto err_out; > } > - need_unref = FALSE; > + safe_unref_device(dev); > } > *discdevs = new_devices; > return r; > err_out: > *discdevs = new_devices; > - if (need_unref) > - libusb_unref_device(dev); > + safe_unref_device(dev); > // Release the remainder of the unprocessed device list. > // The devices added to new_devices already will still be passed up to > libusb, > // which can dispose of them at its leisure. > -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [PATCH] WinCE: Fix device reference leak which caused crash on libusb_exit().
Hi, On 06/26/2013 03:30 PM, Hans de Goede wrote: > Hi, > > Thanks for the patch, pushed to master. On second thought, I've found a problem with your patch, after your patch the code always unrefs dev, but if usbi_get_device_by_session_id() succeeds it returns a so called borrowed reference, so we should not unref it. I've just written and pushed a patch fixing this, by doing libusb_ref_device on dev if usbi_get_device_by_session_id() succeeds, turning the borrowed ref into a real ref (which we then later unref). Please test! Thanks & regards, Hans -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [PATCH] Initialize auto_detach_kernel_driver to 0 for new handle
Hi, On 06/25/2013 11:56 PM, Chris Dickens wrote: > Sorry, here's the patch with correct whitespace. Thanks for this, I thought that we were memsetting things to 0 on creation, so that this would not be necessary, but I was wrong. Patch pushed. Regards, Hans -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
[Libusbx-devel] [PATCH] Core: Avoid passing uninitialised data down the hotplug pipe.
Due to alignment requirements, libusb_hotplug_message might have some padding bytes. This change makes sure that these padding bytes are initialised. Valgrind no longer complains about passing uninitialised data to the write system call. --- libusb/core.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/libusb/core.c b/libusb/core.c index 4e01adb..828f0cb 100644 --- a/libusb/core.c +++ b/libusb/core.c @@ -531,6 +531,7 @@ void usbi_connect_device(struct libusb_device *dev) libusb_hotplug_message message; ssize_t ret; + memset(&message, 0, sizeof(message)); message.event = LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED; message.device = dev; dev->attached = 1; @@ -556,6 +557,7 @@ void usbi_disconnect_device(struct libusb_device *dev) struct libusb_context *ctx = dev->ctx; ssize_t ret; + memset(&message, 0, sizeof(message)); message.event = LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT; message.device = dev; usbi_mutex_lock(&dev->lock); -- 1.7.9.5 -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] Announcing libusbx-1.0.16-rc1
Hi, On 06/23/2013 04:51 PM, Xiaofan Chen wrote: >> Hmm, can you run under gdb, and then do a backtrace when it hangs / >> crashes (when it hangs first do ctrl+c to return to gdb). >> >> IE: >> >> gdb ./examples/listdevs >>> run >> ... >>> bt > > A bit problematic to use gdb here. > > $ ./examples/listdevs > 0483:3748 (bus 1, device 2) > Abort trap (core dumped) > $ gdb ./examples/listdevs > GNU gdb 6.3 > Copyright 2004 Free Software Foundation, Inc. > GDB is free software, covered by the GNU General Public License, and you are > welcome to change it and/or distribute copies of it under certain conditions. > Type "show copying" to see the conditions. > There is absolutely no warranty for GDB. Type "show warranty" for details. > This GDB was configured as > "i386-unknown-openbsd5.2"..."/home/mcuee/Desktop/build/libusbx/libusbx-1.0.16-rc1/examples/listdevs": > not in executable format: File format not recognized > > (gdb) run > Starting program: > No executable file specified. > Use the "file" or "exec-file" command. > (gdb) file ./examples/listdevs > "/home/mcuee/Desktop/build/libusbx/libusbx-1.0.16-rc1/examples/listdevs": > not in executable format: File format not recognized > (gdb) quit > > But I solved the problem by using --disable-shared > configure option. > > $ gdb ./examples/listdevs > GNU gdb 6.3 > Copyright 2004 Free Software Foundation, Inc. > GDB is free software, covered by the GNU General Public License, and you are > welcome to change it and/or distribute copies of it under certain conditions. > Type "show copying" to see the conditions. > There is absolutely no warranty for GDB. Type "show warranty" for details. > This GDB was configured as "i386-unknown-openbsd5.2"... > (gdb) run > Starting program: > /home/mcuee/Desktop/build/libusbx/libusbx-1.0.16-rc1/examples/listdevs > 0483:3748 (bus 1, device 2) > > Program received signal SIGABRT, Aborted. > 0x0cfda98d in kill () from /usr/lib/libc.so.65.0 > (gdb) bt > #0 0x0cfda98d in kill () from /usr/lib/libc.so.65.0 > #1 0x0d046545 in abort () at /usr/src/lib/libc/stdlib/abort.c:68 > #2 0x0a779065 in _rthread_mutex_lock (mutexp=0x853a2d18, trywait=0, > abstime=0x0) at /usr/src/lib/librthread/rthread_sync.c:127 > #3 0x1c001f13 in usbi_disconnect_device (dev=0x853a2800) at core.c:576 > #4 0x1c001ffe in libusb_unref_device (dev=0x853a2800) at core.c:991 > #5 0x1c002dd6 in libusb_exit (ctx=0x853a2d00) at core.c:1920 > #6 0x1c001295 in main () at listdevs.c:69 > (gdb) quit > The program is running. Exit anyway? (y or n) y Thanks, that explains what is going on. I've just pushed 2 commits to fix this. I would appreciate it if you could first run with only the first commit applied: "core: Only do hotplug cleanup for hotplug capable backends" Then you should get a warning on exit saying that some libusb_devices were leaked. Then try again with both commits present in your build, so also the: "openbsd: Fix memleak in obsd_get_device_list()" Commit and then that warning should be gone. Thanks & Regards, Hans -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] umask affects libusb_attach_kernel_driver()
Hi, On 06/25/2013 11:27 PM, Chris Dickens wrote: > Hi, > > I have discovered that the libusb process's file mask affects how > libusb_attach_kernel_driver() restores the driver. In my particular case, I > am dealing with usblp, so the kernel enumerates /dev/usb/lpX device files. > The /dev/usb folder is created and destroyed as necessary for the devices. > > When there is a single usblp device, the libusb_detach_kernel_driver() will > cause the folder to be removed. Upon reattaching the kernel driver, the > folder is restored, and the permissions are dictated by the process's file > mask. To clarify, the individual device files are not affected by the umask; > those permissions follow the udev rules. The permissions of the folder > containing the device files are affected. > > I was very surprised to see this behavior. I don't necessarily want my > application to set its own umask to deal with this issue. > > Does anyone have suggestions? Sounds like a kernel bug to me. I guess you're using devtmpfs on /dev ? Regards, Hans -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] Improving linux hotplug disconnect
Hi, On 06/26/2013 09:10 AM, Chris Dickens wrote: > Hi, > > I have noticed some behavior that I believe most users will find undesirable. > The situation is this: > > 1) The application opens a libusb device, which adds this to the poll fds > 2) While the device is opened and IO is occurring, the device reboots or > otherwise disappears from USB > 3) When the device disappears, the device's fd immediately triggers a POLLERR > condition, which is immediately seen by any active event handlers > 4) At some later point in time, which varies greatly from hundreds of > microseconds to several seconds, the udev monitor will get the disconnect > notification and will then process the hotplug disconnect, removing the > device from all contexts' device list and calling registers hotplug callbacks > 5) Between the time that the POLLERR condition is seen and the udev monitor > is notified, the application gets the list of devices, which still contains > the device that has been disconnected (because it only gets removed by the > udev monitor thread, which hasn't seen the removal yet). The application > tries to open the device, but it fails with LIBUSB_ERROR_NO_DEVICE when the > open() call to the fd fails. It continues to show up in the device list and > fail to open up to the point that the udev monitor thread receives the > notification of removal. > > It occurs to me that if a usb device is open and in the poll fd set, this > will provide relatively immediate notification of device removal, because of > the POLLERR condition. Once this condition is seen, immediate action can be > taken to remove the device from the device lists and notify the hotplug > callbacks. I don't know why udev can sometimes take up to several seconds to > send the event, but it is undesirable to have a device listed while it is > known to be unusable. Prior to hotplug, the described scenario would not have > occurred since the usb device tree was scanned upon every > get_device_list() call. Hmm, the libusb_poll call in get_device_list() should make this quite hard to trigger, this will only happen if udev has not yet read the kernel event, and send an event on its own socket. Which likely means that your setup is somehow cpu starved. With that said: > I have played around and have come with an improvement for this situation. > With my patch, any open devices that are disconnected will immediately be > removed from all contexts' device list and have all hotplug callbacks called. Although I'm not really a fan of having 2 device removed codepaths, I think this is probably a simple and useful patch. Can you post the patch for review please? > Devices that are not currently open will still be subject to the speed of > the udev monitor and may still show up in a context's device list, but this > can also be improved to only happen at most one time. If an attempt to open > a device that has been disconnected is made, it can be handled just like the > POLLERR condition. In both of these situations, when the udev monitor > thread finally gets the removal message, it will try to remove the device (by > its session ID) but will find it has already been removed. This sounds like a lot of dark magic for little gain, I would not bother with attempting something like this, I don't want to add device_disconnect calls to each place were we can get a ENOENT or ENODEV error, just to speed up detection of device removal by a few ms. Regards, Hans -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [PATCH] WinCE: Fix device reference leak which caused crash on libusb_exit().
On 26/06/13 15:44, Hans de Goede wrote: > Hi, > > On 06/26/2013 03:30 PM, Hans de Goede wrote: >> Hi, >> >> Thanks for the patch, pushed to master. > On second thought, I've found a problem with your patch, > after your patch the code always unrefs dev, but if > usbi_get_device_by_session_id() succeeds it returns a > so called borrowed reference, so we should not unref it. Good spot. Thanks for noticing that. > > I've just written and pushed a patch fixing this, by > doing libusb_ref_device on dev if > usbi_get_device_by_session_id() succeeds, turning > the borrowed ref into a real ref (which we then later > unref). > > Please test! All seems good with my testing. I think I'll try to write a test for this, although I'll need to have a think about the simplest way for testing the usbi_get_device_by_session_id() success path. Regards, Toby -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [PATCH] Core: Avoid passing uninitialised data down the hotplug pipe.
Hi, On 06/26/2013 05:26 PM, Toby Gray wrote: > Due to alignment requirements, libusb_hotplug_message might have > some padding bytes. > > This change makes sure that these padding bytes are > initialised. Valgrind no longer complains about passing uninitialised > data to the write system call. Thanks, applied and pushed. Regards, Hans > --- > libusb/core.c |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libusb/core.c b/libusb/core.c > index 4e01adb..828f0cb 100644 > --- a/libusb/core.c > +++ b/libusb/core.c > @@ -531,6 +531,7 @@ void usbi_connect_device(struct libusb_device *dev) > libusb_hotplug_message message; > ssize_t ret; > > + memset(&message, 0, sizeof(message)); > message.event = LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED; > message.device = dev; > dev->attached = 1; > @@ -556,6 +557,7 @@ void usbi_disconnect_device(struct libusb_device *dev) > struct libusb_context *ctx = dev->ctx; > ssize_t ret; > > + memset(&message, 0, sizeof(message)); > message.event = LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT; > message.device = dev; > usbi_mutex_lock(&dev->lock); > -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] Improving linux hotplug disconnect
Hi, On Wed, Jun 26, 2013 at 8:34 AM, Hans de Goede wrote: > Hi, > > Hmm, the libusb_poll call in get_device_list() should make this quite hard > to trigger, this will only > happen if udev has not yet read the kernel event, and send an event on its > own socket. Which likely > means that your setup is somehow cpu starved. Are you talking about the hotplug_poll() call? My system has two Intel Xeon X5560 CPUs, for a total of 16 cores. I'd be very surprised if this were a resource issue. The application at the time this occurs is running between 6 and 8 threads, and there are plenty of blocking calls in these threads to allow others to be scheduled. To verify this, I added a short timeout (10ms) to the linux_udev_event_thread_main() function's poll() call and added a usbi_dbg() call in the loop, and I see that the thread is getting scheduled regularly before the udev monitor event is received. Given this, I think it may be safe to rule out CPU starvation. For whatever reason, the udev event is just not being delivered on the socket. Although I'm not really a fan of having 2 device removed codepaths, I think > this is probably a > simple and useful patch. Can you post the patch for review please? diff --git a/libusb/os/linux_netlink.c b/libusb/os/linux_netlink.c index 3a68f69..6581ebb 100644 --- a/libusb/os/linux_netlink.c +++ b/libusb/os/linux_netlink.c @@ -214,7 +214,7 @@ static int linux_netlink_read_message(void) /* signal device is available (or not) to all contexts */ if (detached) - linux_hotplug_disconnected(busnum, devaddr, sys_name); + linux_device_disconnected(busnum, devaddr, sys_name); else linux_hotplug_enumerate(busnum, devaddr, sys_name); diff --git a/libusb/os/linux_udev.c b/libusb/os/linux_udev.c index 5a2aadf..08b2d70 100644 --- a/libusb/os/linux_udev.c +++ b/libusb/os/linux_udev.c @@ -207,7 +207,7 @@ static void udev_hotplug_event(struct udev_device* udev_dev) if (strncmp(udev_action, "add", 3) == 0) { linux_hotplug_enumerate(busnum, devaddr, sys_name); } else if (detached) { - linux_hotplug_disconnected(busnum, devaddr, sys_name); + linux_device_disconnected(busnum, devaddr, sys_name); } else { usbi_err(NULL, "ignoring udev action %s", udev_action); } diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c index 09288af..50f2d22 100644 --- a/libusb/os/linux_usbfs.c +++ b/libusb/os/linux_usbfs.c @@ -1072,7 +1072,7 @@ void linux_hotplug_enumerate(uint8_t busnum, uint8_t devaddr, const char *sys_na usbi_mutex_static_unlock(&active_contexts_lock); } -void linux_hotplug_disconnected(uint8_t busnum, uint8_t devaddr, const char *sys_name) +void linux_device_disconnected(uint8_t busnum, uint8_t devaddr, const char *sys_name) { struct libusb_context *ctx; struct libusb_device *dev; @@ -1247,8 +1247,18 @@ static int op_open(struct libusb_device_handle *handle) int r; hpriv->fd = _get_usbfs_fd(handle->dev, O_RDWR, 0); - if (hpriv->fd < 0) + if (hpriv->fd < 0) { + if (hpriv->fd == LIBUSB_ERROR_NO_DEVICE) { + /* device will still be marked as attached if hotplug monitor thread + * hasn't processed remove event yet */ + if (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, NULL); + } + } return hpriv->fd; + } r = ioctl(hpriv->fd, IOCTL_USBFS_GET_CAPABILITIES, &hpriv->caps); if (r < 0) { @@ -2482,6 +2492,11 @@ static int op_handle_events(struct libusb_context *ctx, if (pollfd->revents & POLLERR) { usbi_remove_pollfd(HANDLE_CTX(handle), hpriv->fd); usbi_handle_disconnect(handle); + /* device will still be marked as attached if hotplug monitor thread + * hasn't processed remove event yet */ + if (handle->dev->attached) + linux_device_disconnected(handle->dev->bus_number, + handle->dev->device_address, NULL); continue; } diff --git a/libusb/os/linux_usbfs.h b/libusb/os/linux_usbfs.h index 499bab7..1f5b191 100644 --- a/libusb/os/linux_usbfs.h +++ b/libusb/os/linux_usbfs.h @@ -170,7 +170,7 @@ void linux_netlink_hotplug_poll(void); #endif void linux_hotplug_enumerate(uint8_t busnum, uint8_t devaddr, const char *sys_name); -void linux_hotplug_disconnected(uint8_t busnum, uint8_t devaddr, const char *sys_name); +void linux_device_disconnected(uint8_t busnum, uint8_t devaddr, const char *sys_name); int linux_get_device_address (struct libusb_context *ctx, int detached, uint8_t *busnum, uint8_t *devaddr, const char *dev_node, > This sounds like a lot of dark magic for little gain, I would not bother > with attempting something > like this, I don't want to add device_disconnect calls to each place were > we can get a ENOENT or > ENODEV error, just to speed up detection of device removal by a few ms. > My proposal is to only add this check in two places: 1) In linux op_handle_events() 2) In op_open() In all other places that an ENOENT/ENODEV error can occur, a libusb handle is required, so we can assume that t
Re: [Libusbx-devel] umask affects libusb_attach_kernel_driver()
Yes, devtmpfs. Linux Kernel is 2.6.32-358 (RHEL6). Regards, Chris On Wed, Jun 26, 2013 at 8:23 AM, Hans de Goede wrote: > Hi, > > > On 06/25/2013 11:27 PM, Chris Dickens wrote: > >> Hi, >> >> I have discovered that the libusb process's file mask affects how >> libusb_attach_kernel_driver() restores the driver. In my particular case, I >> am dealing with usblp, so the kernel enumerates /dev/usb/lpX device files. >> The /dev/usb folder is created and destroyed as necessary for the devices. >> >> When there is a single usblp device, the libusb_detach_kernel_driver() >> will cause the folder to be removed. Upon reattaching the kernel driver, >> the folder is restored, and the permissions are dictated by the process's >> file mask. To clarify, the individual device files are not affected by the >> umask; those permissions follow the udev rules. The permissions of the >> folder containing the device files are affected. >> >> I was very surprised to see this behavior. I don't necessarily want my >> application to set its own umask to deal with this issue. >> >> Does anyone have suggestions? >> > > Sounds like a kernel bug to me. I guess you're using devtmpfs > on /dev ? > > Regards, > > Hans > -- This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel