All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines
@ 2016-06-02 11:51 Linus Walleij
  2016-06-02 11:51 ` [PATCH 2/4] tools/gpio: add the gpio-hammer tool Linus Walleij
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Linus Walleij @ 2016-06-02 11:51 UTC (permalink / raw)
  To: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov
  Cc: Bamvor Jian Zhang, Grant Likely, Arnd Bergmann, Mark Brown,
	Johan Hovold, Linus Walleij

This adds a userspace ABI for reading and writing GPIO lines.
The mechanism returns an anonymous file handle to a request
to read/write n offsets from a gpiochip. This file handle
in turn accepts two ioctl()s: one that reads and one that
writes values to the selected lines.

- Handles can be requested as input/output, active low,
  open drain, open source, however when you issue a request
  for n lines with GPIO_GET_LINEHANDLE_IOCTL, they must all
  have the same flags, i.e. all inputs or all outputs, all
  open drain etc. If a granular control of the flags for
  each line is desired, they need to be requested
  individually, not in a batch.

- The GPIOHANDLE_GET_LINE_VALUES_IOCTL read ioctl() can be
  issued also to output lines to verify that the hardware
  is in the expected state.

- It reads and writes up to GPIOHANDLES_MAX lines at once,
  utilizing the .set_multiple() call in the driver if
  possible, making the call efficient if several lines
  can be written with a single register update.

The limitation of GPIOHANDLES_MAX to 64 lines is done under
the assumption that we may expect hardware that can issue a
transaction updating 64 bits at an instant but unlikely
anything larger than that.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Use gpiod_get_value_cansleep() so we support also slowpath
  GPIO drivers.
- Fix up the UAPI docs kerneldoc.
- Allocate the anonymous fd last, so that the release
  function don't get called until that point of something
  fails. After this point, skip the errorpath.
ChangeLog v1->v2:
- Handle ioctl_compat() properly based on a similar patch
  to the other ioctl() handling code.
- Use _IOWR() as we pass pointers both in and out of the
  ioctl()
- Use kmalloc() and kfree() for the linehandled, do not
  try to be fancy with devm_* it doesn't work the way I
  thought.
- Fix const-correctness on the linehandle name field.
---
 drivers/gpio/gpiolib.c    | 193 ++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/gpio.h |  61 ++++++++++++++-
 2 files changed, 251 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 68dbff5d8f57..8cfb06410ba7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -21,6 +21,7 @@
 #include <linux/fs.h>
 #include <linux/uaccess.h>
 #include <linux/compat.h>
+#include <linux/anon_inodes.h>
 #include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
@@ -310,6 +311,196 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 	return 0;
 }
 
+/*
+ * GPIO line handle management
+ */
+
+/**
+ * struct linehandle_state - contains the state of a userspace handle
+ * @gdev: the GPIO device the handle pertains to
+ * @label: consumer label used to tag descriptors
+ * @descs: the GPIO descriptors held by this handle
+ * @numdescs: the number of descriptors held in the descs array
+ */
+struct linehandle_state {
+	struct gpio_device *gdev;
+	const char *label;
+	struct gpio_desc *descs[GPIOHANDLES_MAX];
+	u32 numdescs;
+};
+
+static long linehandle_ioctl(struct file *filep, unsigned int cmd,
+			     unsigned long arg)
+{
+	struct linehandle_state *lh = filep->private_data;
+	void __user *ip = (void __user *)arg;
+	struct gpiohandle_data ghd;
+	int i;
+
+	if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
+		int val;
+
+		/* TODO: check if descriptors are really input */
+		for (i = 0; i < lh->numdescs; i++) {
+			val = gpiod_get_value_cansleep(lh->descs[i]);
+			if (val < 0)
+				return val;
+			ghd.values[i] = val;
+		}
+
+		if (copy_to_user(ip, &ghd, sizeof(ghd)))
+			return -EFAULT;
+
+		return 0;
+	} else if (cmd == GPIOHANDLE_SET_LINE_VALUES_IOCTL) {
+		int vals[GPIOHANDLES_MAX];
+
+		/* TODO: check if descriptors are really output */
+		if (copy_from_user(&ghd, ip, sizeof(ghd)))
+			return -EFAULT;
+
+		/* Clamp all values to [0,1] */
+		for (i = 0; i < lh->numdescs; i++)
+			vals[i] = !!ghd.values[i];
+
+		/* Reuse the array setting function */
+		gpiod_set_array_value_complex(false,
+					      true,
+					      lh->numdescs,
+					      lh->descs,
+					      vals);
+		return 0;
+	}
+	return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+static long linehandle_ioctl_compat(struct file *filep, unsigned int cmd,
+			     unsigned long arg)
+{
+	return linehandle_ioctl(filep, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static int linehandle_release(struct inode *inode, struct file *filep)
+{
+	struct linehandle_state *lh = filep->private_data;
+	struct gpio_device *gdev = lh->gdev;
+	int i;
+
+	for (i = 0; i < lh->numdescs; i++)
+		gpiod_free(lh->descs[i]);
+	kfree(lh->label);
+	kfree(lh);
+	put_device(&gdev->dev);
+	return 0;
+}
+
+static const struct file_operations linehandle_fileops = {
+	.release = linehandle_release,
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = linehandle_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = linehandle_ioctl_compat,
+#endif
+};
+
+static int linehandle_create(struct gpio_device *gdev, void __user *ip)
+{
+	struct gpiohandle_request handlereq;
+	struct linehandle_state *lh;
+	int fd, i, ret;
+
+	if (copy_from_user(&handlereq, ip, sizeof(handlereq)))
+		return -EFAULT;
+	if ((handlereq.lines == 0) || (handlereq.lines > GPIOHANDLES_MAX))
+		return -EINVAL;
+
+	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
+	if (!lh)
+		return -ENOMEM;
+	lh->gdev = gdev;
+	get_device(&gdev->dev);
+
+	/* Make sure this is terminated */
+	handlereq.consumer_label[sizeof(handlereq.consumer_label)-1] = '\0';
+	if (strlen(handlereq.consumer_label)) {
+		lh->label = kstrdup(handlereq.consumer_label,
+				    GFP_KERNEL);
+		if (!lh->label) {
+			ret = -ENOMEM;
+			goto out_free_lh;
+		}
+	}
+
+	/* Request each GPIO */
+	for (i = 0; i < handlereq.lines; i++) {
+		u32 offset = handlereq.lineoffsets[i];
+		u32 lflags = handlereq.flags;
+		struct gpio_desc *desc;
+
+		desc = &gdev->descs[offset];
+		ret = gpiod_request(desc, lh->label);
+		if (ret)
+			goto out_free_descs;
+		lh->descs[i] = desc;
+
+		if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
+			set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
+			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
+			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+
+		/*
+		 * Lines have to be requested explicitly for input
+		 * or output, else the line will be treated "as is".
+		 */
+		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
+			int val = !!handlereq.default_values[i];
+
+			ret = gpiod_direction_output(desc, val);
+			if (ret)
+				goto out_free_descs;
+		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
+			ret = gpiod_direction_input(desc);
+			if (ret)
+				goto out_free_descs;
+		}
+		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+			offset);
+	}
+	lh->numdescs = handlereq.lines;
+
+	fd = anon_inode_getfd("gpio-linehandle",
+			      &linehandle_fileops,
+			      lh,
+			      O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		ret = fd;
+		goto out_free_descs;
+	}
+
+	handlereq.fd = fd;
+	if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
+		return -EFAULT;
+
+	dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
+		lh->numdescs);
+
+	return 0;
+
+out_free_descs:
+	for (; i >= 0; i--)
+		gpiod_free(lh->descs[i]);
+	kfree(lh->label);
+out_free_lh:
+	kfree(lh);
+	put_device(&gdev->dev);
+	return ret;
+}
+
 /**
  * gpio_ioctl() - ioctl handler for the GPIO chardev
  */
@@ -385,6 +576,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
 		return 0;
+	} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
+		return linehandle_create(gdev, ip);
 	}
 	return -EINVAL;
 }
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index d0a3cac72250..21905531dcf4 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -1,7 +1,7 @@
 /*
  * <linux/gpio.h> - userspace ABI for the GPIO character devices
  *
- * Copyright (C) 2015 Linus Walleij
+ * Copyright (C) 2016 Linus Walleij
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published by
@@ -26,8 +26,8 @@ struct gpiochip_info {
 	__u32 lines;
 };
 
-/* Line is in use by the kernel */
-#define GPIOLINE_FLAG_KERNEL		(1UL << 0)
+/* Informational flags */
+#define GPIOLINE_FLAG_KERNEL		(1UL << 0) /* Line used by the kernel */
 #define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
 #define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
 #define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
@@ -52,7 +52,62 @@ struct gpioline_info {
 	char consumer[32];
 };
 
+/* Maximum number of requested handles */
+#define GPIOHANDLES_MAX 64
+
+/* Request flags */
+#define GPIOHANDLE_REQUEST_INPUT	(1UL << 0)
+#define GPIOHANDLE_REQUEST_OUTPUT	(1UL << 1)
+#define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
+#define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
+#define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
+
+/**
+ * struct gpiohandle_request - Information about a GPIO handle request
+ * @lineoffsets: an array desired lines, specified by offset index for the
+ * associated GPIO device
+ * @flags: desired flags for the desired GPIO lines, such as
+ * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
+ * together. Note that even if multiple lines are requested, the same flags
+ * must be applicable to all of them, if you want lines with individual
+ * flags set, request them one by one. It is possible to select
+ * a batch of input or output lines, but they must all have the same
+ * characteristics, i.e. all inputs or all outputs, all active low etc
+ * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set for a requested
+ * line, this specifies the default output value, should be 0 (low) or
+ * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
+ * @consumer_label: a desired consumer label for the selected GPIO line(s)
+ * such as "my-bitbanged-relay"
+ * @lines: number of lines requested in this request, i.e. the number of
+ * valid fields in the above arrays, set to 1 to request a single line
+ * @fd: if successful this field will contain a valid anonymous file handle
+ * after a GPIO_GET_LINEHANDLE_IOCTL operation, zero or negative value
+ * means error
+ */
+struct gpiohandle_request {
+	__u32 lineoffsets[GPIOHANDLES_MAX];
+	__u32 flags;
+	__u8 default_values[GPIOHANDLES_MAX];
+	char consumer_label[32];
+	__u32 lines;
+	int fd;
+};
+
 #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
 #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
+#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
+
+/**
+ * struct gpiohandle_data - Information of values on a GPIO handle
+ * @values: when getting the state of lines this contains the current
+ * state of a line, when setting the state of lines these should contain
+ * the desired target state
+ */
+struct gpiohandle_data {
+	__u8 values[GPIOHANDLES_MAX];
+};
+
+#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
+#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
 
 #endif /* _UAPI_GPIO_H_ */
-- 
2.4.11


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

* [PATCH 2/4] tools/gpio: add the gpio-hammer tool
  2016-06-02 11:51 [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Linus Walleij
@ 2016-06-02 11:51 ` Linus Walleij
  2016-06-02 11:51 ` [PATCH 3/4] gpio: userspace ABI for reading GPIO line events Linus Walleij
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2016-06-02 11:51 UTC (permalink / raw)
  To: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov
  Cc: Bamvor Jian Zhang, Grant Likely, Arnd Bergmann, Mark Brown,
	Johan Hovold, Linus Walleij

The gpio-hammer is used from userspace as an example of how
to retrieve a GPIO handle for one or several GPIO lines and
hammer the outputs from low to high and back again. It will
pulse the selected lines once per second for a specified
number of times or indefinitely if no loop count is
supplied.

Example output:
$ gpio-hammer -n gpiochip0 -o5 -o6 -o7
Hammer lines [5, 6, 7] on gpiochip0, initial states: [1, 1, 1]
[-] [5: 0, 6: 0, 7: 0]

Tested-by: Michael Welling <mwelling@ieee.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 tools/gpio/Makefile      |   5 +-
 tools/gpio/gpio-hammer.c | 189 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 192 insertions(+), 2 deletions(-)
 create mode 100644 tools/gpio/gpio-hammer.c

diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
index c155d6bc47a7..aea23949054e 100644
--- a/tools/gpio/Makefile
+++ b/tools/gpio/Makefile
@@ -1,12 +1,13 @@
 CC = $(CROSS_COMPILE)gcc
 CFLAGS += -O2 -Wall -g -D_GNU_SOURCE
 
-all: lsgpio
+all: lsgpio gpio-hammer
 
 lsgpio: lsgpio.o gpio-utils.o
+gpio-hammer: gpio-hammer.o gpio-utils.o
 
 %.o: %.c gpio-utils.h
 
 .PHONY: clean
 clean:
-	rm -f *.o lsgpio
+	rm -f *.o lsgpio gpio-hammer
diff --git a/tools/gpio/gpio-hammer.c b/tools/gpio/gpio-hammer.c
new file mode 100644
index 000000000000..37b3f141053d
--- /dev/null
+++ b/tools/gpio/gpio-hammer.c
@@ -0,0 +1,189 @@
+/*
+ * gpio-hammer - example swiss army knife to shake GPIO lines on a system
+ *
+ * Copyright (C) 2016 Linus Walleij
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * Usage:
+ *	gpio-hammer -n <device-name> -o <offset1> -o <offset2>
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <dirent.h>
+#include <errno.h>
+#include <string.h>
+#include <poll.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <sys/ioctl.h>
+#include <linux/gpio.h>
+
+int hammer_device(const char *device_name, unsigned int *lines, int nlines,
+		  unsigned int loops)
+{
+	struct gpiohandle_request req;
+	struct gpiohandle_data data;
+	char *chrdev_name;
+	char swirr[] = "-\\|/";
+	int fd;
+	int ret;
+	int i, j;
+	unsigned int iteration = 0;
+
+	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
+	if (ret < 0)
+		return -ENOMEM;
+
+	fd = open(chrdev_name, 0);
+	if (fd == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to open %s\n", chrdev_name);
+		goto exit_close_error;
+	}
+
+	/* Request lines as output */
+	for (i = 0; i < nlines; i++)
+		req.lineoffsets[i] = lines[i];
+	req.flags = GPIOHANDLE_REQUEST_OUTPUT; /* Request as output */
+	strcpy(req.consumer_label, "gpio-hammer");
+	req.lines = nlines;
+	ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
+	if (ret == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to issue GET LINEHANDLE "
+			"IOCTL (%d)\n",
+			ret);
+		goto exit_close_error;
+	}
+
+	/* Read initial states */
+	ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
+	if (ret == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to issue GPIOHANDLE GET LINE "
+			"VALUES IOCTL (%d)\n",
+			ret);
+		goto exit_close_error;
+	}
+	fprintf(stdout, "Hammer lines [");
+	for (i = 0; i < nlines; i++) {
+		fprintf(stdout, "%d", lines[i]);
+		if (i != (nlines - 1))
+			fprintf(stdout, ", ");
+	}
+	fprintf(stdout, "] on %s, initial states: [", device_name);
+	for (i = 0; i < nlines; i++) {
+		fprintf(stdout, "%d", data.values[i]);
+		if (i != (nlines - 1))
+			fprintf(stdout, ", ");
+	}
+	fprintf(stdout, "]\n");
+
+	/* Hammertime! */
+	j = 0;
+	while (1) {
+		/* Invert all lines so we blink */
+		for (i = 0; i < nlines; i++)
+			data.values[i] = !data.values[i];
+
+		ret = ioctl(req.fd, GPIOHANDLE_SET_LINE_VALUES_IOCTL, &data);
+		if (ret == -1) {
+			ret = -errno;
+			fprintf(stderr, "Failed to issue GPIOHANDLE SET LINE "
+				"VALUES IOCTL (%d)\n",
+				ret);
+			goto exit_close_error;
+		}
+		/* Re-read values to get status */
+		ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
+		if (ret == -1) {
+			ret = -errno;
+			fprintf(stderr, "Failed to issue GPIOHANDLE GET LINE "
+				"VALUES IOCTL (%d)\n",
+				ret);
+			goto exit_close_error;
+		}
+
+		fprintf(stdout, "[%c] ", swirr[j]);
+		j++;
+		if (j == sizeof(swirr)-1)
+			j = 0;
+
+		fprintf(stdout, "[");
+		for (i = 0; i < nlines; i++) {
+			fprintf(stdout, "%d: %d", lines[i], data.values[i]);
+			if (i != (nlines - 1))
+				fprintf(stdout, ", ");
+		}
+		fprintf(stdout, "]\r");
+		fflush(stdout);
+		sleep(1);
+		iteration++;
+		if (loops && iteration == loops)
+			break;
+	}
+	fprintf(stdout, "\n");
+	ret = 0;
+
+exit_close_error:
+	if (close(fd) == -1)
+		perror("Failed to close GPIO character device file");
+	free(chrdev_name);
+	return ret;
+}
+
+void print_usage(void)
+{
+	fprintf(stderr, "Usage: gpio-hammer [options]...\n"
+		"Hammer GPIO lines, 0->1->0->1...\n"
+		"  -n <name>  Hammer GPIOs on a named device (must be stated)\n"
+		"  -o <n>     Offset[s] to hammer, at least one, several can be stated\n"
+		" [-c <n>]    Do <n> loops (optional, infinite loop if not stated)\n"
+		"  -?         This helptext\n"
+		"\n"
+		"Example:\n"
+		"gpio-hammer -n gpiochip0 -o 4\n"
+	);
+}
+
+int main(int argc, char **argv)
+{
+	const char *device_name = NULL;
+	unsigned int lines[GPIOHANDLES_MAX];
+	unsigned int loops = 0;
+	int nlines;
+	int c;
+	int i;
+
+	i = 0;
+	while ((c = getopt(argc, argv, "c:n:o:?")) != -1) {
+		switch (c) {
+		case 'c':
+			loops = strtoul(optarg, NULL, 10);
+			break;
+		case 'n':
+			device_name = optarg;
+			break;
+		case 'o':
+			lines[i] = strtoul(optarg, NULL, 10);
+			i++;
+			break;
+		case '?':
+			print_usage();
+			return -1;
+		}
+	}
+	nlines = i;
+
+	if (!device_name || !nlines) {
+		print_usage();
+		return -1;
+	}
+	return hammer_device(device_name, lines, nlines, loops);
+}
-- 
2.4.11


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

* [PATCH 3/4] gpio: userspace ABI for reading GPIO line events
  2016-06-02 11:51 [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Linus Walleij
  2016-06-02 11:51 ` [PATCH 2/4] tools/gpio: add the gpio-hammer tool Linus Walleij
