All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/3] tools/usbip: add usb event monitor into libusbip
@ 2021-10-27 19:30 Lars Gunnarsson
  2021-10-27 23:41 ` Shuah Khan
  0 siblings, 1 reply; 3+ messages in thread
From: Lars Gunnarsson @ 2021-10-27 19:30 UTC (permalink / raw)
  To: Valentina Manea, Shuah Khan, linux-usb

This patch implements an usb monitor into libusbip to synchronously wait for usb
events. The api is used in coming patches to allow "usbip bind" to export
device and "usbip attach" to import device, based on udev evens. Example of
api usage:

// wait for an usb divce to be plugged in:
usbip_monitor_t *monitor = usbip_monitor_new();
usbip_monitor_set_busid(monitor, "3-3.1.2.3");
usbip_monitor_await_usb_bind(monitor, "usb");  // this is a blocking call
// usb device with busid 3-3.1.2.3 is now bound to driver "usb".
usbip_monitor_delete(monitor);

Signed-off-by: Lars Gunnarsson <gunnarsson.lars@gmail.com>
---
v2: Change title, fix style warnings, improve feature description, add timeout
   into usbip_monitor.
v3: Change title and description.

Justifications of remaining warnings from "scripts/checkpatch.pl":

* Exception according to Linux kernel coding style 5.a where
  "usbip_monitor_t" is a totally opaque object:

  WARNING: do not add new typedefs
  #199: FILE: tools/usb/usbip/libsrc/usbip_monitor.h:8:
  +typedef struct usbip_monitor usbip_monitor_t;

 tools/usb/usbip/.gitignore             |   1 +
 tools/usb/usbip/libsrc/Makefile.am     |   3 +-
 tools/usb/usbip/libsrc/usbip_common.h  |   1 +
 tools/usb/usbip/libsrc/usbip_monitor.c | 159 +++++++++++++++++++++++++
 tools/usb/usbip/libsrc/usbip_monitor.h |  36 ++++++
 5 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 tools/usb/usbip/libsrc/usbip_monitor.c
 create mode 100644 tools/usb/usbip/libsrc/usbip_monitor.h

diff --git a/tools/usb/usbip/.gitignore b/tools/usb/usbip/.gitignore
index 597361a96dbb..6304adefb5e1 100644
--- a/tools/usb/usbip/.gitignore
+++ b/tools/usb/usbip/.gitignore
@@ -28,6 +28,7 @@ libsrc/libusbip_la-usbip_common.lo
 libsrc/libusbip_la-usbip_device_driver.lo
 libsrc/libusbip_la-usbip_host_common.lo
 libsrc/libusbip_la-usbip_host_driver.lo
+libsrc/libusbip_la-usbip_monitor.lo
 libsrc/libusbip_la-vhci_driver.lo
 src/usbip
 src/usbipd
diff --git a/tools/usb/usbip/libsrc/Makefile.am b/tools/usb/usbip/libsrc/Makefile.am
index dabd2c91d311..3e31e33729cf 100644
--- a/tools/usb/usbip/libsrc/Makefile.am
+++ b/tools/usb/usbip/libsrc/Makefile.am
@@ -8,4 +8,5 @@ libusbip_la_SOURCES := names.c names.h usbip_host_driver.c usbip_host_driver.h \
 		       usbip_device_driver.c usbip_device_driver.h \
 		       usbip_common.c usbip_common.h usbip_host_common.h \
 		       usbip_host_common.c vhci_driver.c vhci_driver.h \
-		       sysfs_utils.c sysfs_utils.h
+		       sysfs_utils.c sysfs_utils.h \
+		       usbip_monitor.c
diff --git a/tools/usb/usbip/libsrc/usbip_common.h b/tools/usb/usbip/libsrc/usbip_common.h
index 73a367a7fa10..13f1d4ca47c5 100644
--- a/tools/usb/usbip/libsrc/usbip_common.h
+++ b/tools/usb/usbip/libsrc/usbip_common.h
@@ -30,6 +30,7 @@

 /* kernel module names */
 #define USBIP_CORE_MOD_NAME	"usbip-core"
+#define USBIP_USB_DRV_NAME	"usb"
 #define USBIP_HOST_DRV_NAME	"usbip-host"
 #define USBIP_DEVICE_DRV_NAME	"usbip-vudc"
 #define USBIP_VHCI_DRV_NAME	"vhci_hcd"
diff --git a/tools/usb/usbip/libsrc/usbip_monitor.c b/tools/usb/usbip/libsrc/usbip_monitor.c
new file mode 100644
index 000000000000..ce60069d86ca
--- /dev/null
+++ b/tools/usb/usbip/libsrc/usbip_monitor.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Copyright (C) 2021 Lars Gunnarsson
+ */
+#include <libudev.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/poll.h>
+
+#include "usbip_monitor.h"
+
+struct usbip_monitor {
+	const char *busid;
+	int timeout_ms;
+	struct udev *udev;
+	struct udev_monitor *udev_monitor;
+};
+
+usbip_monitor_t *usbip_monitor_new(void)
+{
+	usbip_monitor_t *monitor = NULL;
+	struct udev *udev = udev_new();
+
+	if (udev) {
+		struct udev_monitor *udev_monitor =
+			udev_monitor_new_from_netlink(udev, "udev");
+		udev_monitor_filter_add_match_subsystem_devtype(
+			udev_monitor,
+			/*subsystem=*/"usb",
+			/*devtype=*/"usb_device");
+		udev_monitor_enable_receiving(udev_monitor);
+		monitor = malloc(sizeof(struct usbip_monitor));
+		monitor->busid = NULL;
+		monitor->timeout_ms = -1;
+		monitor->udev = udev;
+		monitor->udev_monitor = udev_monitor;
+	}
+	return monitor;
+}
+
+void usbip_monitor_delete(usbip_monitor_t *monitor)
+{
+	if (monitor) {
+		udev_monitor_unref(monitor->udev_monitor);
+		udev_unref(monitor->udev);
+		free(monitor);
+	}
+}
+
+void usbip_monitor_set_busid(usbip_monitor_t *monitor, const char *busid)
+{
+	monitor->busid = busid;
+}
+
+void usbip_monitor_set_timeout(usbip_monitor_t *monitor, int milliseconds)
+{
+	monitor->timeout_ms = milliseconds;
+}
+
+static struct udev_device *await_udev_event(const usbip_monitor_t *monitor)
+{
+	struct udev_device *dev = NULL;
+	int fd = udev_monitor_get_fd(monitor->udev_monitor);
+	const int nfds = 1;
+	struct pollfd pollfd[] = { { fd, POLLIN, 0 } };
+	int nfd = poll(pollfd, nfds, monitor->timeout_ms);
+
+	if (nfd)
+		dev = udev_monitor_receive_device(monitor->udev_monitor);
+	return dev;
+}
+
+static int optional_filter_busid(const char *busid, const char *udev_busid)
+{
+	int filter_match = 0;
+
+	if (busid) {
+		if (strcmp(busid, udev_busid) == 0)
+			filter_match = 1;
+	} else {
+		filter_match = 1;
+	}
+	return filter_match;
+}
+
+static bool await_usb_with_driver(const usbip_monitor_t *monitor,
+				  const char *driver, const char *action)
+{
+	bool event_occured = false;
+
+	while (!event_occured) {
+		struct udev_device *dev = await_udev_event(monitor);
+
+		if (dev) {
+			const char *udev_action = udev_device_get_action(dev);
+			const char *udev_driver = udev_device_get_driver(dev);
+			const char *udev_busid = udev_device_get_sysname(dev);
+
+			if (strcmp(udev_action, action) == 0 &&
+			    strcmp(udev_driver, driver) == 0) {
+				if (optional_filter_busid(monitor->busid,
+							  udev_busid)) {
+					event_occured = true;
+				}
+			}
+			udev_device_unref(dev);
+		} else {
+			break;
+		}
+	}
+	return event_occured;
+}
+
+bool usbip_monitor_await_usb_add(const usbip_monitor_t *monitor,
+				 const char *driver)
+{
+	return await_usb_with_driver(monitor, driver, "add");
+}
+
+bool usbip_monitor_await_usb_bind(const usbip_monitor_t *monitor,
+				  const char *driver)
+{
+	return await_usb_with_driver(monitor, driver, "bind");
+}
+
+static bool await_usb(const usbip_monitor_t *monitor, const char *action)
+{
+	bool event_occured = false;
+
+	while (!event_occured) {
+		struct udev_device *dev = await_udev_event(monitor);
+
+		if (dev) {
+			const char *udev_action = udev_device_get_action(dev);
+			const char *udev_busid = udev_device_get_sysname(dev);
+
+			if (strcmp(udev_action, action) == 0) {
+				if (optional_filter_busid(monitor->busid,
+							  udev_busid)) {
+					event_occured = true;
+				}
+			}
+			udev_device_unref(dev);
+		} else {
+			break;
+		}
+	}
+	return event_occured;
+}
+
+bool usbip_monitor_await_usb_unbind(const usbip_monitor_t *monitor)
+{
+	return await_usb(monitor, "unbind");
+}
+
+bool usbip_monitor_await_usb_delete(const usbip_monitor_t *monitor)
+{
+	return await_usb(monitor, "delete");
+}
diff --git a/tools/usb/usbip/libsrc/usbip_monitor.h b/tools/usb/usbip/libsrc/usbip_monitor.h
new file mode 100644
index 000000000000..750abb6b79e0
--- /dev/null
+++ b/tools/usb/usbip/libsrc/usbip_monitor.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * Copyright (C) 2021 Lars Gunnarsson
+ */
+#ifndef __USBIP_MONITOR_H
+#define __USBIP_MONITOR_H
+
+#include <stdbool.h>
+
+typedef struct usbip_monitor usbip_monitor_t;
+
+usbip_monitor_t *usbip_monitor_new(void);
+void usbip_monitor_delete(usbip_monitor_t *monitor);
+
+/**
+ * Set busid to await events on. If unset, any busid will be matched.
+ */
+void usbip_monitor_set_busid(usbip_monitor_t *monitor, const char *busid);
+
+/**
+ * Set timeout for await calls in milliseconds, default is no timeout (-1).
+ */
+void usbip_monitor_set_timeout(usbip_monitor_t *monitor, int milliseconds);
+
+/**
+ * Functions below is blocking. Returns true if event occurred, or false on
+ * timeouts.
+ */
+bool usbip_monitor_await_usb_add(const usbip_monitor_t *monitor,
+				 const char *driver);
+bool usbip_monitor_await_usb_bind(const usbip_monitor_t *monitor,
+				  const char *driver);
+bool usbip_monitor_await_usb_unbind(const usbip_monitor_t *monitor);
+bool usbip_monitor_await_usb_delete(const usbip_monitor_t *monitor);
+
+#endif /* __USBIP_MONITOR_H */
--
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 1/3] tools/usbip: add usb event monitor into libusbip
  2021-10-27 19:30 [PATCH v3 1/3] tools/usbip: add usb event monitor into libusbip Lars Gunnarsson
@ 2021-10-27 23:41 ` Shuah Khan
  2021-11-03 21:08   ` Lars Gunnarsson
  0 siblings, 1 reply; 3+ messages in thread
From: Shuah Khan @ 2021-10-27 23:41 UTC (permalink / raw)
  To: Lars Gunnarsson, Valentina Manea, Shuah Khan, linux-usb, Shuah Khan

On 10/27/21 1:30 PM, Lars Gunnarsson wrote:
> This patch implements an usb monitor into libusbip to synchronously wait for usb
> events. The api is used in coming patches to allow "usbip bind" to export
> device and "usbip attach" to import device, based on udev evens

events?

Example of
> api usage:
> 
> // wait for an usb divce to be plugged in:
> usbip_monitor_t *monitor = usbip_monitor_new();
> usbip_monitor_set_busid(monitor, "3-3.1.2.3");
> usbip_monitor_await_usb_bind(monitor, "usb");  // this is a blocking call
> // usb device with busid 3-3.1.2.3 is now bound to driver "usb".
> usbip_monitor_delete(monitor);
> 

Whe and where should this API be used? Can you describe how this API
fits into the usbip host and clinet sides?

> Signed-off-by: Lars Gunnarsson <gunnarsson.lars@gmail.com>
> ---
> v2: Change title, fix style warnings, improve feature description, add timeout
>     into usbip_monitor.
> v3: Change title and description.
> 
> Justifications of remaining warnings from "scripts/checkpatch.pl":
> 
> * Exception according to Linux kernel coding style 5.a where
>    "usbip_monitor_t" is a totally opaque object:
> 
>    WARNING: do not add new typedefs
>    #199: FILE: tools/usb/usbip/libsrc/usbip_monitor.h:8:
>    +typedef struct usbip_monitor usbip_monitor_t;
> 
>   tools/usb/usbip/.gitignore             |   1 +
>   tools/usb/usbip/libsrc/Makefile.am     |   3 +-
>   tools/usb/usbip/libsrc/usbip_common.h  |   1 +
>   tools/usb/usbip/libsrc/usbip_monitor.c | 159 +++++++++++++++++++++++++
>   tools/usb/usbip/libsrc/usbip_monitor.h |  36 ++++++
>   5 files changed, 199 insertions(+), 1 deletion(-)
>   create mode 100644 tools/usb/usbip/libsrc/usbip_monitor.c
>   create mode 100644 tools/usb/usbip/libsrc/usbip_monitor.h
> 
> diff --git a/tools/usb/usbip/.gitignore b/tools/usb/usbip/.gitignore
> index 597361a96dbb..6304adefb5e1 100644
> --- a/tools/usb/usbip/.gitignore
> +++ b/tools/usb/usbip/.gitignore
> @@ -28,6 +28,7 @@ libsrc/libusbip_la-usbip_common.lo
>   libsrc/libusbip_la-usbip_device_driver.lo
>   libsrc/libusbip_la-usbip_host_common.lo
>   libsrc/libusbip_la-usbip_host_driver.lo
> +libsrc/libusbip_la-usbip_monitor.lo
>   libsrc/libusbip_la-vhci_driver.lo
>   src/usbip
>   src/usbipd
> diff --git a/tools/usb/usbip/libsrc/Makefile.am b/tools/usb/usbip/libsrc/Makefile.am
> index dabd2c91d311..3e31e33729cf 100644
> --- a/tools/usb/usbip/libsrc/Makefile.am
> +++ b/tools/usb/usbip/libsrc/Makefile.am
> @@ -8,4 +8,5 @@ libusbip_la_SOURCES := names.c names.h usbip_host_driver.c usbip_host_driver.h \
>   		       usbip_device_driver.c usbip_device_driver.h \
>   		       usbip_common.c usbip_common.h usbip_host_common.h \
>   		       usbip_host_common.c vhci_driver.c vhci_driver.h \
> -		       sysfs_utils.c sysfs_utils.h
> +		       sysfs_utils.c sysfs_utils.h \
> +		       usbip_monitor.c
> diff --git a/tools/usb/usbip/libsrc/usbip_common.h b/tools/usb/usbip/libsrc/usbip_common.h
> index 73a367a7fa10..13f1d4ca47c5 100644
> --- a/tools/usb/usbip/libsrc/usbip_common.h
> +++ b/tools/usb/usbip/libsrc/usbip_common.h
> @@ -30,6 +30,7 @@
> 
>   /* kernel module names */
>   #define USBIP_CORE_MOD_NAME	"usbip-core"
> +#define USBIP_USB_DRV_NAME	"usb"

Shouldn't this be usb-monitor?

>   #define USBIP_HOST_DRV_NAME	"usbip-host"
>   #define USBIP_DEVICE_DRV_NAME	"usbip-vudc"
>   #define USBIP_VHCI_DRV_NAME	"vhci_hcd"
> diff --git a/tools/usb/usbip/libsrc/usbip_monitor.c b/tools/usb/usbip/libsrc/usbip_monitor.c
> new file mode 100644
> index 000000000000..ce60069d86ca
> --- /dev/null
> +++ b/tools/usb/usbip/libsrc/usbip_monitor.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * Copyright (C) 2021 Lars Gunnarsson

Add your email as well here. This should look like the Singed-off-by

> + */
> +#include <libudev.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/poll.h>
> +
> +#include "usbip_monitor.h"
> +
> +struct usbip_monitor {
> +	const char *busid;
> +	int timeout_ms;
> +	struct udev *udev;
> +	struct udev_monitor *udev_monitor;
> +};
> +
> +usbip_monitor_t *usbip_monitor_new(void)
> +{
> +	usbip_monitor_t *monitor = NULL;
> +	struct udev *udev = udev_new();
> +
> +	if (udev) {
> +		struct udev_monitor *udev_monitor =
> +			udev_monitor_new_from_netlink(udev, "udev");
> +		udev_monitor_filter_add_match_subsystem_devtype(

This looks odd and not readable. The name is very long.


> +			udev_monitor,
> +			/*subsystem=*/"usb",
> +			/*devtype=*/"usb_device");

These comments look odd.

> +		udev_monitor_enable_receiving(udev_monitor);
> +		monitor = malloc(sizeof(struct usbip_monitor));
> +		monitor->busid = NULL;
> +		monitor->timeout_ms = -1;
> +		monitor->udev = udev;
> +		monitor->udev_monitor = udev_monitor;
> +	}
> +	return monitor;
> +}
> +
> +void usbip_monitor_delete(usbip_monitor_t *monitor)
> +{
> +	if (monitor) {
> +		udev_monitor_unref(monitor->udev_monitor);
> +		udev_unref(monitor->udev);
> +		free(monitor);
> +	}
> +}
> +
> +void usbip_monitor_set_busid(usbip_monitor_t *monitor, const char *busid)
> +{
> +	monitor->busid = busid;
> +}
> +
> +void usbip_monitor_set_timeout(usbip_monitor_t *monitor, int milliseconds)
> +{
> +	monitor->timeout_ms = milliseconds;
> +}
> +
> +static struct udev_device *await_udev_event(const usbip_monitor_t *monitor)
> +{
> +	struct udev_device *dev = NULL;
> +	int fd = udev_monitor_get_fd(monitor->udev_monitor);

Why not check if monitor is null?

> +	const int nfds = 1;
> +	struct pollfd pollfd[] = { { fd, POLLIN, 0 } };
> +	int nfd = poll(pollfd, nfds, monitor->timeout_ms);
> +
> +	if (nfd)
> +		dev = udev_monitor_receive_device(monitor->udev_monitor);
> +	return dev;
> +}
> +
> +static int optional_filter_busid(const char *busid, const char *udev_busid)
> +{
> +	int filter_match = 0;
> +
> +	if (busid) {
> +		if (strcmp(busid, udev_busid) == 0)
> +			filter_match = 1;
> +	} else {
> +		filter_match = 1;
> +	}
> +	return filter_match;
> +}
> +
> +static bool await_usb_with_driver(const usbip_monitor_t *monitor,
> +				  const char *driver, const char *action)
> +{
> +	bool event_occured = false;
> +
> +	while (!event_occured) {
> +		struct udev_device *dev = await_udev_event(monitor);
> +
> +		if (dev) {
> +			const char *udev_action = udev_device_get_action(dev);
> +			const char *udev_driver = udev_device_get_driver(dev);
> +			const char *udev_busid = udev_device_get_sysname(dev);
> +
> +			if (strcmp(udev_action, action) == 0 &&
> +			    strcmp(udev_driver, driver) == 0) {
> +				if (optional_filter_busid(monitor->busid,
> +							  udev_busid)) {
> +					event_occured = true;
> +				}
> +			}
> +			udev_device_unref(dev);
> +		} else {
> +			break;
> +		}
> +	}
> +	return event_occured;
> +}
> +
> +bool usbip_monitor_await_usb_add(const usbip_monitor_t *monitor,
> +				 const char *driver)
> +{
> +	return await_usb_with_driver(monitor, driver, "add");
> +}
> +
> +bool usbip_monitor_await_usb_bind(const usbip_monitor_t *monitor,
> +				  const char *driver)
> +{
> +	return await_usb_with_driver(monitor, driver, "bind");
> +}
> +
> +static bool await_usb(const usbip_monitor_t *monitor, const char *action)
> +{
> +	bool event_occured = false;
> +
> +	while (!event_occured) {
> +		struct udev_device *dev = await_udev_event(monitor);
> +
> +		if (dev) {
> +			const char *udev_action = udev_device_get_action(dev);
> +			const char *udev_busid = udev_device_get_sysname(dev);
> +
> +			if (strcmp(udev_action, action) == 0) {
> +				if (optional_filter_busid(monitor->busid,
> +							  udev_busid)) {
> +					event_occured = true;
> +				}
> +			}
> +			udev_device_unref(dev);
> +		} else {
> +			break;
> +		}
> +	}
> +	return event_occured;
> +}
> +
> +bool usbip_monitor_await_usb_unbind(const usbip_monitor_t *monitor)
> +{
> +	return await_usb(monitor, "unbind");
> +}
> +
> +bool usbip_monitor_await_usb_delete(const usbip_monitor_t *monitor)
> +{
> +	return await_usb(monitor, "delete");
> +}
> diff --git a/tools/usb/usbip/libsrc/usbip_monitor.h b/tools/usb/usbip/libsrc/usbip_monitor.h
> new file mode 100644
> index 000000000000..750abb6b79e0
> --- /dev/null
> +++ b/tools/usb/usbip/libsrc/usbip_monitor.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/**
> + * Copyright (C) 2021 Lars Gunnarsson
> + */
> +#ifndef __USBIP_MONITOR_H
> +#define __USBIP_MONITOR_H
> +
> +#include <stdbool.h>
> +
> +typedef struct usbip_monitor usbip_monitor_t;
> +
> +usbip_monitor_t *usbip_monitor_new(void);
> +void usbip_monitor_delete(usbip_monitor_t *monitor);
> +
> +/**
> + * Set busid to await events on. If unset, any busid will be matched.
> + */
> +void usbip_monitor_set_busid(usbip_monitor_t *monitor, const char *busid);
> +
> +/**
> + * Set timeout for await calls in milliseconds, default is no timeout (-1).
> + */
> +void usbip_monitor_set_timeout(usbip_monitor_t *monitor, int milliseconds);
> +
> +/**
> + * Functions below is blocking. Returns true if event occurred, or false on
> + * timeouts.
> + */
> +bool usbip_monitor_await_usb_add(const usbip_monitor_t *monitor,
> +				 const char *driver);
> +bool usbip_monitor_await_usb_bind(const usbip_monitor_t *monitor,
> +				  const char *driver);
> +bool usbip_monitor_await_usb_unbind(const usbip_monitor_t *monitor);
> +bool usbip_monitor_await_usb_delete(const usbip_monitor_t *monitor);
> +
> +#endif /* __USBIP_MONITOR_H */
> --
> 2.25.1
> 
> 

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 1/3] tools/usbip: add usb event monitor into libusbip
  2021-10-27 23:41 ` Shuah Khan
