Hi,
On Wed, Jun 26, 2013 at 8:34 AM, Hans de Goede <hdego...@redhat.com> 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 the device's fd is already in the poll
fd set and will be caught by the next thread handling events.
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