@ 2016-06-02 11:51 ` Linus Walleij
  2016-06-02 11:51 ` [PATCH 4/4] tools/gpio: add the gpio-event-mon tool Linus Walleij
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2016-06-02 11:51 UTC (permalink / raw)
  To: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov
  Cc: Bamvor Jian Zhang, Grant Likely, Arnd Bergmann, Mark Brown,
	Johan Hovold, Linus Walleij

This adds an ABI for listening to events on GPIO lines.
The mechanism returns an anonymous file handle to a request
to listen to a specific offset on a specific gpiochip.
To fetch the stream of events from the file handle, userspace
simply reads an event.

- Events can be requested with the same flags as ordinary
  handles, i.e. open drain or open source. An ioctl() call
  GPIO_GET_LINEEVENT_IOCTL is issued indicating the desired
  line.

- Events can be requested for falling edge events, rising
  edge events, or both.

- All events are timestamped using the kernel real time
  nanosecond timestamp (the same as is used by IIO).

- The supplied consumer label will appear in "lsgpio"
  listings of the lines, and in /proc/interrupts as the
  mechanism will request an interrupt from the gpio chip.

- Events are not supported on gpiochips that do not serve
  interrupts (no legal .to_irq() call). The event interrupt
  is threaded to avoid any realtime problems.