@ 2021-11-03 21:08   ` Lars Gunnarsson
  0 siblings, 0 replies; 3+ messages in thread
From: Lars Gunnarsson @ 2021-11-03 21:08 UTC (permalink / raw)
  To: Shuah Khan; +Cc: Valentina Manea, Shuah Khan, linux-usb

Thanks for the comments, below are my responses.

On Wed, Oct 27, 2021 at 05:41:33PM -0600, Shuah Khan wrote:
> On 10/27/21 1:30 PM, Lars Gunnarsson wrote:
> > This patch implements an usb monitor into libusbip to synchronously wait for usb
> > events. The api is used in coming patches to allow "usbip bind" to export
> > device and "usbip attach" to import device, based on udev evens
> 
> events?

Will fix this.

> 
> Example of
> > api usage:
> > 
> > // wait for an usb divce to be plugged in:
> > usbip_monitor_t *monitor = usbip_monitor_new();
> > usbip_monitor_set_busid(monitor, "3-3.1.2.3");
> > usbip_monitor_await_usb_bind(monitor, "usb");  // this is a blocking call
> > // usb device with busid 3-3.1.2.3 is now bound to driver "usb".
> > usbip_monitor_delete(monitor);
> > 
> 
> Whe and where should this API be used? Can you describe how this API
> fits into the usbip host and clinet sides?

Will fix this.

> 
> > Signed-off-by: Lars Gunnarsson <gunnarsson.lars@gmail.com>
> > ---
> > v2: Change title, fix style warnings, improve feature description, add timeout
> >     into usbip_monitor.
> > v3: Change title and description.
> > 
> > Justifications of remaining warnings from "scripts/checkpatch.pl":
> > 
> > * Exception according to Linux kernel coding style 5.a where
> >    "usbip_monitor_t" is a totally opaque object:
> > 
> >    WARNING: do not add new typedefs
> >    #199: FILE: tools/usb/usbip/libsrc/usbip_monitor.h:8:
> >    +typedef struct usbip_monitor usbip_monitor_t;
> > 
> >   tools/usb/usbip/.gitignore             |   1 +
> >   tools/usb/usbip/libsrc/Makefile.am     |   3 +-
> >   tools/usb/usbip/libsrc/usbip_common.h  |   1 +
> >   tools/usb/usbip/libsrc/usbip_monitor.c | 159 +++++++++++++++++++++++++
> >   tools/usb/usbip/libsrc/usbip_monitor.h |  36 ++++++
> >   5 files changed, 199 insertions(+), 1 deletion(-)
> >   create mode 100644 tools/usb/usbip/libsrc/usbip_monitor.c
> >   create mode 100644 tools/usb/usbip/libsrc/usbip_monitor.h
> > 
> > diff --git a/tools/usb/usbip/.gitignore b/tools/usb/usbip/.gitignore
> > index 597361a96dbb..6304adefb5e1 100644
> > --- a/tools/usb/usbip/.gitignore
> > +++ b/tools/usb/usbip/.gitignore
> > @@ -28,6 +28,7 @@ libsrc/libusbip_la-usbip_common.lo
> >   libsrc/libusbip_la-usbip_device_driver.lo
> >   libsrc/libusbip_la-usbip_host_common.lo
> >   libsrc/libusbip_la-usbip_host_driver.lo
> > +libsrc/libusbip_la-usbip_monitor.lo
> >   libsrc/libusbip_la-vhci_driver.lo
> >   src/usbip
> >   src/usbipd
> > diff --git a/tools/usb/usbip/libsrc/Makefile.am b/tools/usb/usbip/libsrc/Makefile.am
> > index dabd2c91d311..3e31e33729cf 100644
> > --- a/tools/usb/usbip/libsrc/Makefile.am
> > +++ b/tools/usb/usbip/libsrc/Makefile.am
> > @@ -8,4 +8,5 @@ libusbip_la_SOURCES := names.c names.h usbip_host_driver.c usbip_host_driver.h \
> >   		       usbip_device_driver.c usbip_device_driver.h \
> >   		       usbip_common.c usbip_common.h usbip_host_common.h \
> >   		       usbip_host_common.c vhci_driver.c vhci_driver.h \
> > -		       sysfs_utils.c sysfs_utils.h
> > +		       sysfs_utils.c sysfs_utils.h \
> > +		       usbip_monitor.c
> > diff --git a/tools/usb/usbip/libsrc/usbip_common.h b/tools/usb/usbip/libsrc/usbip_common.h
> > index 73a367a7fa10..13f1d4ca47c5 100644
> > --- a/tools/usb/usbip/libsrc/usbip_common.h
> > +++ b/tools/usb/usbip/libsrc/usbip_common.h
> > @@ -30,6 +30,7 @@
> > 
> >   /* kernel module names */
> >   #define USBIP_CORE_MOD_NAME	"usbip-core"
> > +#define USBIP_USB_DRV_NAME	"usb"
> 
> Shouldn't this be usb-monitor?

USBIP_USB_DRV_NAME is for targeting driver "usb" that was missing here.

> 
> >   #define USBIP_HOST_DRV_NAME	"usbip-host"
> >   #define USBIP_DEVICE_DRV_NAME	"usbip-vudc"
> >   #define USBIP_VHCI_DRV_NAME	"vhci_hcd"
> > diff --git a/tools/usb/usbip/libsrc/usbip_monitor.c b/tools/usb/usbip/libsrc/usbip_monitor.c
> > new file mode 100644
> > index 000000000000..ce60069d86ca
> > --- /dev/null
> > +++ b/tools/usb/usbip/libsrc/usbip_monitor.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * Copyright (C) 2021 Lars Gunnarsson
> 
> Add your email as well here. This should look like the Singed-off-by

Will fix this.

> 
> > + */
> > +#include <libudev.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/poll.h>
> > +
> > +#include "usbip_monitor.h"
> > +
> > +struct usbip_monitor {
> > +	const char *busid;
> > +	int timeout_ms;
> > +	struct udev *udev;
> > +	struct udev_monitor *udev_monitor;
> > +};
> > +
> > +usbip_monitor_t *usbip_monitor_new(void)
> > +{
> > +	usbip_monitor_t *monitor = NULL;
> > +	struct udev *udev = udev_new();
> > +
> > +	if (udev) {
> > +		struct udev_monitor *udev_monitor =
> > +			udev_monitor_new_from_netlink(udev, "udev");
> > +		udev_monitor_filter_add_match_subsystem_devtype(
> 
> This looks odd and not readable. The name is very long.

This is part of udev api (libudev.h) and unfortunately out of my hands.

> 
> 
> > +			udev_monitor,
> > +			/*subsystem=*/"usb",
> > +			/*devtype=*/"usb_device");
> 
> These comments look odd.

Will remove these.

> 
> > +		udev_monitor_enable_receiving(udev_monitor);
> > +		monitor = malloc(sizeof(struct usbip_monitor));
> > +		monitor->busid = NULL;
> > +		monitor->timeout_ms = -1;
> > +		monitor->udev = udev;
> > +		monitor->udev_monitor = udev_monitor;
> > +	}
> > +	return monitor;
> > +}
> > +
> > +void usbip_monitor_delete(usbip_monitor_t *monitor)
> > +{
> > +	if (monitor) {
> > +		udev_monitor_unref(monitor->udev_monitor);
> > +		udev_unref(monitor->udev);
> > +		free(monitor);
> > +	}
> > +}
> > +
> > +void usbip_monitor_set_busid(usbip_monitor_t *monitor, const char *busid)
> > +{
> > +	monitor->busid = busid;
> > +}
> > +
> > +void usbip_monitor_set_timeout(usbip_monitor_t *monitor, int milliseconds)
> > +{
> > +	monitor->timeout_ms = milliseconds;
> > +}
> > +
> > +static struct udev_device *await_udev_event(const usbip_monitor_t *monitor)
> > +{
> > +	struct udev_device *dev = NULL;
> > +	int fd = udev_monitor_get_fd(monitor->udev_monitor);
> 
> Why not check if monitor is null?

Will fix this.

> 
> > +	const int nfds = 1;
> > +	struct pollfd pollfd[] = { { fd, POLLIN, 0 } };
> > +	int nfd = poll(pollfd, nfds, monitor->timeout_ms);
> > +
> > +	if (nfd)
> > +		dev = udev_monitor_receive_device(monitor->udev_monitor);
> > +	return dev;
> > +}
> > +
> > +static int optional_filter_busid(const char *busid, const char *udev_busid)
> > +{
> > +	int filter_match = 0;
> > +
> > +	if (busid) {
> > +		if (strcmp(busid, udev_busid) == 0)
> > +			filter_match = 1;
> > +	} else {
> > +		filter_match = 1;
> > +	}
> > +	return filter_match;
> > +}
> > +
> > +static bool await_usb_with_driver(const usbip_monitor_t *monitor,
> > +				  const char *driver, const char *action)
> > +{
> > +	bool event_occured = false;
> > +
> > +	while (!event_occured) {
> > +		struct udev_device *dev = await_udev_event(monitor);
> > +
> > +		if (dev) {
> > +			const char *udev_action = udev_device_get_action(dev);
> > +			const char *udev_driver = udev_device_get_driver(dev);
> > +			const char *udev_busid = udev_device_get_sysname(dev);
> > +
> > +			if (strcmp(udev_action, action) == 0 &&
> > +			    strcmp(udev_driver, driver) == 0) {
> > +				if (optional_filter_busid(monitor->busid,
> > +							  udev_busid)) {
> > +					event_occured = true;
> > +				}
> > +			}
> > +			udev_device_unref(dev);
> > +		} else {
> > +			break;
> > +		}
> > +	}
> > +	return event_occured;
> > +}
> > +
> > +bool usbip_monitor_await_usb_add(const usbip_monitor_t *monitor,
> > +				 const char *driver)
> > +{
> > +	return await_usb_with_driver(monitor, driver, "add");
> > +}
> > +
> > +bool usbip_monitor_await_usb_bind(const usbip_monitor_t *monitor,
> > +				  const char *driver)
> > +{
> > +	return await_usb_with_driver(monitor, driver, "bind");
> > +}
> > +
> > +static bool await_usb(const usbip_monitor_t *monitor, const char *action)
> > +{
> > +	bool event_occured = false;
> > +
> > +	while (!event_occured) {
> > +		struct udev_device *dev = await_udev_event(monitor);
> > +
> > +		if (dev) {
> > +			const char *udev_action = udev_device_get_action(dev);
> > +			const char *udev_busid = udev_device_get_sysname(dev);
> > +
> > +			if (strcmp(udev_action, action) == 0) {
> > +				if (optional_filter_busid(monitor->busid,
> > +							  udev_busid)) {
> > +					event_occured = true;
> > +				}
> > +			}
> > +			udev_device_unref(dev);
> > +		} else {
> > +			break;
> > +		}
> > +	}
> > +	return event_occured;
> > +}
> > +
> > +bool usbip_monitor_await_usb_unbind(const usbip_monitor_t *monitor)
> > +{
> > +	return await_usb(monitor, "unbind");
> > +}
> > +
> > +bool usbip_monitor_await_usb_delete(const usbip_monitor_t *monitor)
> > +{
> > +	return await_usb(monitor, "delete");
> > +}
> > diff --git a/tools/usb/usbip/libsrc/usbip_monitor.h b/tools/usb/usbip/libsrc/usbip_monitor.h
> > new file mode 100644
> > index 000000000000..750abb6b79e0
> > --- /dev/null
> > +++ b/tools/usb/usbip/libsrc/usbip_monitor.h
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/**
> > + * Copyright (C) 2021 Lars Gunnarsson
> > + */
> > +#ifndef __USBIP_MONITOR_H
> > +#define __USBIP_MONITOR_H
> > +
> > +#include <stdbool.h>
> > +
> > +typedef struct usbip_monitor usbip_monitor_t;
> > +
> > +usbip_monitor_t *usbip_monitor_new(void);
> > +void usbip_monitor_delete(usbip_monitor_t *monitor);
> > +
> > +/**
> > + * Set busid to await events on. If unset, any busid will be matched.
> > + */
> > +void usbip_monitor_set_busid(usbip_monitor_t *monitor, const char *busid);
> > +
> > +/**
> > + * Set timeout for await calls in milliseconds, default is no timeout (-1).
> > + */
> > +void usbip_monitor_set_timeout(usbip_monitor_t *monitor, int milliseconds);
> > +
> > +/**
> > + * Functions below is blocking. Returns true if event occurred, or false on
> > + * timeouts.
> > + */
> > +bool usbip_monitor_await_usb_add(const usbip_monitor_t *monitor,
> > +				 const char *driver);
> > +bool usbip_monitor_await_usb_bind(const usbip_monitor_t *monitor,
> > +				  const char *driver);
> > +bool usbip_monitor_await_usb_unbind(const usbip_monitor_t *monitor);
> > +bool usbip_monitor_await_usb_delete(const usbip_monitor_t *monitor);
> > +
> > +#endif /* __USBIP_MONITOR_H */
> > --
> > 2.25.1
> > 
> > 
> 
> thanks,
> -- Shuah

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-11-03 21:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-27 19:30 [PATCH v3 1/3] tools/usbip: add usb event monitor into libusbip Lars Gunnarsson
2021-10-27 23:41 ` Shuah Khan
2021-11-03 21:08   ` Lars Gunnarsson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.