- It is possible to also directly read the current value
  of the registered GPIO line by issuing the same
  GPIOHANDLE_GET_LINE_VALUES_IOCTL as used by the
  line handles. Setting the value is not supported: we
  do not listen to events on output lines.

This ABI is strongly influenced by Industrial I/O and surpasses
the old sysfs ABI by providing proper precision timestamps,
making it possible to set flags like open drain, and put
consumer names on the GPIO lines.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c    | 298 ++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/gpio.h |  54 ++++++++-
 2 files changed, 347 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8cfb06410ba7..d034d1659fe3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -22,6 +22,9 @@
 #include <linux/uaccess.h>
 #include <linux/compat.h>
 #include <linux/anon_inodes.h>
+#include <linux/kfifo.h>
+#include <linux/poll.h>
+#include <linux/timekeeping.h>
 #include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
@@ -501,6 +504,299 @@ out_free_lh:
 	return ret;
 }
 
+/*
+ * GPIO line event management
+ */
+
+/**
+ * struct lineevent_state - contains the state of a userspace event
+ * @gdev: the GPIO device the event pertains to
+ * @label: consumer label used to tag descriptors
+ * @desc: the GPIO descriptor held by this event
+ * @eflags: the event flags this line was requested with
+ * @irq: the interrupt that trigger in response to events on this GPIO
+ * @wait: wait queue that handles blocking reads of events
+ * @events: KFIFO for the GPIO events
+ * @read_lock: mutex lock to protect reads from colliding with adding
+ * new events to the FIFO
+ */
+struct lineevent_state {
+	struct gpio_device *gdev;
+	const char *label;
+	struct gpio_desc *desc;
+	u32 eflags;
+	int irq;
+	wait_queue_head_t wait;
+	DECLARE_KFIFO(events, struct gpioevent_data, 16);
+	struct mutex read_lock;
+};
+
+static unsigned int lineevent_poll(struct file *filep,
+				   struct poll_table_struct *wait)
+{
+	struct lineevent_state *le = filep->private_data;
+	unsigned int events = 0;
+
+	poll_wait(filep, &le->wait, wait);
+
+	if (!kfifo_is_empty(&le->events))
+		events = POLLIN | POLLRDNORM;
+
+	return events;
+}
+
+
+static ssize_t lineevent_read(struct file *filep,
+			      char __user *buf,
+			      size_t count,
+			      loff_t *f_ps)
+{
+	struct lineevent_state *le = filep->private_data;
+	unsigned int copied;
+	int ret;
+
+	if (count < sizeof(struct gpioevent_data))
+		return -EINVAL;
+
+	do {
+		if (kfifo_is_empty(&le->events)) {
+			if (filep->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			ret = wait_event_interruptible(le->wait,
+					!kfifo_is_empty(&le->events));
+			if (ret)
+				return ret;
+		}
+
+		if (mutex_lock_interruptible(&le->read_lock))
+			return -ERESTARTSYS;
+		ret = kfifo_to_user(&le->events, buf, count, &copied);
+		mutex_unlock(&le->read_lock);
+
+		if (ret)
+			return ret;
+
+		/*
+		 * If we couldn't read anything from the fifo (a different
+		 * thread might have been faster) we either return -EAGAIN if
+		 * the file descriptor is non-blocking, otherwise we go back to
+		 * sleep and wait for more data to arrive.
+		 */
+		if (copied == 0 && (filep->f_flags & O_NONBLOCK))
+			return -EAGAIN;
+
+	} while (copied == 0);
+
+	return copied;
+}
+
+static int lineevent_release(struct inode *inode, struct file *filep)
+{
+	struct lineevent_state *le = filep->private_data;
+	struct gpio_device *gdev = le->gdev;
+
+	free_irq(le->irq, le);
+	gpiod_free(le->desc);
+	kfree(le->label);
+	kfree(le);
+	put_device(&gdev->dev);
+	return 0;
+}
+
+static long lineevent_ioctl(struct file *filep, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct lineevent_state *le = filep->private_data;
+	void __user *ip = (void __user *)arg;
+	struct gpiohandle_data ghd;
+
+	/*
+	 * We can get the value for an event line but not set it,
+	 * because it is input by definition.
+	 */
+	if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
+		int val;
+
+		val = gpiod_get_value_cansleep(le->desc);
+		if (val < 0)
+			return val;
+		ghd.values[0] = val;
+
+		if (copy_to_user(ip, &ghd, sizeof(ghd)))
+			return -EFAULT;
+
+		return 0;
+	}
+	return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+static long lineevent_ioctl_compat(struct file *filep, unsigned int cmd,
+				   unsigned long arg)
+{
+	return lineevent_ioctl(filep, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static const struct file_operations lineevent_fileops = {
+	.release = lineevent_release,
+	.read = lineevent_read,
+	.poll = lineevent_poll,
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = lineevent_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = lineevent_ioctl_compat,
+#endif
+};
+
+irqreturn_t lineevent_irq_thread(int irq, void *p)
+{
+	struct lineevent_state *le = p;
+	struct gpioevent_data ge;
+	int ret;
+
+	ge.timestamp = ktime_get_real_ns();
+
+	if (le->eflags & GPIOEVENT_REQUEST_BOTH_EDGES) {
+		int level = gpiod_get_value_cansleep(le->desc);
+
+		if (level)
+			/* Emit low-to-high event */
+			ge.id = GPIOEVENT_EVENT_RISING_EDGE;
+		else
+			/* Emit high-to-low event */
+			ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
+	} else if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE) {
+		/* Emit low-to-high event */
+		ge.id = GPIOEVENT_EVENT_RISING_EDGE;
+	} else if (le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
+		/* Emit high-to-low event */
+		ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
+	}
+
+	ret = kfifo_put(&le->events, ge);
+	if (ret != 0)
+		wake_up_poll(&le->wait, POLLIN);
+
+	return IRQ_HANDLED;
+}
+
+static int lineevent_create(struct gpio_device *gdev, void __user *ip)
+{
+	struct gpioevent_request eventreq;
+	struct lineevent_state *le;
+	struct gpio_desc *desc;
+	u32 offset;
+	u32 lflags;
+	u32 eflags;
+	int fd;
+	int ret;
+	int irqflags = 0;
+
+	if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
+		return -EFAULT;
+
+	le = kzalloc(sizeof(*le), GFP_KERNEL);
+	if (!le)
+		return -ENOMEM;
+	le->gdev = gdev;
+	get_device(&gdev->dev);
+
+	/* Make sure this is terminated */
+	eventreq.consumer_label[sizeof(eventreq.consumer_label)-1] = '\0';
+	if (strlen(eventreq.consumer_label)) {
+		le->label = kstrdup(eventreq.consumer_label,
+				    GFP_KERNEL);
+		if (!le->label) {
+			ret = -ENOMEM;
+			goto out_free_le;
+		}
+	}
+
+	offset = eventreq.lineoffset;
+	lflags = eventreq.handleflags;
+	eflags = eventreq.eventflags;
+
+	/* This is just wrong: we don't look for events on output lines */
+	if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
+		ret = -EINVAL;
+		goto out_free_label;
+	}
+
+	desc = &gdev->descs[offset];
+	ret = gpiod_request(desc, le->label);
+	if (ret)
+		goto out_free_desc;
+	le->desc = desc;
+	le->eflags = eflags;
+
+	if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
+		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
+		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+	if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
+		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+
+	ret = gpiod_direction_input(desc);
+	if (ret)
+		goto out_free_desc;
+
+	le->irq = gpiod_to_irq(desc);
+	if (le->irq <= 0) {
+		ret = -ENODEV;
+		goto out_free_desc;
+	}
+
+	if (eflags & GPIOEVENT_REQUEST_RISING_EDGE)
+		irqflags |= IRQF_TRIGGER_RISING;
+	if (eflags & GPIOEVENT_REQUEST_FALLING_EDGE)
+		irqflags |= IRQF_TRIGGER_FALLING;
+	irqflags |= IRQF_ONESHOT;
+	irqflags |= IRQF_SHARED;
+
+	INIT_KFIFO(le->events);
+	init_waitqueue_head(&le->wait);
+	mutex_init(&le->read_lock);
+
+	/* Request a thread to read the events */
+	ret = request_threaded_irq(le->irq,
+			NULL,
+			lineevent_irq_thread,
+			irqflags,
+			le->label,
+			le);
+	if (ret)
+		goto out_free_desc;
+
+	fd = anon_inode_getfd("gpio-event",
+			      &lineevent_fileops,
+			      le,
+			      O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		ret = fd;
+		goto out_free_irq;
+	}
+
+	eventreq.fd = fd;
+	if (copy_to_user(ip, &eventreq, sizeof(eventreq)))
+		return -EFAULT;
+
+	return 0;
+
+out_free_irq:
+	free_irq(le->irq, le);
+out_free_desc:
+	gpiod_free(le->desc);
+out_free_label:
+	kfree(le->label);
+out_free_le:
+	kfree(le);
+	put_device(&gdev->dev);
+	return ret;
+}
+
 /**
  * gpio_ioctl() - ioctl handler for the GPIO chardev
  */
@@ -578,6 +874,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return 0;
 	} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
 		return linehandle_create(gdev, ip);
+	} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
+		return lineevent_create(gdev, ip);
 	}
 	return -EINVAL;
 }
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 21905531dcf4..333d3544c964 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -55,7 +55,7 @@ struct gpioline_info {
 /* Maximum number of requested handles */
 #define GPIOHANDLES_MAX 64
 
-/* Request flags */
+/* Linerequest flags */
 #define GPIOHANDLE_REQUEST_INPUT	(1UL << 0)
 #define GPIOHANDLE_REQUEST_OUTPUT	(1UL << 1)
 #define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
@@ -93,10 +93,6 @@ struct gpiohandle_request {
 	int fd;
 };
 
-#define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
-#define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
-#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
-
 /**
  * struct gpiohandle_data - Information of values on a GPIO handle
  * @values: when getting the state of lines this contains the current
@@ -110,4 +106,52 @@ struct gpiohandle_data {
 #define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
 #define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
 
+/* Eventrequest flags */
+#define GPIOEVENT_REQUEST_RISING_EDGE	(1UL << 0)
+#define GPIOEVENT_REQUEST_FALLING_EDGE	(1UL << 1)
+#define GPIOEVENT_REQUEST_BOTH_EDGES	((1UL << 0) | (1UL << 1))
+
+/**
+ * struct gpioevent_request - Information about a GPIO event request
+ * @lineoffset: the desired line to subscribe to events from, specified by
+ * offset index for the associated GPIO device
+ * @handleflags: desired handle flags for the desired GPIO line, such as
+ * GPIOHANDLE_REQUEST_ACTIVE_LOW or GPIOHANDLE_REQUEST_OPEN_DRAIN
+ * @eventflags: desired flags for the desired GPIO event line, such as
+ * GPIOEVENT_REQUEST_RISING_EDGE or GPIOEVENT_REQUEST_FALLING_EDGE
+ * @consumer_label: a desired consumer label for the selected GPIO line(s)
+ * such as "my-listener"
+ * @fd: if successful this field will contain a valid anonymous file handle
+ * after a GPIO_GET_LINEEVENT_IOCTL operation, zero or negative value
+ * means error
+ */
+struct gpioevent_request {
+	__u32 lineoffset;
+	__u32 handleflags;
+	__u32 eventflags;
+	char consumer_label[32];
+	int fd;
+};
+
+/**
+ * GPIO event types
+ */
+#define GPIOEVENT_EVENT_RISING_EDGE 0x01
+#define GPIOEVENT_EVENT_FALLING_EDGE 0x02
+
+/**
+ * struct gpioevent_data - The actual event being pushed to userspace
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @id: event identifier
+ */
+struct gpioevent_data {
+	__u64 timestamp;
+	__u32 id;
+};
+
+#define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
+#define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
+#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
+#define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
+
 #endif /* _UAPI_GPIO_H_ */
-- 
2.4.11


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

* [PATCH 4/4] tools/gpio: add the gpio-event-mon tool
  2016-06-02 11:51 [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Linus Walleij
  2016-06-02 11:51 ` [PATCH 2/4] tools/gpio: add the gpio-hammer tool Linus Walleij
  2016-06-02 11:51 ` [PATCH 3/4] gpio: userspace ABI for reading GPIO line events Linus Walleij
@ 2016-06-02 11:51 ` Linus Walleij
  2016-06-02 18:12 ` [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Michael Welling
  2016-06-29  8:53 ` Alexander Stein
  4 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2016-06-02 11:51 UTC (permalink / raw)
  To: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov
  Cc: Bamvor Jian Zhang, Grant Likely, Arnd Bergmann, Mark Brown,
	Johan Hovold, Linus Walleij

The gpio-event-mon is used from userspace as an example of how
to monitor GPIO line events. It will latch on to a certain
GPIO line on a certain gpiochip and print timestamped events
as they arrive.

Example output:
$ gpio-event-mon -n gpiochip2 -o 0 -r -f
Monitoring line 0 on gpiochip2
Initial line value: 1
GPIO EVENT 946685798487609863: falling edge
GPIO EVENT 946685798732482910: rising edge
GPIO EVENT 946685799115997314: falling edge
GPIO EVENT 946685799381469726: rising edge

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 tools/gpio/Makefile         |   5 +-
 tools/gpio/gpio-event-mon.c | 192 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 195 insertions(+), 2 deletions(-)
 create mode 100644 tools/gpio/gpio-event-mon.c

diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
index aea23949054e..619314ff9bd6 100644
--- a/tools/gpio/Makefile
+++ b/tools/gpio/Makefile
@@ -1,13 +1,14 @@
 CC = $(CROSS_COMPILE)gcc
 CFLAGS += -O2 -Wall -g -D_GNU_SOURCE
 
-all: lsgpio gpio-hammer
+all: lsgpio gpio-hammer gpio-event-mon
 
 lsgpio: lsgpio.o gpio-utils.o
 gpio-hammer: gpio-hammer.o gpio-utils.o
+gpio-event-mon: gpio-event-mon.o gpio-utils.o
 
 %.o: %.c gpio-utils.h
 
 .PHONY: clean
 clean:
-	rm -f *.o lsgpio gpio-hammer
+	rm -f *.o lsgpio gpio-hammer gpio-event-mon
diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
new file mode 100644
index 000000000000..448ed96b3b4f
--- /dev/null
+++ b/tools/gpio/gpio-event-mon.c
@@ -0,0 +1,192 @@
+/*
+ * gpio-hammer - example swiss army knife to shake GPIO lines on a system
+ *
+ * Copyright (C) 2016 Linus Walleij
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * Usage:
+ *	gpio-event-mon -n <device-name> -o <offset>
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <dirent.h>
+#include <errno.h>
+#include <string.h>
+#include <poll.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <inttypes.h>
+#include <sys/ioctl.h>
+#include <linux/gpio.h>
+
+int monitor_device(const char *device_name,
+		   unsigned int line,
+		   u_int32_t handleflags,
+		   u_int32_t eventflags,
+		   unsigned int loops)
+{
+	struct gpioevent_request req;
+	struct gpiohandle_data data;
+	char *chrdev_name;
+	int fd;
+	int ret;
+	int i = 0;
+
+	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
+	if (ret < 0)
+		return -ENOMEM;
+
+	fd = open(chrdev_name, 0);
+	if (fd == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to open %s\n", chrdev_name);
+		goto exit_close_error;
+	}
+
+	req.lineoffset = line;
+	req.handleflags = handleflags;
+	req.eventflags = eventflags;
+	strcpy(req.consumer_label, "gpio-event-mon");
+
+	ret = ioctl(fd, GPIO_GET_LINEEVENT_IOCTL, &req);
+	if (ret == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to issue GET EVENT "
+			"IOCTL (%d)\n",
+			ret);
+		goto exit_close_error;
+	}
+
+	/* Read initial states */
+	ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
+	if (ret == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to issue GPIOHANDLE GET LINE "
+			"VALUES IOCTL (%d)\n",
+			ret);
+		goto exit_close_error;
+	}
+
+	fprintf(stdout, "Monitoring line %d on %s\n", line, device_name);
+	fprintf(stdout, "Initial line value: %d\n", data.values[0]);
+
+	while (1) {
+		struct gpioevent_data event;
+
+		ret = read(req.fd, &event, sizeof(event));
+		if (ret == -1) {
+			if (errno == -EAGAIN) {
+				fprintf(stderr, "nothing available\n");
+				continue;
+			} else {
+				ret = -errno;
+				fprintf(stderr, "Failed to read event (%d)\n",
+					ret);
+				break;
+			}
+		}
+
+		if (ret != sizeof(event)) {
+			fprintf(stderr, "Reading event failed\n");
+			ret = -EIO;
+			break;
+		}
+		fprintf(stdout, "GPIO EVENT %" PRIu64 ": ", event.timestamp);
+		switch (event.id) {
+		case GPIOEVENT_EVENT_RISING_EDGE:
+			fprintf(stdout, "rising edge");
+			break;
+		case GPIOEVENT_EVENT_FALLING_EDGE:
+			fprintf(stdout, "falling edge");
+			break;
+		default:
+			fprintf(stdout, "unknown event");
+		}
+		fprintf(stdout, "\n");
+
+		i++;
+		if (i == loops)
+			break;
+	}
+
+exit_close_error:
+	if (close(fd) == -1)
+		perror("Failed to close GPIO character device file");
+	free(chrdev_name);
+	return ret;
+}
+
+void print_usage(void)
+{
+	fprintf(stderr, "Usage: gpio-event-mon [options]...\n"
+		"Listen to events on GPIO lines, 0->1 1->0\n"
+		"  -n <name>  Listen on GPIOs on a named device (must be stated)\n"
+		"  -o <n>     Offset to monitor\n"
+		"  -d         Set line as open drain\n"
+		"  -s         Set line as open source\n"
+		"  -r         Listen for rising edges\n"
+		"  -f         Listen for falling edges\n"
+		" [-c <n>]    Do <n> loops (optional, infinite loop if not stated)\n"
+		"  -?         This helptext\n"
+		"\n"
+		"Example:\n"
+		"gpio-event-mon -n gpiochip0 -o 4 -r -f\n"
+	);
+}
+
+int main(int argc, char **argv)
+{
+	const char *device_name = NULL;
+	unsigned int line = -1;
+	unsigned int loops = 0;
+	u_int32_t handleflags = GPIOHANDLE_REQUEST_INPUT;
+	u_int32_t eventflags = 0;
+	int c;
+
+	while ((c = getopt(argc, argv, "c:n:o:dsrf?")) != -1) {
+		switch (c) {
+		case 'c':
+			loops = strtoul(optarg, NULL, 10);
+			break;
+		case 'n':
+			device_name = optarg;
+			break;
+		case 'o':
+			line = strtoul(optarg, NULL, 10);
+			break;
+		case 'd':
+			handleflags |= GPIOHANDLE_REQUEST_OPEN_DRAIN;
+			break;
+		case 's':
+			handleflags |= GPIOHANDLE_REQUEST_OPEN_SOURCE;
+			break;
+		case 'r':
+			eventflags |= GPIOEVENT_REQUEST_RISING_EDGE;
+			break;
+		case 'f':
+			eventflags |= GPIOEVENT_REQUEST_FALLING_EDGE;
+			break;
+		case '?':
+			print_usage();
+			return -1;
+		}
+	}
+
+	if (!device_name || line == -1) {
+		print_usage();
+		return -1;
+	}
+	if (!eventflags) {
+		printf("No flags specified, listening on both rising and "
+		       "falling edges\n");
+		eventflags = GPIOEVENT_REQUEST_BOTH_EDGES;
+	}
+	return monitor_device(device_name, line, handleflags,
+			      eventflags, loops);
+}
-- 
2.4.11


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

* Re: [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines
  2016-06-02 11:51 [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Linus Walleij
                   ` (2 preceding siblings ...)
  2016-06-02 11:51 ` [PATCH 4/4] tools/gpio: add the gpio-event-mon tool Linus Walleij
@ 2016-06-02 18:12 ` Michael Welling
  2016-06-29  8:53 ` Alexander Stein
  4 siblings, 0 replies; 9+ messages in thread
From: Michael Welling @ 2016-06-02 18:12 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-gpio, Alexandre Courbot, Markus Pargmann, Lee Campbell,
	Dmitry Torokhov, Bamvor Jian Zhang, Grant Likely, Arnd Bergmann,
	Mark Brown, Johan Hovold

On Thu, Jun 02, 2016 at 01:51:26PM +0200, Linus Walleij wrote:
> This adds a userspace ABI for reading and writing GPIO lines.
> The mechanism returns an anonymous file handle to a request
> to read/write n offsets from a gpiochip. This file handle
> in turn accepts two ioctl()s: one that reads and one that
> writes values to the selected lines.
> 
> - Handles can be requested as input/output, active low,
>   open drain, open source, however when you issue a request
>   for n lines with GPIO_GET_LINEHANDLE_IOCTL, they must all
>   have the same flags, i.e. all inputs or all outputs, all
>   open drain etc. If a granular control of the flags for
>   each line is desired, they need to be requested
>   individually, not in a batch.
> 
> - The GPIOHANDLE_GET_LINE_VALUES_IOCTL read ioctl() can be
>   issued also to output lines to verify that the hardware
>   is in the expected state.
> 
> - It reads and writes up to GPIOHANDLES_MAX lines at once,
>   utilizing the .set_multiple() call in the driver if
>   possible, making the call efficient if several lines
>   can be written with a single register update.
> 
> The limitation of GPIOHANDLES_MAX to 64 lines is done under
> the assumption that we may expect hardware that can issue a
> transaction updating 64 bits at an instant but unlikely
> anything larger than that.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

The SPI based gpio controllers do not invoke a warning anymore when using this
interface.

Acked-by: Michael Welling <mwelling@ieee.org>

> ---
> ChangeLog v2->v3:
> - Use gpiod_get_value_cansleep() so we support also slowpath
>   GPIO drivers.
> - Fix up the UAPI docs kerneldoc.
> - Allocate the anonymous fd last, so that the release
>   function don't get called until that point of something
>   fails. After this point, skip the errorpath.
> ChangeLog v1->v2:
> - Handle ioctl_compat() properly based on a similar patch
>   to the other ioctl() handling code.
> - Use _IOWR() as we pass pointers both in and out of the
>   ioctl()
> - Use kmalloc() and kfree() for the linehandled, do not
>   try to be fancy with devm_* it doesn't work the way I
>   thought.
> - Fix const-correctness on the linehandle name field.
> ---
>  drivers/gpio/gpiolib.c    | 193 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/gpio.h |  61 ++++++++++++++-
>  2 files changed, 251 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 68dbff5d8f57..8cfb06410ba7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -21,6 +21,7 @@
>  #include <linux/fs.h>
>  #include <linux/uaccess.h>
>  #include <linux/compat.h>
> +#include <linux/anon_inodes.h>
>  #include <uapi/linux/gpio.h>
>  
>  #include "gpiolib.h"
> @@ -310,6 +311,196 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
>  	return 0;
>  }
>  
> +/*
> + * GPIO line handle management
> + */
> +
> +/**
> + * struct linehandle_state - contains the state of a userspace handle
> + * @gdev: the GPIO device the handle pertains to
> + * @label: consumer label used to tag descriptors
> + * @descs: the GPIO descriptors held by this handle
> + * @numdescs: the number of descriptors held in the descs array
> + */
> +struct linehandle_state {
> +	struct gpio_device *gdev;
> +	const char *label;
> +	struct gpio_desc *descs[GPIOHANDLES_MAX];
> +	u32 numdescs;
> +};
> +
> +static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	struct linehandle_state *lh = filep->private_data;
> +	void __user *ip = (void __user *)arg;
> +	struct gpiohandle_data ghd;
> +	int i;
> +
> +	if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
> +		int val;
> +
> +		/* TODO: check if descriptors are really input */
> +		for (i = 0; i < lh->numdescs; i++) {
> +			val = gpiod_get_value_cansleep(lh->descs[i]);
> +			if (val < 0)
> +				return val;
> +			ghd.values[i] = val;
> +		}
> +
> +		if (copy_to_user(ip, &ghd, sizeof(ghd)))
> +			return -EFAULT;
> +
> +		return 0;
> +	} else if (cmd == GPIOHANDLE_SET_LINE_VALUES_IOCTL) {
> +		int vals[GPIOHANDLES_MAX];
> +
> +		/* TODO: check if descriptors are really output */
> +		if (copy_from_user(&ghd, ip, sizeof(ghd)))
> +			return -EFAULT;
> +
> +		/* Clamp all values to [0,1] */
> +		for (i = 0; i < lh->numdescs; i++)
> +			vals[i] = !!ghd.values[i];
> +
> +		/* Reuse the array setting function */
> +		gpiod_set_array_value_complex(false,
> +					      true,
> +					      lh->numdescs,
> +					      lh->descs,
> +					      vals);
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long linehandle_ioctl_compat(struct file *filep, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	return linehandle_ioctl(filep, cmd, (unsigned long)compat_ptr(arg));
> +}
> +#endif
> +
> +static int linehandle_release(struct inode *inode, struct file *filep)
> +{
> +	struct linehandle_state *lh = filep->private_data;
> +	struct gpio_device *gdev = lh->gdev;
> +	int i;
> +
> +	for (i = 0; i < lh->numdescs; i++)
> +		gpiod_free(lh->descs[i]);
> +	kfree(lh->label);
> +	kfree(lh);
> +	put_device(&gdev->dev);
> +	return 0;
> +}
> +
> +static const struct file_operations linehandle_fileops = {
> +	.release = linehandle_release,
> +	.owner = THIS_MODULE,
> +	.llseek = noop_llseek,
> +	.unlocked_ioctl = linehandle_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = linehandle_ioctl_compat,
> +#endif
> +};
> +
> +static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> +{
> +	struct gpiohandle_request handlereq;
> +	struct linehandle_state *lh;
> +	int fd, i, ret;
> +
> +	if (copy_from_user(&handlereq, ip, sizeof(handlereq)))
> +		return -EFAULT;
> +	if ((handlereq.lines == 0) || (handlereq.lines > GPIOHANDLES_MAX))
> +		return -EINVAL;
> +
> +	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
> +	if (!lh)
> +		return -ENOMEM;
> +	lh->gdev = gdev;
> +	get_device(&gdev->dev);
> +
> +	/* Make sure this is terminated */
> +	handlereq.consumer_label[sizeof(handlereq.consumer_label)-1] = '\0';
> +	if (strlen(handlereq.consumer_label)) {
> +		lh->label = kstrdup(handlereq.consumer_label,
> +				    GFP_KERNEL);
> +		if (!lh->label) {
> +			ret = -ENOMEM;
> +			goto out_free_lh;
> +		}
> +	}
> +
> +	/* Request each GPIO */
> +	for (i = 0; i < handlereq.lines; i++) {
> +		u32 offset = handlereq.lineoffsets[i];
> +		u32 lflags = handlereq.flags;
> +		struct gpio_desc *desc;
> +
> +		desc = &gdev->descs[offset];
> +		ret = gpiod_request(desc, lh->label);
> +		if (ret)
> +			goto out_free_descs;
> +		lh->descs[i] = desc;
> +
> +		if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
> +			set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +		if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
> +			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> +		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
> +			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +
> +		/*
> +		 * Lines have to be requested explicitly for input
> +		 * or output, else the line will be treated "as is".
> +		 */
> +		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
> +			int val = !!handlereq.default_values[i];
> +
> +			ret = gpiod_direction_output(desc, val);
> +			if (ret)
> +				goto out_free_descs;
> +		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
> +			ret = gpiod_direction_input(desc);
> +			if (ret)
> +				goto out_free_descs;
> +		}
> +		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> +			offset);
> +	}
> +	lh->numdescs = handlereq.lines;
> +
> +	fd = anon_inode_getfd("gpio-linehandle",
> +			      &linehandle_fileops,
> +			      lh,
> +			      O_RDONLY | O_CLOEXEC);
> +	if (fd < 0) {
> +		ret = fd;
> +		goto out_free_descs;
> +	}
> +
> +	handlereq.fd = fd;
> +	if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
> +		return -EFAULT;
> +
> +	dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
> +		lh->numdescs);
> +
> +	return 0;
> +
> +out_free_descs:
> +	for (; i >= 0; i--)
> +		gpiod_free(lh->descs[i]);
> +	kfree(lh->label);
> +out_free_lh:
> +	kfree(lh);
> +	put_device(&gdev->dev);
> +	return ret;
> +}
> +
>  /**
>   * gpio_ioctl() - ioctl handler for the GPIO chardev
>   */
> @@ -385,6 +576,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
>  			return -EFAULT;
>  		return 0;
> +	} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
> +		return linehandle_create(gdev, ip);
>  	}
>  	return -EINVAL;
>  }
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index d0a3cac72250..21905531dcf4 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -1,7 +1,7 @@
>  /*
>   * <linux/gpio.h> - userspace ABI for the GPIO character devices
>   *
> - * Copyright (C) 2015 Linus Walleij
> + * Copyright (C) 2016 Linus Walleij
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License version 2 as published by
> @@ -26,8 +26,8 @@ struct gpiochip_info {
>  	__u32 lines;
>  };
>  
> -/* Line is in use by the kernel */
> -#define GPIOLINE_FLAG_KERNEL		(1UL << 0)
> +/* Informational flags */
> +#define GPIOLINE_FLAG_KERNEL		(1UL << 0) /* Line used by the kernel */
>  #define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
>  #define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
>  #define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
> @@ -52,7 +52,62 @@ struct gpioline_info {
>  	char consumer[32];
>  };
>  
> +/* Maximum number of requested handles */
> +#define GPIOHANDLES_MAX 64
> +
> +/* Request flags */
> +#define GPIOHANDLE_REQUEST_INPUT	(1UL << 0)
> +#define GPIOHANDLE_REQUEST_OUTPUT	(1UL << 1)
> +#define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
> +#define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
> +#define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
> +
> +/**
> + * struct gpiohandle_request - Information about a GPIO handle request
> + * @lineoffsets: an array desired lines, specified by offset index for the
> + * associated GPIO device
> + * @flags: desired flags for the desired GPIO lines, such as
> + * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
> + * together. Note that even if multiple lines are requested, the same flags
> + * must be applicable to all of them, if you want lines with individual
> + * flags set, request them one by one. It is possible to select
> + * a batch of input or output lines, but they must all have the same
> + * characteristics, i.e. all inputs or all outputs, all active low etc
> + * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set for a requested
> + * line, this specifies the default output value, should be 0 (low) or
> + * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
> + * @consumer_label: a desired consumer label for the selected GPIO line(s)
> + * such as "my-bitbanged-relay"
> + * @lines: number of lines requested in this request, i.e. the number of
> + * valid fields in the above arrays, set to 1 to request a single line
> + * @fd: if successful this field will contain a valid anonymous file handle
> + * after a GPIO_GET_LINEHANDLE_IOCTL operation, zero or negative value
> + * means error
> + */
> +struct gpiohandle_request {
> +	__u32 lineoffsets[GPIOHANDLES_MAX];
> +	__u32 flags;
> +	__u8 default_values[GPIOHANDLES_MAX];
> +	char consumer_label[32];
> +	__u32 lines;
> +	int fd;
> +};
> +
>  #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
>  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
> +#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> +
> +/**
> + * struct gpiohandle_data - Information of values on a GPIO handle
> + * @values: when getting the state of lines this contains the current
> + * state of a line, when setting the state of lines these should contain
> + * the desired target state
> + */
> +struct gpiohandle_data {
> +	__u8 values[GPIOHANDLES_MAX];
> +};
> +
> +#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
> +#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
>  
>  #endif /* _UAPI_GPIO_H_ */
> -- 
> 2.4.11
> 

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

* Re: [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines
  2016-06-02 11:51 [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Linus Walleij
                   ` (3 preceding siblings ...)
  2016-06-02 18:12 ` [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Michael Welling
@ 2016-06-29  8:53 ` Alexander Stein
  2016-06-29  9:05   ` Linus Walleij
  4 siblings, 1 reply; 9+ messages in thread
From: Alexander Stein @ 2016-06-29  8:53 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov, Bamvor Jian Zhang, Grant Likely,
	Arnd Bergmann, Mark Brown, Johan Hovold

On Thursday 02 June 2016 13:51:26, Linus Walleij wrote:
> This adds a userspace ABI for reading and writing GPIO lines.
> The mechanism returns an anonymous file handle to a request
> to read/write n offsets from a gpiochip. This file handle
> in turn accepts two ioctl()s: one that reads and one that
> writes values to the selected lines.
> [...]
> +static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> +{
> +	struct gpiohandle_request handlereq;
> +	struct linehandle_state *lh;
> +	int fd, i, ret;
> +
> +	if (copy_from_user(&handlereq, ip, sizeof(handlereq)))
> +		return -EFAULT;
> +	if ((handlereq.lines == 0) || (handlereq.lines > GPIOHANDLES_MAX))
> +		return -EINVAL;
> +
> +	lh = kzalloc(sizeof(*lh), GFP_KERNEL);
> +	if (!lh)
> +		return -ENOMEM;
> +	lh->gdev = gdev;
> +	get_device(&gdev->dev);
> +
> +	/* Make sure this is terminated */
> +	handlereq.consumer_label[sizeof(handlereq.consumer_label)-1] = '\0';
> +	if (strlen(handlereq.consumer_label)) {
> +		lh->label = kstrdup(handlereq.consumer_label,
> +				    GFP_KERNEL);
> +		if (!lh->label) {
> +			ret = -ENOMEM;
> +			goto out_free_lh;
> +		}
> +	}
> +
> +	/* Request each GPIO */
> +	for (i = 0; i < handlereq.lines; i++) {
> +		u32 offset = handlereq.lineoffsets[i];
> +		u32 lflags = handlereq.flags;
> +		struct gpio_desc *desc;
> +
> +		desc = &gdev->descs[offset];
> +		ret = gpiod_request(desc, lh->label);
> +		if (ret)
> +			goto out_free_descs;
> +		lh->descs[i] = desc;
> +
> +		if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
> +			set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +		if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
> +			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> +		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
> +			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +
> +		/*
> +		 * Lines have to be requested explicitly for input
> +		 * or output, else the line will be treated "as is".
> +		 */
> +		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
> +			int val = !!handlereq.default_values[i];
> +
> +			ret = gpiod_direction_output(desc, val);
> +			if (ret)
> +				goto out_free_descs;
> +		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
> +			ret = gpiod_direction_input(desc);
> +			if (ret)
> +				goto out_free_descs;
> +		}
> +		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> +			offset);
> +	}
> +	lh->numdescs = handlereq.lines;
> +
> +	fd = anon_inode_getfd("gpio-linehandle",
> +			      &linehandle_fileops,
> +			      lh,
> +			      O_RDONLY | O_CLOEXEC);

When will linehandle_release actually be called? When we explicitely call 
close(req.fd) or when application exits and all FDs will be closed anyway?

> +	if (fd < 0) {
> +		ret = fd;
> +		goto out_free_descs;
> +	}
> +
> +	handlereq.fd = fd;
> +	if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
> +		return -EFAULT;

If I'm right above doesn't that then leak lh until application eventually 
exits? Userspace won't receive the newly created fd. The same would apply to 
patch 3/4.

Best regards,
Alexander


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

* Re: [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines
  2016-06-29  8:53 ` Alexander Stein
@ 2016-06-29  9:05   ` Linus Walleij
  2016-06-29  9:43     ` Alexander Stein
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2016-06-29  9:05 UTC (permalink / raw)
  To: Alexander Stein
  Cc: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov, Bamvor Jian Zhang, Grant Likely,
	Arnd Bergmann, Mark Brown, Johan Hovold

On Wed, Jun 29, 2016 at 10:53 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> On Thursday 02 June 2016 13:51:26, Linus Walleij wrote:

>> +     fd = anon_inode_getfd("gpio-linehandle",
>> +                           &linehandle_fileops,
>> +                           lh,
>> +                           O_RDONLY | O_CLOEXEC);
>
> When will linehandle_release actually be called? When we explicitely call
> close(req.fd) or when application exits and all FDs will be closed anyway?

As far as I understand: both.

And I don't think anything else makes sense? If an application is terminated,
the operating environment will close all open file handles as far as I know,
at least that is how I always thought it works.

But hey, it's userspace so what do I know...

Anyways if it doesn't work like so, there are a bunch of subsystems in
the kernel using it so they would all have severe problems.

>> +     if (fd < 0) {
>> +             ret = fd;
>> +             goto out_free_descs;
>> +     }
>> +
>> +     handlereq.fd = fd;
>> +     if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
>> +             return -EFAULT;
>
> If I'm right above doesn't that then leak lh until application eventually
> exits? Userspace won't receive the newly created fd. The same would apply to
> patch 3/4.

I don't understand....

The userspace application reads the .fd fields of the handle request after
issuing the ioctl() and it then contains the new file handle.

Yours,
Linus Walleij

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

* Re: [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines
  2016-06-29  9:05   ` Linus Walleij
@ 2016-06-29  9:43     ` Alexander Stein
  2016-07-04  9:55       ` Linus Walleij
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Stein @ 2016-06-29  9:43 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov, Bamvor Jian Zhang, Grant Likely,
	Arnd Bergmann, Mark Brown, Johan Hovold

On Wednesday 29 June 2016 11:05:49, Linus Walleij wrote:
> On Wed, Jun 29, 2016 at 10:53 AM, Alexander Stein
> 
> <alexander.stein@systec-electronic.com> wrote:
> > On Thursday 02 June 2016 13:51:26, Linus Walleij wrote:
> >> +     fd = anon_inode_getfd("gpio-linehandle",
> >> +                           &linehandle_fileops,
> >> +                           lh,
> >> +                           O_RDONLY | O_CLOEXEC);
> > 
> > When will linehandle_release actually be called? When we explicitely call
> > close(req.fd) or when application exits and all FDs will be closed anyway?
> 
> As far as I understand: both.
> 
> And I don't think anything else makes sense? If an application is
> terminated, the operating environment will close all open file handles as
> far as I know, at least that is how I always thought it works.
> 
> But hey, it's userspace so what do I know...
> 
> Anyways if it doesn't work like so, there are a bunch of subsystems in
> the kernel using it so they would all have severe problems.

Fine, just wanted to make sure.

> >> +     if (fd < 0) {
> >> +             ret = fd;
> >> +             goto out_free_descs;
> >> +     }
> >> +
> >> +     handlereq.fd = fd;
> >> +     if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
> >> +             return -EFAULT;
> > 
> > If I'm right above doesn't that then leak lh until application eventually
> > exits? Userspace won't receive the newly created fd. The same would apply
> > to patch 3/4.
> 
> I don't understand....
> 
> The userspace application reads the .fd fields of the handle request after
> issuing the ioctl() and it then contains the new file handle.

There is no valid .fd if copy_to_user() fails and immediately returning
-EFAULT. Thus the previously kzalloc'ed lh (well anything linehandle_release 
frees) is unaccessible. Userspace has no .fd for neither any ioctl nor for 
calling close(). This allocated memory will remain unavailable until the 
process exits. I think something like this should be added:

> @@ -486,8 +486,10 @@ static int linehandle_create(struct gpio_device *gdev,
> void __user *ip)> 
>  	}
>  	
>  	handlereq.fd = fd;
> 
> -	if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
> -		return -EFAULT;
> +	if (copy_to_user(ip, &handlereq, sizeof(handlereq))) {
> +		ret = -EFAULT;
> +		goto out_free_descs;
> +	}
> 
>  	dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
>  	
>  		lh->numdescs);

I hope this could make my concerns more clear. Opinions?

Best regards,
Alexander


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

* Re: [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines
  2016-06-29  9:43     ` Alexander Stein
@ 2016-07-04  9:55       ` Linus Walleij
  0 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2016-07-04  9:55 UTC (permalink / raw)
  To: Alexander Stein
  Cc: linux-gpio, Alexandre Courbot, Michael Welling, Markus Pargmann,
	Lee Campbell, Dmitry Torokhov, Bamvor Jian Zhang, Grant Likely,
	Arnd Bergmann, Mark Brown, Johan Hovold

On Wed, Jun 29, 2016 at 11:43 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> There is no valid .fd if copy_to_user() fails and immediately returning
> -EFAULT. Thus the previously kzalloc'ed lh (well anything linehandle_release
> frees) is unaccessible. Userspace has no .fd for neither any ioctl nor for
> calling close(). This allocated memory will remain unavailable until the
> process exits. I think something like this should be added:
>
>> @@ -486,8 +486,10 @@ static int linehandle_create(struct gpio_device *gdev,
>> void __user *ip)>
>>       }
>>
>>       handlereq.fd = fd;
>>
>> -     if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
>> -             return -EFAULT;
>> +     if (copy_to_user(ip, &handlereq, sizeof(handlereq))) {
>> +             ret = -EFAULT;
>> +             goto out_free_descs;
>> +     }

I see. I'm fixing a patch, also checking if I have that bug in more
places.

Yours,
Linus Walleij

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

end of thread, other threads:[~2016-07-04  9:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-02 11:51 [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Linus Walleij
2016-06-02 11:51 ` [PATCH 2/4] tools/gpio: add the gpio-hammer tool Linus Walleij
2016-06-02 11:51 ` [PATCH 3/4] gpio: userspace ABI for reading GPIO line events Linus Walleij
2016-06-02 11:51 ` [PATCH 4/4] tools/gpio: add the gpio-event-mon tool Linus Walleij
2016-06-02 18:12 ` [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines Michael Welling
2016-06-29  8:53 ` Alexander Stein
2016-06-29  9:05   ` Linus Walleij
2016-06-29  9:43     ` Alexander Stein
2016-07-04  9:55       ` Linus Walleij

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.