2019-12-04 16:04:08

by Bartosz Golaszewski

Subject: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

From: Bartosz Golaszewski <[email protected]>

Currently there is no way for user-space to be informed about changes
in status of GPIO lines e.g. when someone else requests the line or its
config changes. We can only periodically re-read the line-info. This
is fine for simple one-off user-space tools, but any daemon that provides
a centralized access to GPIO chips would benefit hugely from an event
driven line info synchronization.

This patch adds a new ioctl() that allows user-space processes to reuse
the file descriptor associated with the character device for watching
any changes in line properties. Every such event contains the updated
line information.

Currently the events are generated on three types of status changes: when
a line is requested, when it's released and when its config is changed.
The first two are self-explanatory. For the third one: this will only
happen when another user-space process calls the new SET_CONFIG ioctl()
as any changes that can happen from within the kernel (i.e.
set_transitory() or set_debounce()) are of no interest to user-space.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpiolib.c | 177 ++++++++++++++++++++++++++++++++++++--
drivers/gpio/gpiolib.h | 2 +
include/uapi/linux/gpio.h | 24 ++++++
3 files changed, 195 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 711963aa9239..2ff15ef0bbe0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -547,6 +547,9 @@ static long linehandle_set_config(struct linehandle_state *lh,
if (ret)
return ret;
}
+
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_CONFIG, desc);
}
return 0;
}
@@ -1199,14 +1202,24 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
spin_unlock_irqrestore(&gpio_lock, flags);
}

+struct gpio_chardev_data {
+ struct gpio_device *gdev;
+ wait_queue_head_t wait;
+ DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
+ struct notifier_block lineinfo_changed_nb;
+};
+
/*
* gpio_ioctl() - ioctl handler for the GPIO chardev
*/
static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
- struct gpio_device *gdev = filp->private_data;
+ struct gpio_chardev_data *priv = filp->private_data;
+ struct gpio_device *gdev = priv->gdev;
struct gpio_chip *chip = gdev->chip;
void __user *ip = (void __user *)arg;
+ struct gpio_desc *desc;
+ __u32 offset;

/* We fail any subsequent ioctl():s when the chip is gone */
if (!chip)
@@ -1228,9 +1241,9 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
return -EFAULT;
return 0;
- } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+ } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
+ cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
struct gpioline_info lineinfo;
- struct gpio_desc *desc;

if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
@@ -1243,11 +1256,25 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)

if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
+
+ if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL)
+ set_bit(FLAG_WATCHED, &desc->flags);
+
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);
+ } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
+ if (copy_from_user(&offset, ip, sizeof(offset)))
+ return -EFAULT;
+
+ desc = gpiochip_get_desc(chip, offset);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ clear_bit(FLAG_WATCHED, &desc->flags);
+ return 0;
}
return -EINVAL;
}
@@ -1260,6 +1287,99 @@ static long gpio_ioctl_compat(struct file *filp, unsigned int cmd,
}
#endif

+static struct gpio_chardev_data *
+to_gpio_chardev_data(struct notifier_block *nb)
+{
+ return container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
+}
+
+static int lineinfo_changed_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct gpio_chardev_data *priv = to_gpio_chardev_data(nb);
+ struct gpioline_info_changed chg;
+ struct gpio_desc *desc = data;
+ int ret = NOTIFY_DONE;
+
+ if (test_bit(FLAG_WATCHED, &desc->flags)) {
+ memset(&chg, 0, sizeof(chg));
+ chg.info.line_offset = gpio_chip_hwgpio(desc);
+ chg.event_type = action;
+ chg.timestamp = ktime_get_real_ns();
+ gpio_desc_to_lineinfo(desc, &chg.info);
+
+ ret = kfifo_in_spinlocked(&priv->events, &chg,
+ 1, &priv->wait.lock);
+ if (ret)
+ wake_up_poll(&priv->wait, EPOLLIN);
+ else
+ pr_debug_ratelimited(
+ "%s: lineinfo event FIFO is full - event dropped\n",
+ __func__);
+
+ ret = NOTIFY_OK;
+ }
+
+ return ret;
+}
+
+static __poll_t lineinfo_watch_poll(struct file *filep,
+ struct poll_table_struct *pollt)
+{
+ struct gpio_chardev_data *priv = filep->private_data;
+ __poll_t events = 0;
+
+ poll_wait(filep, &priv->wait, pollt);
+
+ spin_lock(&priv->wait.lock);
+ if (!kfifo_is_empty(&priv->events))
+ events = EPOLLIN | EPOLLRDNORM;
+ spin_unlock(&priv->wait.lock);
+
+ return events;
+}
+
+static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
+ size_t count, loff_t *off)
+{
+ struct gpio_chardev_data *priv = filep->private_data;
+ struct gpioline_info_changed event;
+ int ret;
+
+ if (count < sizeof(event))
+ return -EINVAL;
+
+ for (;;) {
+ spin_lock(&priv->wait.lock);
+ if (kfifo_is_empty(&priv->events)) {
+ if (filep->f_flags & O_NONBLOCK) {
+ spin_unlock(&priv->wait.lock);
+ return -EAGAIN;
+ }
+
+ ret = wait_event_interruptible_locked(priv->wait,
+ !kfifo_is_empty(&priv->events));
+ if (ret) {
+ spin_unlock(&priv->wait.lock);
+ return ret;
+ }
+ }
+
+ ret = kfifo_out(&priv->events, &event, 1);
+ spin_unlock(&priv->wait.lock);
+ if (ret == 1)
+ break;
+
+ /* We should never get here. See lineevent_read(). */
+ }
+
+ ret = copy_to_user(buf, &event, sizeof(event));
+ if (ret)
+ return -EFAULT;
+
+ return sizeof(event);
+}
+
/**
* gpio_chrdev_open() - open the chardev for ioctl operations
* @inode: inode for this chardev
@@ -1270,14 +1390,42 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
{
struct gpio_device *gdev = container_of(inode->i_cdev,
struct gpio_device, chrdev);
+ struct gpio_chardev_data *priv;
+ int ret;

/* Fail on open if the backing gpiochip is gone */
if (!gdev->chip)
return -ENODEV;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ init_waitqueue_head(&priv->wait);
+ INIT_KFIFO(priv->events);
+ priv->gdev = gdev;
+
+ priv->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
+ ret = atomic_notifier_chain_register(&gdev->notifier,
+ &priv->lineinfo_changed_nb);
+ if (ret)
+ goto out_free_priv;
+
get_device(&gdev->dev);
- filp->private_data = gdev;
+ filp->private_data = priv;
+
+ ret = nonseekable_open(inode, filp);
+ if (ret)
+ goto out_unregister_notifier;

- return nonseekable_open(inode, filp);
+ return ret;
+
+out_unregister_notifier:
+ atomic_notifier_chain_unregister(&gdev->notifier,
+ &priv->lineinfo_changed_nb);
+out_free_priv:
+ kfree(priv);
+ return ret;
}

/**
@@ -1288,17 +1436,22 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
*/
static int gpio_chrdev_release(struct inode *inode, struct file *filp)
{
- struct gpio_device *gdev = container_of(inode->i_cdev,
- struct gpio_device, chrdev);
+ struct gpio_chardev_data *priv = filp->private_data;
+ struct gpio_device *gdev = priv->gdev;

+ atomic_notifier_chain_unregister(&gdev->notifier,
+ &priv->lineinfo_changed_nb);
put_device(&gdev->dev);
+ kfree(priv);
+
return 0;
}

-
static const struct file_operations gpio_fileops = {
.release = gpio_chrdev_release,
.open = gpio_chrdev_open,
+ .poll = lineinfo_watch_poll,
+ .read = lineinfo_watch_read,
.owner = THIS_MODULE,
.llseek = no_llseek,
.unlocked_ioctl = gpio_ioctl,
@@ -1509,6 +1662,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
for (i = 0; i < chip->ngpio; i++)
gdev->descs[i].gdev = gdev;

+ ATOMIC_INIT_NOTIFIER_HEAD(&gdev->notifier);
+
#ifdef CONFIG_PINCTRL
INIT_LIST_HEAD(&gdev->pin_ranges);
#endif
@@ -2848,6 +3003,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
}
done:
spin_unlock_irqrestore(&gpio_lock, flags);
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_REQUESTED, desc);
return ret;
}

@@ -2945,6 +3102,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
}

spin_unlock_irqrestore(&gpio_lock, flags);
+ atomic_notifier_call_chain(&desc->gdev->notifier,
+ GPIOLINE_CHANGED_RELEASED, desc);
+
return ret;
}

@@ -3107,6 +3267,7 @@ static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
if (ret != -ENOTSUPP)
return ret;
}
+
return 0;
}

diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a1cbeabadc69..4fca77241fb0 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -54,6 +54,7 @@ struct gpio_device {
const char *label;
void *data;
struct list_head list;
+ struct atomic_notifier_head notifier;

#ifdef CONFIG_PINCTRL
/*
@@ -112,6 +113,7 @@ struct gpio_desc {
#define FLAG_PULL_UP 13 /* GPIO has pull up enabled */
#define FLAG_PULL_DOWN 14 /* GPIO has pull down enabled */
#define FLAG_BIAS_DISABLE 15 /* GPIO has pull disabled */
+#define FLAG_WATCHED 16 /* GPIO line is being watched by user-space */

/* Connection label */
const char *label;
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 799cf823d493..2401028ae7de 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -59,6 +59,28 @@ struct gpioline_info {
/* Maximum number of requested handles */
#define GPIOHANDLES_MAX 64

+/* Possible line status change events */
+enum {
+ GPIOLINE_CHANGED_REQUESTED = 1,
+ GPIOLINE_CHANGED_RELEASED,
+ GPIOLINE_CHANGED_CONFIG,
+};
+
+/**
+ * struct gpioline_info_changed - Information about a change in status
+ * of a GPIO line
+ * @timestamp: estimate of time of status change occurrence, in nanoseconds
+ * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
+ * and GPIOLINE_CHANGED_CONFIG
+ * @info: updated line information
+ */
+struct gpioline_info_changed {
+ __u64 timestamp;
+ __u32 event_type;
+ struct gpioline_info info;
+ __u32 padding[4]; /* for future use */
+};
+
/* Linerequest flags */
#define GPIOHANDLE_REQUEST_INPUT (1UL << 0)
#define GPIOHANDLE_REQUEST_OUTPUT (1UL << 1)
@@ -176,6 +198,8 @@ struct gpioevent_data {

#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_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
+#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
#define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
#define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)

--
2.23.0

2019-12-04 17:18:29

by Bartosz Golaszewski

Subject: [PATCH v2 11/11] tools: gpio: implement gpio-watch

From: Bartosz Golaszewski <[email protected]>

Add a simple program that allows to test the new LINECHANGED_FD ioctl().

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
tools/gpio/.gitignore | 1 +
tools/gpio/Build | 1 +
tools/gpio/Makefile | 11 +++-
tools/gpio/gpio-watch.c | 112 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 124 insertions(+), 1 deletion(-)
create mode 100644 tools/gpio/gpio-watch.c

diff --git a/tools/gpio/.gitignore b/tools/gpio/.gitignore
index a94c0e83b209..fffd32969d62 100644
--- a/tools/gpio/.gitignore
+++ b/tools/gpio/.gitignore
@@ -1,4 +1,5 @@
gpio-event-mon
gpio-hammer
lsgpio
+gpio-watch
include/linux/gpio.h
diff --git a/tools/gpio/Build b/tools/gpio/Build
index 4141f35837db..67c7b7f6a717 100644
--- a/tools/gpio/Build
+++ b/tools/gpio/Build
@@ -2,3 +2,4 @@ gpio-utils-y += gpio-utils.o
lsgpio-y += lsgpio.o gpio-utils.o
gpio-hammer-y += gpio-hammer.o gpio-utils.o
gpio-event-mon-y += gpio-event-mon.o gpio-utils.o
+gpio-watch-y += gpio-watch.o
diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
index 6080de58861f..842287e42c83 100644
--- a/tools/gpio/Makefile
+++ b/tools/gpio/Makefile
@@ -18,7 +18,7 @@ MAKEFLAGS += -r

override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include

-ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon
+ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon gpio-watch
ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))

all: $(ALL_PROGRAMS)
@@ -66,6 +66,15 @@ $(GPIO_EVENT_MON_IN): prepare FORCE $(OUTPUT)gpio-utils-in.o
$(OUTPUT)gpio-event-mon: $(GPIO_EVENT_MON_IN)
$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@

+#
+# gpio-watch
+#
+GPIO_WATCH_IN := $(OUTPUT)gpio-watch-in.o
+$(GPIO_WATCH_IN): prepare FORCE
+ $(Q)$(MAKE) $(build)=gpio-watch
+$(OUTPUT)gpio-watch: $(GPIO_WATCH_IN)
+ $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
clean:
rm -f $(ALL_PROGRAMS)
rm -f $(OUTPUT)include/linux/gpio.h
diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
new file mode 100644
index 000000000000..69aee43655ae
--- /dev/null
+++ b/tools/gpio/gpio-watch.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * gpio-watch - monitor unrequested lines for property changes using the
+ * character device
+ *
+ * Copyright (C) 2019 BayLibre SAS
+ * Author: Bartosz Golaszewski <[email protected]>
+ */
+
+#include <ctype.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <poll.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+static bool isnumber(const char *str)
+{
+ size_t sz = strlen(str);
+ int i;
+
+ for (i = 0; i < sz; i++) {
+ if (!isdigit(str[i]))
+ return false;
+ }
+
+ return true;
+}
+
+int main(int argc, char **argv)
+{
+ struct gpioline_info_changed chg;
+ struct gpioline_info req;
+ struct pollfd pfd;
+ int fd, i, j, ret;
+ char *event;
+ ssize_t rd;
+
+ if (argc < 3)
+ goto err_usage;
+
+ fd = open(argv[1], O_RDWR | O_CLOEXEC);
+ if (fd < 0) {
+ perror("unable to open gpiochip");
+ return EXIT_FAILURE;
+ }
+
+ for (i = 0, j = 2; i < argc - 2; i++, j++) {
+ if (!isnumber(argv[j]))
+ goto err_usage;
+
+ memset(&req, 0, sizeof(req));
+ req.line_offset = atoi(argv[j]);
+
+ ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
+ if (ret) {
+ perror("unable to set up line watch");
+ return EXIT_FAILURE;
+ }
+ }
+
+ pfd.fd = fd;
+ pfd.events = POLLIN | POLLPRI;
+
+ for (;;) {
+ ret = poll(&pfd, 1, 5000);
+ if (ret < 0) {
+ perror("error polling the linechanged fd");
+ return EXIT_FAILURE;
+ } else if (ret > 0) {
+ memset(&chg, 0, sizeof(chg));
+ rd = read(pfd.fd, &chg, sizeof(chg));
+ if (rd < 0 || rd != sizeof(chg)) {
+ if (rd != sizeof(chg))
+ errno = EIO;
+
+ perror("error reading line change event");
+ return EXIT_FAILURE;
+ }
+
+ switch (chg.event_type) {
+ case GPIOLINE_CHANGED_REQUESTED:
+ event = "requested";
+ break;
+ case GPIOLINE_CHANGED_RELEASED:
+ event = "released";
+ break;
+ case GPIOLINE_CHANGED_CONFIG:
+ event = "config changed";
+ break;
+ default:
+ fprintf(stderr,
+ "invalid event type received from the kernel\n");
+ return EXIT_FAILURE;
+ }
+
+ printf("line %u: %s at %llu\n",
+ chg.info.line_offset, event, chg.timestamp);
+ }
+ }
+
+ return 0;
+
+err_usage:
+ printf("%s: <gpiochip> <line0> <line1> ...\n", argv[0]);
+ return EXIT_FAILURE;
+}
--
2.23.0

2019-12-04 22:35:09

by Andy Shevchenko

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> Currently there is no way for user-space to be informed about changes
> in status of GPIO lines e.g. when someone else requests the line or its
> config changes. We can only periodically re-read the line-info. This
> is fine for simple one-off user-space tools, but any daemon that provides
> a centralized access to GPIO chips would benefit hugely from an event
> driven line info synchronization.
>
> This patch adds a new ioctl() that allows user-space processes to reuse
> the file descriptor associated with the character device for watching
> any changes in line properties. Every such event contains the updated
> line information.
>
> Currently the events are generated on three types of status changes: when
> a line is requested, when it's released and when its config is changed.
> The first two are self-explanatory. For the third one: this will only
> happen when another user-space process calls the new SET_CONFIG ioctl()
> as any changes that can happen from within the kernel (i.e.
> set_transitory() or set_debounce()) are of no interest to user-space.

> +/**
> + * struct gpioline_info_changed - Information about a change in status
> + * of a GPIO line
> + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> + * and GPIOLINE_CHANGED_CONFIG
> + * @info: updated line information
> + */
> +struct gpioline_info_changed {
> + __u64 timestamp;
> + __u32 event_type;
> + struct gpioline_info info;
> + __u32 padding[4]; /* for future use */
> +};

Has this been tested against 64-bit kernel / 32-bit userspace case?

--
With Best Regards,
Andy Shevchenko

2019-12-04 22:44:34

by Andy Shevchenko

Subject: Re: [PATCH v2 11/11] tools: gpio: implement gpio-watch

On Wed, Dec 4, 2019 at 7:19 PM Bartosz Golaszewski <[email protected]> wrote:
> Add a simple program that allows to test the new LINECHANGED_FD ioctl().

> --- a/tools/gpio/.gitignore
> +++ b/tools/gpio/.gitignore
> @@ -1,4 +1,5 @@
> gpio-event-mon
> gpio-hammer
> lsgpio
> +gpio-watch

Perhaps keep it sorted?

> +++ b/tools/gpio/gpio-watch.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * gpio-watch - monitor unrequested lines for property changes using the
> + * character device
> + *
> + * Copyright (C) 2019 BayLibre SAS
> + * Author: Bartosz Golaszewski <[email protected]>
> + */
> +
> +#include <ctype.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/gpio.h>
> +#include <poll.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +static bool isnumber(const char *str)
> +{
> + size_t sz = strlen(str);
> + int i;
> +
> + for (i = 0; i < sz; i++) {
> + if (!isdigit(str[i]))
> + return false;
> + }
> +
> + return true;
> +}

strtoul() will do the same.

char *p;
unsigned long dummy; // do we need it?
dummy = strtoul(..., &p);
return *p == '\0';

> +int main(int argc, char **argv)
> +{
> + struct gpioline_info_changed chg;
> + struct gpioline_info req;
> + struct pollfd pfd;
> + int fd, i, j, ret;
> + char *event;
> + ssize_t rd;
> +
> + if (argc < 3)
> + goto err_usage;
> +
> + fd = open(argv[1], O_RDWR | O_CLOEXEC);
> + if (fd < 0) {
> + perror("unable to open gpiochip");
> + return EXIT_FAILURE;
> + }
> +
> + for (i = 0, j = 2; i < argc - 2; i++, j++) {
> + if (!isnumber(argv[j]))
> + goto err_usage;
> +
> + memset(&req, 0, sizeof(req));
> + req.line_offset = atoi(argv[j]);

Oh, why not to call strtoul() directly?

> +
> + ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
> + if (ret) {
> + perror("unable to set up line watch");

Don't you need to unwatch previously added ones?

> + return EXIT_FAILURE;
> + }
> + }

> + for (;;) {
> + ret = poll(&pfd, 1, 5000);
> + if (ret < 0) {
> + perror("error polling the linechanged fd");
> + return EXIT_FAILURE;
> + } else if (ret > 0) {
> + memset(&chg, 0, sizeof(chg));

> + rd = read(pfd.fd, &chg, sizeof(chg));
> + if (rd < 0 || rd != sizeof(chg)) {
> + if (rd != sizeof(chg))
> + errno = EIO;
> +
> + perror("error reading line change event");
> + return EXIT_FAILURE;
> + }

Shouldn't we handle the -EINTR?

> + }
> + }
> +
> + return 0;


--
With Best Regards,
Andy Shevchenko

2019-12-05 09:43:08

by Bartosz Golaszewski

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

śr., 4 gru 2019 o 23:34 Andy Shevchenko <[email protected]> napisał(a):
>
> On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Currently there is no way for user-space to be informed about changes
> > in status of GPIO lines e.g. when someone else requests the line or its
> > config changes. We can only periodically re-read the line-info. This
> > is fine for simple one-off user-space tools, but any daemon that provides
> > a centralized access to GPIO chips would benefit hugely from an event
> > driven line info synchronization.
> >
> > This patch adds a new ioctl() that allows user-space processes to reuse
> > the file descriptor associated with the character device for watching
> > any changes in line properties. Every such event contains the updated
> > line information.
> >
> > Currently the events are generated on three types of status changes: when
> > a line is requested, when it's released and when its config is changed.
> > The first two are self-explanatory. For the third one: this will only
> > happen when another user-space process calls the new SET_CONFIG ioctl()
> > as any changes that can happen from within the kernel (i.e.
> > set_transitory() or set_debounce()) are of no interest to user-space.
>
> > +/**
> > + * struct gpioline_info_changed - Information about a change in status
> > + * of a GPIO line
> > + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> > + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> > + * and GPIOLINE_CHANGED_CONFIG
> > + * @info: updated line information
> > + */
> > +struct gpioline_info_changed {
> > + __u64 timestamp;
> > + __u32 event_type;
> > + struct gpioline_info info;
> > + __u32 padding[4]; /* for future use */
> > +};
>
> Has this been tested against 64-bit kernel / 32-bit userspace case?
>

No. Since this is a new thing - do you think it's possible to simply
arrange the fields or add padding such that the problem doesn't even
appear in the first place?

Bart

2019-12-05 09:45:18

by Kent Gibson

Subject: Re: [PATCH v2 11/11] tools: gpio: implement gpio-watch

On Wed, Dec 04, 2019 at 04:59:41PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a simple program that allows to test the new LINECHANGED_FD ioctl().
>

A minor nit - the ioctl has since been changed to LINEINFO_WATCH.

Do you have anything else to test the ioctls?
Either way, I'll try to find some time to add some to my gpiod library.

Kent.

> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> tools/gpio/.gitignore | 1 +
> tools/gpio/Build | 1 +
> tools/gpio/Makefile | 11 +++-
> tools/gpio/gpio-watch.c | 112 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 124 insertions(+), 1 deletion(-)
> create mode 100644 tools/gpio/gpio-watch.c
>
> diff --git a/tools/gpio/.gitignore b/tools/gpio/.gitignore
> index a94c0e83b209..fffd32969d62 100644
> --- a/tools/gpio/.gitignore
> +++ b/tools/gpio/.gitignore
> @@ -1,4 +1,5 @@
> gpio-event-mon
> gpio-hammer
> lsgpio
> +gpio-watch
> include/linux/gpio.h
> diff --git a/tools/gpio/Build b/tools/gpio/Build
> index 4141f35837db..67c7b7f6a717 100644
> --- a/tools/gpio/Build
> +++ b/tools/gpio/Build
> @@ -2,3 +2,4 @@ gpio-utils-y += gpio-utils.o
> lsgpio-y += lsgpio.o gpio-utils.o
> gpio-hammer-y += gpio-hammer.o gpio-utils.o
> gpio-event-mon-y += gpio-event-mon.o gpio-utils.o
> +gpio-watch-y += gpio-watch.o
> diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
> index 6080de58861f..842287e42c83 100644
> --- a/tools/gpio/Makefile
> +++ b/tools/gpio/Makefile
> @@ -18,7 +18,7 @@ MAKEFLAGS += -r
>
> override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>
> -ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon
> +ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon gpio-watch
> ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>
> all: $(ALL_PROGRAMS)
> @@ -66,6 +66,15 @@ $(GPIO_EVENT_MON_IN): prepare FORCE $(OUTPUT)gpio-utils-in.o
> $(OUTPUT)gpio-event-mon: $(GPIO_EVENT_MON_IN)
> $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
>
> +#
> +# gpio-watch
> +#
> +GPIO_WATCH_IN := $(OUTPUT)gpio-watch-in.o
> +$(GPIO_WATCH_IN): prepare FORCE
> + $(Q)$(MAKE) $(build)=gpio-watch
> +$(OUTPUT)gpio-watch: $(GPIO_WATCH_IN)
> + $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> +
> clean:
> rm -f $(ALL_PROGRAMS)
> rm -f $(OUTPUT)include/linux/gpio.h
> diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
> new file mode 100644
> index 000000000000..69aee43655ae
> --- /dev/null
> +++ b/tools/gpio/gpio-watch.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * gpio-watch - monitor unrequested lines for property changes using the
> + * character device
> + *
> + * Copyright (C) 2019 BayLibre SAS
> + * Author: Bartosz Golaszewski <[email protected]>
> + */
> +
> +#include <ctype.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/gpio.h>
> +#include <poll.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +static bool isnumber(const char *str)
> +{
> + size_t sz = strlen(str);
> + int i;
> +
> + for (i = 0; i < sz; i++) {
> + if (!isdigit(str[i]))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + struct gpioline_info_changed chg;
> + struct gpioline_info req;
> + struct pollfd pfd;
> + int fd, i, j, ret;
> + char *event;
> + ssize_t rd;
> +
> + if (argc < 3)
> + goto err_usage;
> +
> + fd = open(argv[1], O_RDWR | O_CLOEXEC);
> + if (fd < 0) {
> + perror("unable to open gpiochip");
> + return EXIT_FAILURE;
> + }
> +
> + for (i = 0, j = 2; i < argc - 2; i++, j++) {
> + if (!isnumber(argv[j]))
> + goto err_usage;
> +
> + memset(&req, 0, sizeof(req));
> + req.line_offset = atoi(argv[j]);
> +
> + ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
> + if (ret) {
> + perror("unable to set up line watch");
> + return EXIT_FAILURE;
> + }
> + }
> +
> + pfd.fd = fd;
> + pfd.events = POLLIN | POLLPRI;
> +
> + for (;;) {
> + ret = poll(&pfd, 1, 5000);
> + if (ret < 0) {
> + perror("error polling the linechanged fd");
> + return EXIT_FAILURE;
> + } else if (ret > 0) {
> + memset(&chg, 0, sizeof(chg));
> + rd = read(pfd.fd, &chg, sizeof(chg));
> + if (rd < 0 || rd != sizeof(chg)) {
> + if (rd != sizeof(chg))
> + errno = EIO;
> +
> + perror("error reading line change event");
> + return EXIT_FAILURE;
> + }
> +
> + switch (chg.event_type) {
> + case GPIOLINE_CHANGED_REQUESTED:
> + event = "requested";
> + break;
> + case GPIOLINE_CHANGED_RELEASED:
> + event = "released";
> + break;
> + case GPIOLINE_CHANGED_CONFIG:
> + event = "config changed";
> + break;
> + default:
> + fprintf(stderr,
> + "invalid event type received from the kernel\n");
> + return EXIT_FAILURE;
> + }
> +
> + printf("line %u: %s at %llu\n",
> + chg.info.line_offset, event, chg.timestamp);
> + }
> + }
> +
> + return 0;
> +
> +err_usage:
> + printf("%s: <gpiochip> <line0> <line1> ...\n", argv[0]);
> + return EXIT_FAILURE;
> +}
> --
> 2.23.0
>

2019-12-05 09:46:34

by Bartosz Golaszewski

Subject: Re: [PATCH v2 11/11] tools: gpio: implement gpio-watch

śr., 4 gru 2019 o 23:42 Andy Shevchenko <[email protected]> napisał(a):
>
> On Wed, Dec 4, 2019 at 7:19 PM Bartosz Golaszewski <[email protected]> wrote:
> > Add a simple program that allows to test the new LINECHANGED_FD ioctl().
>
> > --- a/tools/gpio/.gitignore
> > +++ b/tools/gpio/.gitignore
> > @@ -1,4 +1,5 @@
> > gpio-event-mon
> > gpio-hammer
> > lsgpio
> > +gpio-watch
>
> Perhaps keep it sorted?
>

Sure I can do this.

> > +++ b/tools/gpio/gpio-watch.c
> > @@ -0,0 +1,112 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * gpio-watch - monitor unrequested lines for property changes using the
> > + * character device
> > + *
> > + * Copyright (C) 2019 BayLibre SAS
> > + * Author: Bartosz Golaszewski <[email protected]>
> > + */
> > +
> > +#include <ctype.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/gpio.h>
> > +#include <poll.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +static bool isnumber(const char *str)
> > +{
> > + size_t sz = strlen(str);
> > + int i;
> > +
> > + for (i = 0; i < sz; i++) {
> > + if (!isdigit(str[i]))
> > + return false;
> > + }
> > +
> > + return true;
> > +}
>
> strtoul() will do the same.
>
> char *p;
> unsigned long dummy; // do we need it?
> dummy = strtoul(..., &p);
> return *p == '\0';

Yeah strtoul does allow for error checking. I'll change this in v3.

>
> > +int main(int argc, char **argv)
> > +{
> > + struct gpioline_info_changed chg;
> > + struct gpioline_info req;
> > + struct pollfd pfd;
> > + int fd, i, j, ret;
> > + char *event;
> > + ssize_t rd;
> > +
> > + if (argc < 3)
> > + goto err_usage;
> > +
> > + fd = open(argv[1], O_RDWR | O_CLOEXEC);
> > + if (fd < 0) {
> > + perror("unable to open gpiochip");
> > + return EXIT_FAILURE;
> > + }
> > +
> > + for (i = 0, j = 2; i < argc - 2; i++, j++) {
> > + if (!isnumber(argv[j]))
> > + goto err_usage;
> > +
> > + memset(&req, 0, sizeof(req));
> > + req.line_offset = atoi(argv[j]);
>
> Oh, why not to call strtoul() directly?
>
> > +
> > + ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
> > + if (ret) {
> > + perror("unable to set up line watch");
>
> Don't you need to unwatch previously added ones?

Nah, we're exiting anyway, the fd will be released.

>
> > + return EXIT_FAILURE;
> > + }
> > + }
>
> > + for (;;) {
> > + ret = poll(&pfd, 1, 5000);
> > + if (ret < 0) {
> > + perror("error polling the linechanged fd");
> > + return EXIT_FAILURE;
> > + } else if (ret > 0) {
> > + memset(&chg, 0, sizeof(chg));
>
> > + rd = read(pfd.fd, &chg, sizeof(chg));
> > + if (rd < 0 || rd != sizeof(chg)) {
> > + if (rd != sizeof(chg))
> > + errno = EIO;
> > +
> > + perror("error reading line change event");
> > + return EXIT_FAILURE;
> > + }
>
> Shouldn't we handle the -EINTR?

Indeed we should. Will fix.

Bart

>
> > + }
> > + }
> > +
> > + return 0;
>
>
> --
> With Best Regards,
> Andy Shevchenko

2019-12-05 09:50:06

by Bartosz Golaszewski

Subject: Re: [PATCH v2 11/11] tools: gpio: implement gpio-watch

czw., 5 gru 2019 o 10:44 Kent Gibson <[email protected]> napisał(a):
>
> On Wed, Dec 04, 2019 at 04:59:41PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Add a simple program that allows to test the new LINECHANGED_FD ioctl().
> >
>
> A minor nit - the ioctl has since been changed to LINEINFO_WATCH.

Will fix, thanks.

>
> Do you have anything else to test the ioctls?
> Either way, I'll try to find some time to add some to my gpiod library.
>

The time I can spend on this is limited, so I decided to go with the
kernel API first with a simple user-space test in the kernel source.
Once we agree upon it, I'll add support for this to libgpiod. The
user-space part would then probably be merged after v5.6-rc1 is tagged
and it leaves us 7 weeks to fix any bugs.

This is what I plan on doing with your series too. I hope to merge it
next week and then we can fix any potential bugs and do a libgpiod
release simultaneous with linux v5.5.

Bart

> Kent.
>
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > tools/gpio/.gitignore | 1 +
> > tools/gpio/Build | 1 +
> > tools/gpio/Makefile | 11 +++-
> > tools/gpio/gpio-watch.c | 112 ++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 124 insertions(+), 1 deletion(-)
> > create mode 100644 tools/gpio/gpio-watch.c
> >
> > diff --git a/tools/gpio/.gitignore b/tools/gpio/.gitignore
> > index a94c0e83b209..fffd32969d62 100644
> > --- a/tools/gpio/.gitignore
> > +++ b/tools/gpio/.gitignore
> > @@ -1,4 +1,5 @@
> > gpio-event-mon
> > gpio-hammer
> > lsgpio
> > +gpio-watch
> > include/linux/gpio.h
> > diff --git a/tools/gpio/Build b/tools/gpio/Build
> > index 4141f35837db..67c7b7f6a717 100644
> > --- a/tools/gpio/Build
> > +++ b/tools/gpio/Build
> > @@ -2,3 +2,4 @@ gpio-utils-y += gpio-utils.o
> > lsgpio-y += lsgpio.o gpio-utils.o
> > gpio-hammer-y += gpio-hammer.o gpio-utils.o
> > gpio-event-mon-y += gpio-event-mon.o gpio-utils.o
> > +gpio-watch-y += gpio-watch.o
> > diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
> > index 6080de58861f..842287e42c83 100644
> > --- a/tools/gpio/Makefile
> > +++ b/tools/gpio/Makefile
> > @@ -18,7 +18,7 @@ MAKEFLAGS += -r
> >
> > override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
> >
> > -ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon
> > +ALL_TARGETS := lsgpio gpio-hammer gpio-event-mon gpio-watch
> > ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
> >
> > all: $(ALL_PROGRAMS)
> > @@ -66,6 +66,15 @@ $(GPIO_EVENT_MON_IN): prepare FORCE $(OUTPUT)gpio-utils-in.o
> > $(OUTPUT)gpio-event-mon: $(GPIO_EVENT_MON_IN)
> > $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> >
> > +#
> > +# gpio-watch
> > +#
> > +GPIO_WATCH_IN := $(OUTPUT)gpio-watch-in.o
> > +$(GPIO_WATCH_IN): prepare FORCE
> > + $(Q)$(MAKE) $(build)=gpio-watch
> > +$(OUTPUT)gpio-watch: $(GPIO_WATCH_IN)
> > + $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> > +
> > clean:
> > rm -f $(ALL_PROGRAMS)
> > rm -f $(OUTPUT)include/linux/gpio.h
> > diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
> > new file mode 100644
> > index 000000000000..69aee43655ae
> > --- /dev/null
> > +++ b/tools/gpio/gpio-watch.c
> > @@ -0,0 +1,112 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * gpio-watch - monitor unrequested lines for property changes using the
> > + * character device
> > + *
> > + * Copyright (C) 2019 BayLibre SAS
> > + * Author: Bartosz Golaszewski <[email protected]>
> > + */
> > +
> > +#include <ctype.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/gpio.h>
> > +#include <poll.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +static bool isnumber(const char *str)
> > +{
> > + size_t sz = strlen(str);
> > + int i;
> > +
> > + for (i = 0; i < sz; i++) {
> > + if (!isdigit(str[i]))
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + struct gpioline_info_changed chg;
> > + struct gpioline_info req;
> > + struct pollfd pfd;
> > + int fd, i, j, ret;
> > + char *event;
> > + ssize_t rd;
> > +
> > + if (argc < 3)
> > + goto err_usage;
> > +
> > + fd = open(argv[1], O_RDWR | O_CLOEXEC);
> > + if (fd < 0) {
> > + perror("unable to open gpiochip");
> > + return EXIT_FAILURE;
> > + }
> > +
> > + for (i = 0, j = 2; i < argc - 2; i++, j++) {
> > + if (!isnumber(argv[j]))
> > + goto err_usage;
> > +
> > + memset(&req, 0, sizeof(req));
> > + req.line_offset = atoi(argv[j]);
> > +
> > + ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
> > + if (ret) {
> > + perror("unable to set up line watch");
> > + return EXIT_FAILURE;
> > + }
> > + }
> > +
> > + pfd.fd = fd;
> > + pfd.events = POLLIN | POLLPRI;
> > +
> > + for (;;) {
> > + ret = poll(&pfd, 1, 5000);
> > + if (ret < 0) {
> > + perror("error polling the linechanged fd");
> > + return EXIT_FAILURE;
> > + } else if (ret > 0) {
> > + memset(&chg, 0, sizeof(chg));
> > + rd = read(pfd.fd, &chg, sizeof(chg));
> > + if (rd < 0 || rd != sizeof(chg)) {
> > + if (rd != sizeof(chg))
> > + errno = EIO;
> > +
> > + perror("error reading line change event");
> > + return EXIT_FAILURE;
> > + }
> > +
> > + switch (chg.event_type) {
> > + case GPIOLINE_CHANGED_REQUESTED:
> > + event = "requested";
> > + break;
> > + case GPIOLINE_CHANGED_RELEASED:
> > + event = "released";
> > + break;
> > + case GPIOLINE_CHANGED_CONFIG:
> > + event = "config changed";
> > + break;
> > + default:
> > + fprintf(stderr,
> > + "invalid event type received from the kernel\n");
> > + return EXIT_FAILURE;
> > + }
> > +
> > + printf("line %u: %s at %llu\n",
> > + chg.info.line_offset, event, chg.timestamp);
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_usage:
> > + printf("%s: <gpiochip> <line0> <line1> ...\n", argv[0]);
> > + return EXIT_FAILURE;
> > +}
> > --
> > 2.23.0
> >

2019-12-05 10:28:08

by Andy Shevchenko

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

On Thu, Dec 5, 2019 at 11:42 AM Bartosz Golaszewski
<[email protected]> wrote:
> śr., 4 gru 2019 o 23:34 Andy Shevchenko <[email protected]> napisał(a):
> > On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:

> > > +struct gpioline_info_changed {
> > > + __u64 timestamp;
> > > + __u32 event_type;
> > > + struct gpioline_info info;
> > > + __u32 padding[4]; /* for future use */
> > > +};
> >
> > Has this been tested against 64-bit kernel / 32-bit userspace case?
> >
>
> No. Since this is a new thing - do you think it's possible to simply
> arrange the fields or add padding such that the problem doesn't even
> appear in the first place?

Yes. this can be done, though be careful about potential endianess
issues (the ABI must be tested on BE as well).

So, the test cases, I can imagine of, should include (k - kernel, u - user):
- 64k-64u: LE and BE
- 64k-32u: LE and BE
- 32k-32u: LE and BE

--
With Best Regards,
Andy Shevchenko

2019-12-05 10:48:19

by Kent Gibson

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

On Wed, Dec 04, 2019 at 04:59:40PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Currently there is no way for user-space to be informed about changes
> in status of GPIO lines e.g. when someone else requests the line or its
> config changes. We can only periodically re-read the line-info. This
> is fine for simple one-off user-space tools, but any daemon that provides
> a centralized access to GPIO chips would benefit hugely from an event
> driven line info synchronization.
>
> This patch adds a new ioctl() that allows user-space processes to reuse
> the file descriptor associated with the character device for watching
> any changes in line properties. Every such event contains the updated
> line information.
>
> Currently the events are generated on three types of status changes: when
> a line is requested, when it's released and when its config is changed.
> The first two are self-explanatory. For the third one: this will only
> happen when another user-space process calls the new SET_CONFIG ioctl()
> as any changes that can happen from within the kernel (i.e.
> set_transitory() or set_debounce()) are of no interest to user-space.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/gpio/gpiolib.c | 177 ++++++++++++++++++++++++++++++++++++--
> drivers/gpio/gpiolib.h | 2 +
> include/uapi/linux/gpio.h | 24 ++++++
> 3 files changed, 195 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 711963aa9239..2ff15ef0bbe0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -547,6 +547,9 @@ static long linehandle_set_config(struct linehandle_state *lh,
> if (ret)
> return ret;
> }
> +
> + atomic_notifier_call_chain(&desc->gdev->notifier,
> + GPIOLINE_CHANGED_CONFIG, desc);
> }
> return 0;
> }
> @@ -1199,14 +1202,24 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> spin_unlock_irqrestore(&gpio_lock, flags);
> }
>
> +struct gpio_chardev_data {
> + struct gpio_device *gdev;
> + wait_queue_head_t wait;
> + DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
> + struct notifier_block lineinfo_changed_nb;
> +};
> +
> /*
> * gpio_ioctl() - ioctl handler for the GPIO chardev
> */
> static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> - struct gpio_device *gdev = filp->private_data;
> + struct gpio_chardev_data *priv = filp->private_data;
> + struct gpio_device *gdev = priv->gdev;
> struct gpio_chip *chip = gdev->chip;
> void __user *ip = (void __user *)arg;
> + struct gpio_desc *desc;
> + __u32 offset;
>
> /* We fail any subsequent ioctl():s when the chip is gone */
> if (!chip)
> @@ -1228,9 +1241,9 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
> return -EFAULT;
> return 0;
> - } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
> + } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
> + cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
> struct gpioline_info lineinfo;
> - struct gpio_desc *desc;
>
> if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
> return -EFAULT;
> @@ -1243,11 +1256,25 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>
> if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> return -EFAULT;
> +
> + if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL)
> + set_bit(FLAG_WATCHED, &desc->flags);
> +

The WATCHED flag is stored globally in the gdev->descs?
Shouldn't it be stored in struct gpio_chardev_data?
Otherwise I can open the chip and disable your watches.

Kent.


> 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);
> + } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
> + if (copy_from_user(&offset, ip, sizeof(offset)))
> + return -EFAULT;
> +
> + desc = gpiochip_get_desc(chip, offset);
> + if (IS_ERR(desc))
> + return PTR_ERR(desc);
> +
> + clear_bit(FLAG_WATCHED, &desc->flags);
> + return 0;
> }
> return -EINVAL;
> }
> @@ -1260,6 +1287,99 @@ static long gpio_ioctl_compat(struct file *filp, unsigned int cmd,
> }
> #endif
>
> +static struct gpio_chardev_data *
> +to_gpio_chardev_data(struct notifier_block *nb)
> +{
> + return container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
> +}
> +
> +static int lineinfo_changed_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct gpio_chardev_data *priv = to_gpio_chardev_data(nb);
> + struct gpioline_info_changed chg;
> + struct gpio_desc *desc = data;
> + int ret = NOTIFY_DONE;
> +
> + if (test_bit(FLAG_WATCHED, &desc->flags)) {
> + memset(&chg, 0, sizeof(chg));
> + chg.info.line_offset = gpio_chip_hwgpio(desc);
> + chg.event_type = action;
> + chg.timestamp = ktime_get_real_ns();
> + gpio_desc_to_lineinfo(desc, &chg.info);
> +
> + ret = kfifo_in_spinlocked(&priv->events, &chg,
> + 1, &priv->wait.lock);
> + if (ret)
> + wake_up_poll(&priv->wait, EPOLLIN);
> + else
> + pr_debug_ratelimited(
> + "%s: lineinfo event FIFO is full - event dropped\n",
> + __func__);
> +
> + ret = NOTIFY_OK;
> + }
> +
> + return ret;
> +}
> +
> +static __poll_t lineinfo_watch_poll(struct file *filep,
> + struct poll_table_struct *pollt)
> +{
> + struct gpio_chardev_data *priv = filep->private_data;
> + __poll_t events = 0;
> +
> + poll_wait(filep, &priv->wait, pollt);
> +
> + spin_lock(&priv->wait.lock);
> + if (!kfifo_is_empty(&priv->events))
> + events = EPOLLIN | EPOLLRDNORM;
> + spin_unlock(&priv->wait.lock);
> +
> + return events;
> +}
> +
> +static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
> + size_t count, loff_t *off)
> +{
> + struct gpio_chardev_data *priv = filep->private_data;
> + struct gpioline_info_changed event;
> + int ret;
> +
> + if (count < sizeof(event))
> + return -EINVAL;
> +
> + for (;;) {
> + spin_lock(&priv->wait.lock);
> + if (kfifo_is_empty(&priv->events)) {
> + if (filep->f_flags & O_NONBLOCK) {
> + spin_unlock(&priv->wait.lock);
> + return -EAGAIN;
> + }
> +
> + ret = wait_event_interruptible_locked(priv->wait,
> + !kfifo_is_empty(&priv->events));
> + if (ret) {
> + spin_unlock(&priv->wait.lock);
> + return ret;
> + }
> + }
> +
> + ret = kfifo_out(&priv->events, &event, 1);
> + spin_unlock(&priv->wait.lock);
> + if (ret == 1)
> + break;
> +
> + /* We should never get here. See lineevent_read(). */
> + }
> +
> + ret = copy_to_user(buf, &event, sizeof(event));
> + if (ret)
> + return -EFAULT;
> +
> + return sizeof(event);
> +}
> +
> /**
> * gpio_chrdev_open() - open the chardev for ioctl operations
> * @inode: inode for this chardev
> @@ -1270,14 +1390,42 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
> {
> struct gpio_device *gdev = container_of(inode->i_cdev,
> struct gpio_device, chrdev);
> + struct gpio_chardev_data *priv;
> + int ret;
>
> /* Fail on open if the backing gpiochip is gone */
> if (!gdev->chip)
> return -ENODEV;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + init_waitqueue_head(&priv->wait);
> + INIT_KFIFO(priv->events);
> + priv->gdev = gdev;
> +
> + priv->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
> + ret = atomic_notifier_chain_register(&gdev->notifier,
> + &priv->lineinfo_changed_nb);
> + if (ret)
> + goto out_free_priv;
> +
> get_device(&gdev->dev);
> - filp->private_data = gdev;
> + filp->private_data = priv;
> +
> + ret = nonseekable_open(inode, filp);
> + if (ret)
> + goto out_unregister_notifier;
>
> - return nonseekable_open(inode, filp);
> + return ret;
> +
> +out_unregister_notifier:
> + atomic_notifier_chain_unregister(&gdev->notifier,
> + &priv->lineinfo_changed_nb);
> +out_free_priv:
> + kfree(priv);
> + return ret;
> }
>
> /**
> @@ -1288,17 +1436,22 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
> */
> static int gpio_chrdev_release(struct inode *inode, struct file *filp)
> {
> - struct gpio_device *gdev = container_of(inode->i_cdev,
> - struct gpio_device, chrdev);
> + struct gpio_chardev_data *priv = filp->private_data;
> + struct gpio_device *gdev = priv->gdev;
>
> + atomic_notifier_chain_unregister(&gdev->notifier,
> + &priv->lineinfo_changed_nb);
> put_device(&gdev->dev);
> + kfree(priv);
> +
> return 0;
> }
>
> -
> static const struct file_operations gpio_fileops = {
> .release = gpio_chrdev_release,
> .open = gpio_chrdev_open,
> + .poll = lineinfo_watch_poll,
> + .read = lineinfo_watch_read,
> .owner = THIS_MODULE,
> .llseek = no_llseek,
> .unlocked_ioctl = gpio_ioctl,
> @@ -1509,6 +1662,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
> for (i = 0; i < chip->ngpio; i++)
> gdev->descs[i].gdev = gdev;
>
> + ATOMIC_INIT_NOTIFIER_HEAD(&gdev->notifier);
> +
> #ifdef CONFIG_PINCTRL
> INIT_LIST_HEAD(&gdev->pin_ranges);
> #endif
> @@ -2848,6 +3003,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> }
> done:
> spin_unlock_irqrestore(&gpio_lock, flags);
> + atomic_notifier_call_chain(&desc->gdev->notifier,
> + GPIOLINE_CHANGED_REQUESTED, desc);
> return ret;
> }
>
> @@ -2945,6 +3102,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> }
>
> spin_unlock_irqrestore(&gpio_lock, flags);
> + atomic_notifier_call_chain(&desc->gdev->notifier,
> + GPIOLINE_CHANGED_RELEASED, desc);
> +
> return ret;
> }
>
> @@ -3107,6 +3267,7 @@ static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
> if (ret != -ENOTSUPP)
> return ret;
> }
> +
> return 0;
> }
>
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index a1cbeabadc69..4fca77241fb0 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -54,6 +54,7 @@ struct gpio_device {
> const char *label;
> void *data;
> struct list_head list;
> + struct atomic_notifier_head notifier;
>
> #ifdef CONFIG_PINCTRL
> /*
> @@ -112,6 +113,7 @@ struct gpio_desc {
> #define FLAG_PULL_UP 13 /* GPIO has pull up enabled */
> #define FLAG_PULL_DOWN 14 /* GPIO has pull down enabled */
> #define FLAG_BIAS_DISABLE 15 /* GPIO has pull disabled */
> +#define FLAG_WATCHED 16 /* GPIO line is being watched by user-space */
>
> /* Connection label */
> const char *label;
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 799cf823d493..2401028ae7de 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -59,6 +59,28 @@ struct gpioline_info {
> /* Maximum number of requested handles */
> #define GPIOHANDLES_MAX 64
>
> +/* Possible line status change events */
> +enum {
> + GPIOLINE_CHANGED_REQUESTED = 1,
> + GPIOLINE_CHANGED_RELEASED,
> + GPIOLINE_CHANGED_CONFIG,
> +};
> +
> +/**
> + * struct gpioline_info_changed - Information about a change in status
> + * of a GPIO line
> + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> + * and GPIOLINE_CHANGED_CONFIG
> + * @info: updated line information
> + */
> +struct gpioline_info_changed {
> + __u64 timestamp;
> + __u32 event_type;
> + struct gpioline_info info;
> + __u32 padding[4]; /* for future use */
> +};
> +
> /* Linerequest flags */
> #define GPIOHANDLE_REQUEST_INPUT (1UL << 0)
> #define GPIOHANDLE_REQUEST_OUTPUT (1UL << 1)
> @@ -176,6 +198,8 @@ struct gpioevent_data {
>
> #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_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
> +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
> #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
>
> --
> 2.23.0
>

2019-12-05 10:52:07

by Bartosz Golaszewski

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

czw., 5 gru 2019 o 11:47 Kent Gibson <[email protected]> napisał(a):
>
> On Wed, Dec 04, 2019 at 04:59:40PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Currently there is no way for user-space to be informed about changes
> > in status of GPIO lines e.g. when someone else requests the line or its
> > config changes. We can only periodically re-read the line-info. This
> > is fine for simple one-off user-space tools, but any daemon that provides
> > a centralized access to GPIO chips would benefit hugely from an event
> > driven line info synchronization.
> >
> > This patch adds a new ioctl() that allows user-space processes to reuse
> > the file descriptor associated with the character device for watching
> > any changes in line properties. Every such event contains the updated
> > line information.
> >
> > Currently the events are generated on three types of status changes: when
> > a line is requested, when it's released and when its config is changed.
> > The first two are self-explanatory. For the third one: this will only
> > happen when another user-space process calls the new SET_CONFIG ioctl()
> > as any changes that can happen from within the kernel (i.e.
> > set_transitory() or set_debounce()) are of no interest to user-space.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > drivers/gpio/gpiolib.c | 177 ++++++++++++++++++++++++++++++++++++--
> > drivers/gpio/gpiolib.h | 2 +
> > include/uapi/linux/gpio.h | 24 ++++++
> > 3 files changed, 195 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 711963aa9239..2ff15ef0bbe0 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -547,6 +547,9 @@ static long linehandle_set_config(struct linehandle_state *lh,
> > if (ret)
> > return ret;
> > }
> > +
> > + atomic_notifier_call_chain(&desc->gdev->notifier,
> > + GPIOLINE_CHANGED_CONFIG, desc);
> > }
> > return 0;
> > }
> > @@ -1199,14 +1202,24 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> > spin_unlock_irqrestore(&gpio_lock, flags);
> > }
> >
> > +struct gpio_chardev_data {
> > + struct gpio_device *gdev;
> > + wait_queue_head_t wait;
> > + DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
> > + struct notifier_block lineinfo_changed_nb;
> > +};
> > +
> > /*
> > * gpio_ioctl() - ioctl handler for the GPIO chardev
> > */
> > static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > {
> > - struct gpio_device *gdev = filp->private_data;
> > + struct gpio_chardev_data *priv = filp->private_data;
> > + struct gpio_device *gdev = priv->gdev;
> > struct gpio_chip *chip = gdev->chip;
> > void __user *ip = (void __user *)arg;
> > + struct gpio_desc *desc;
> > + __u32 offset;
> >
> > /* We fail any subsequent ioctl():s when the chip is gone */
> > if (!chip)
> > @@ -1228,9 +1241,9 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
> > return -EFAULT;
> > return 0;
> > - } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
> > + } else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
> > + cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
> > struct gpioline_info lineinfo;
> > - struct gpio_desc *desc;
> >
> > if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
> > return -EFAULT;
> > @@ -1243,11 +1256,25 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >
> > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> > return -EFAULT;
> > +
> > + if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL)
> > + set_bit(FLAG_WATCHED, &desc->flags);
> > +
>
> The WATCHED flag is stored globally in the gdev->descs?
> Shouldn't it be stored in struct gpio_chardev_data?
> Otherwise I can open the chip and disable your watches.
>

Yes, I feel stupid now. :)

Of course. I guess a bitmap for all the lines as part of the
chardev_data will be fine.

Bart

> Kent.
>
>
> > 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);
> > + } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
> > + if (copy_from_user(&offset, ip, sizeof(offset)))
> > + return -EFAULT;
> > +
> > + desc = gpiochip_get_desc(chip, offset);
> > + if (IS_ERR(desc))
> > + return PTR_ERR(desc);
> > +
> > + clear_bit(FLAG_WATCHED, &desc->flags);
> > + return 0;
> > }
> > return -EINVAL;
> > }
> > @@ -1260,6 +1287,99 @@ static long gpio_ioctl_compat(struct file *filp, unsigned int cmd,
> > }
> > #endif
> >
> > +static struct gpio_chardev_data *
> > +to_gpio_chardev_data(struct notifier_block *nb)
> > +{
> > + return container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
> > +}
> > +
> > +static int lineinfo_changed_notify(struct notifier_block *nb,
> > + unsigned long action, void *data)
> > +{
> > + struct gpio_chardev_data *priv = to_gpio_chardev_data(nb);
> > + struct gpioline_info_changed chg;
> > + struct gpio_desc *desc = data;
> > + int ret = NOTIFY_DONE;
> > +
> > + if (test_bit(FLAG_WATCHED, &desc->flags)) {
> > + memset(&chg, 0, sizeof(chg));
> > + chg.info.line_offset = gpio_chip_hwgpio(desc);
> > + chg.event_type = action;
> > + chg.timestamp = ktime_get_real_ns();
> > + gpio_desc_to_lineinfo(desc, &chg.info);
> > +
> > + ret = kfifo_in_spinlocked(&priv->events, &chg,
> > + 1, &priv->wait.lock);
> > + if (ret)
> > + wake_up_poll(&priv->wait, EPOLLIN);
> > + else
> > + pr_debug_ratelimited(
> > + "%s: lineinfo event FIFO is full - event dropped\n",
> > + __func__);
> > +
> > + ret = NOTIFY_OK;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static __poll_t lineinfo_watch_poll(struct file *filep,
> > + struct poll_table_struct *pollt)
> > +{
> > + struct gpio_chardev_data *priv = filep->private_data;
> > + __poll_t events = 0;
> > +
> > + poll_wait(filep, &priv->wait, pollt);
> > +
> > + spin_lock(&priv->wait.lock);
> > + if (!kfifo_is_empty(&priv->events))
> > + events = EPOLLIN | EPOLLRDNORM;
> > + spin_unlock(&priv->wait.lock);
> > +
> > + return events;
> > +}
> > +
> > +static ssize_t lineinfo_watch_read(struct file *filep, char __user *buf,
> > + size_t count, loff_t *off)
> > +{
> > + struct gpio_chardev_data *priv = filep->private_data;
> > + struct gpioline_info_changed event;
> > + int ret;
> > +
> > + if (count < sizeof(event))
> > + return -EINVAL;
> > +
> > + for (;;) {
> > + spin_lock(&priv->wait.lock);
> > + if (kfifo_is_empty(&priv->events)) {
> > + if (filep->f_flags & O_NONBLOCK) {
> > + spin_unlock(&priv->wait.lock);
> > + return -EAGAIN;
> > + }
> > +
> > + ret = wait_event_interruptible_locked(priv->wait,
> > + !kfifo_is_empty(&priv->events));
> > + if (ret) {
> > + spin_unlock(&priv->wait.lock);
> > + return ret;
> > + }
> > + }
> > +
> > + ret = kfifo_out(&priv->events, &event, 1);
> > + spin_unlock(&priv->wait.lock);
> > + if (ret == 1)
> > + break;
> > +
> > + /* We should never get here. See lineevent_read(). */
> > + }
> > +
> > + ret = copy_to_user(buf, &event, sizeof(event));
> > + if (ret)
> > + return -EFAULT;
> > +
> > + return sizeof(event);
> > +}
> > +
> > /**
> > * gpio_chrdev_open() - open the chardev for ioctl operations
> > * @inode: inode for this chardev
> > @@ -1270,14 +1390,42 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
> > {
> > struct gpio_device *gdev = container_of(inode->i_cdev,
> > struct gpio_device, chrdev);
> > + struct gpio_chardev_data *priv;
> > + int ret;
> >
> > /* Fail on open if the backing gpiochip is gone */
> > if (!gdev->chip)
> > return -ENODEV;
> > +
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + init_waitqueue_head(&priv->wait);
> > + INIT_KFIFO(priv->events);
> > + priv->gdev = gdev;
> > +
> > + priv->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
> > + ret = atomic_notifier_chain_register(&gdev->notifier,
> > + &priv->lineinfo_changed_nb);
> > + if (ret)
> > + goto out_free_priv;
> > +
> > get_device(&gdev->dev);
> > - filp->private_data = gdev;
> > + filp->private_data = priv;
> > +
> > + ret = nonseekable_open(inode, filp);
> > + if (ret)
> > + goto out_unregister_notifier;
> >
> > - return nonseekable_open(inode, filp);
> > + return ret;
> > +
> > +out_unregister_notifier:
> > + atomic_notifier_chain_unregister(&gdev->notifier,
> > + &priv->lineinfo_changed_nb);
> > +out_free_priv:
> > + kfree(priv);
> > + return ret;
> > }
> >
> > /**
> > @@ -1288,17 +1436,22 @@ static int gpio_chrdev_open(struct inode *inode, struct file *filp)
> > */
> > static int gpio_chrdev_release(struct inode *inode, struct file *filp)
> > {
> > - struct gpio_device *gdev = container_of(inode->i_cdev,
> > - struct gpio_device, chrdev);
> > + struct gpio_chardev_data *priv = filp->private_data;
> > + struct gpio_device *gdev = priv->gdev;
> >
> > + atomic_notifier_chain_unregister(&gdev->notifier,
> > + &priv->lineinfo_changed_nb);
> > put_device(&gdev->dev);
> > + kfree(priv);
> > +
> > return 0;
> > }
> >
> > -
> > static const struct file_operations gpio_fileops = {
> > .release = gpio_chrdev_release,
> > .open = gpio_chrdev_open,
> > + .poll = lineinfo_watch_poll,
> > + .read = lineinfo_watch_read,
> > .owner = THIS_MODULE,
> > .llseek = no_llseek,
> > .unlocked_ioctl = gpio_ioctl,
> > @@ -1509,6 +1662,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
> > for (i = 0; i < chip->ngpio; i++)
> > gdev->descs[i].gdev = gdev;
> >
> > + ATOMIC_INIT_NOTIFIER_HEAD(&gdev->notifier);
> > +
> > #ifdef CONFIG_PINCTRL
> > INIT_LIST_HEAD(&gdev->pin_ranges);
> > #endif
> > @@ -2848,6 +3003,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > }
> > done:
> > spin_unlock_irqrestore(&gpio_lock, flags);
> > + atomic_notifier_call_chain(&desc->gdev->notifier,
> > + GPIOLINE_CHANGED_REQUESTED, desc);
> > return ret;
> > }
> >
> > @@ -2945,6 +3102,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> > }
> >
> > spin_unlock_irqrestore(&gpio_lock, flags);
> > + atomic_notifier_call_chain(&desc->gdev->notifier,
> > + GPIOLINE_CHANGED_RELEASED, desc);
> > +
> > return ret;
> > }
> >
> > @@ -3107,6 +3267,7 @@ static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc)
> > if (ret != -ENOTSUPP)
> > return ret;
> > }
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> > index a1cbeabadc69..4fca77241fb0 100644
> > --- a/drivers/gpio/gpiolib.h
> > +++ b/drivers/gpio/gpiolib.h
> > @@ -54,6 +54,7 @@ struct gpio_device {
> > const char *label;
> > void *data;
> > struct list_head list;
> > + struct atomic_notifier_head notifier;
> >
> > #ifdef CONFIG_PINCTRL
> > /*
> > @@ -112,6 +113,7 @@ struct gpio_desc {
> > #define FLAG_PULL_UP 13 /* GPIO has pull up enabled */
> > #define FLAG_PULL_DOWN 14 /* GPIO has pull down enabled */
> > #define FLAG_BIAS_DISABLE 15 /* GPIO has pull disabled */
> > +#define FLAG_WATCHED 16 /* GPIO line is being watched by user-space */
> >
> > /* Connection label */
> > const char *label;
> > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> > index 799cf823d493..2401028ae7de 100644
> > --- a/include/uapi/linux/gpio.h
> > +++ b/include/uapi/linux/gpio.h
> > @@ -59,6 +59,28 @@ struct gpioline_info {
> > /* Maximum number of requested handles */
> > #define GPIOHANDLES_MAX 64
> >
> > +/* Possible line status change events */
> > +enum {
> > + GPIOLINE_CHANGED_REQUESTED = 1,
> > + GPIOLINE_CHANGED_RELEASED,
> > + GPIOLINE_CHANGED_CONFIG,
> > +};
> > +
> > +/**
> > + * struct gpioline_info_changed - Information about a change in status
> > + * of a GPIO line
> > + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> > + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> > + * and GPIOLINE_CHANGED_CONFIG
> > + * @info: updated line information
> > + */
> > +struct gpioline_info_changed {
> > + __u64 timestamp;
> > + __u32 event_type;
> > + struct gpioline_info info;
> > + __u32 padding[4]; /* for future use */
> > +};
> > +
> > /* Linerequest flags */
> > #define GPIOHANDLE_REQUEST_INPUT (1UL << 0)
> > #define GPIOHANDLE_REQUEST_OUTPUT (1UL << 1)
> > @@ -176,6 +198,8 @@ struct gpioevent_data {
> >
> > #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_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
> > +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
> > #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> > #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
> >
> > --
> > 2.23.0
> >

2019-12-05 13:48:35

by Bartosz Golaszewski

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

czw., 5 gru 2019 o 11:27 Andy Shevchenko <[email protected]> napisał(a):
>
> On Thu, Dec 5, 2019 at 11:42 AM Bartosz Golaszewski
> <[email protected]> wrote:
> > śr., 4 gru 2019 o 23:34 Andy Shevchenko <[email protected]> napisał(a):
> > > On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:
>
> > > > +struct gpioline_info_changed {
> > > > + __u64 timestamp;
> > > > + __u32 event_type;
> > > > + struct gpioline_info info;
> > > > + __u32 padding[4]; /* for future use */
> > > > +};
> > >
> > > Has this been tested against 64-bit kernel / 32-bit userspace case?
> > >
> >
> > No. Since this is a new thing - do you think it's possible to simply
> > arrange the fields or add padding such that the problem doesn't even
> > appear in the first place?
>
> Yes. this can be done, though be careful about potential endianess
> issues (the ABI must be tested on BE as well).
>
> So, the test cases, I can imagine of, should include (k - kernel, u - user):
> - 64k-64u: LE and BE
> - 64k-32u: LE and BE
> - 32k-32u: LE and BE

I usually use qemu VMs built with yocto for testing but I don't see
any way of creating a 32-bit user-space with 64-bit kernel. Any ideas
on how to prepare a testing environment?

Bart

>
> --
> With Best Regards,
> Andy Shevchenko

2019-12-05 17:05:38

by Andy Shevchenko

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

On Thu, Dec 5, 2019 at 3:47 PM Bartosz Golaszewski <[email protected]> wrote:
> czw., 5 gru 2019 o 11:27 Andy Shevchenko <[email protected]> napisał(a):
> > On Thu, Dec 5, 2019 at 11:42 AM Bartosz Golaszewski
> > <[email protected]> wrote:
> > > śr., 4 gru 2019 o 23:34 Andy Shevchenko <[email protected]> napisał(a):
> > > > On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:

> > So, the test cases, I can imagine of, should include (k - kernel, u - user):
> > - 64k-64u: LE and BE
> > - 64k-32u: LE and BE
> > - 32k-32u: LE and BE
>
> I usually use qemu VMs built with yocto for testing but I don't see
> any way of creating a 32-bit user-space with 64-bit kernel. Any ideas
> on how to prepare a testing environment?

In my case it's very easy. I do
- compile kernel as 64-bit separately;
- compile initramfs of Buildroot distro with external kernel build provided.

That's setup in which I observed the issue.

--
With Best Regards,
Andy Shevchenko

2019-12-06 21:22:34

by Bartosz Golaszewski

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

czw., 5 gru 2019 o 18:02 Andy Shevchenko <[email protected]> napisał(a):
>
> On Thu, Dec 5, 2019 at 3:47 PM Bartosz Golaszewski <[email protected]> wrote:
> > czw., 5 gru 2019 o 11:27 Andy Shevchenko <[email protected]> napisał(a):
> > > On Thu, Dec 5, 2019 at 11:42 AM Bartosz Golaszewski
> > > <[email protected]> wrote:
> > > > śr., 4 gru 2019 o 23:34 Andy Shevchenko <[email protected]> napisał(a):
> > > > > On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:
>
> > > So, the test cases, I can imagine of, should include (k - kernel, u - user):
> > > - 64k-64u: LE and BE
> > > - 64k-32u: LE and BE
> > > - 32k-32u: LE and BE
> >
> > I usually use qemu VMs built with yocto for testing but I don't see
> > any way of creating a 32-bit user-space with 64-bit kernel. Any ideas
> > on how to prepare a testing environment?
>
> In my case it's very easy. I do
> - compile kernel as 64-bit separately;
> - compile initramfs of Buildroot distro with external kernel build provided.
>

Any specific config options are needed on x86-64 kernel to use 32-bit
user-space? I'm not well versed in x86 architectures, that's why I'm
asking. I built a 32-bit userspace qemu image with yocto and then
manually built a 64-bit kernel. I tried running it but I'm getting a
kernel panic when the rootfs is being mounted.

On a different note: why would endianness be an issue here? 32-bit
variables with 64-bit alignment should still be in the same place in
memory, right?

Any reason not to use __packed for this structure and not deal with
this whole compat mess?

I also noticed that my change will only allow user-space to read one
event at a time which seems to be a regression with regard to the
current implementation. I probably need to address this too.

Bart

> That's setup in which I observed the issue.
>
> --
> With Best Regards,
> Andy Shevchenko

2019-12-10 17:02:23

by Andy Shevchenko

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

On Fri, Dec 6, 2019 at 11:20 PM Bartosz Golaszewski <[email protected]> wrote:
> czw., 5 gru 2019 o 18:02 Andy Shevchenko <[email protected]> napisał(a):
> > On Thu, Dec 5, 2019 at 3:47 PM Bartosz Golaszewski <[email protected]> wrote:
> > > czw., 5 gru 2019 o 11:27 Andy Shevchenko <[email protected]> napisał(a):
> > > > On Thu, Dec 5, 2019 at 11:42 AM Bartosz Golaszewski
> > > > <[email protected]> wrote:
> > > > > śr., 4 gru 2019 o 23:34 Andy Shevchenko <[email protected]> napisał(a):
> > > > > > On Wed, Dec 4, 2019 at 6:03 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > > > So, the test cases, I can imagine of, should include (k - kernel, u - user):
> > > > - 64k-64u: LE and BE
> > > > - 64k-32u: LE and BE
> > > > - 32k-32u: LE and BE
> > >
> > > I usually use qemu VMs built with yocto for testing but I don't see
> > > any way of creating a 32-bit user-space with 64-bit kernel. Any ideas
> > > on how to prepare a testing environment?
> >
> > In my case it's very easy. I do
> > - compile kernel as 64-bit separately;
> > - compile initramfs of Buildroot distro with external kernel build provided.
> >
>
> Any specific config options are needed on x86-64 kernel to use 32-bit
> user-space? I'm not well versed in x86 architectures, that's why I'm
> asking. I built a 32-bit userspace qemu image with yocto and then
> manually built a 64-bit kernel. I tried running it but I'm getting a
> kernel panic when the rootfs is being mounted.

Just published set of scripts [1] we are using internally for our development.
Find README.coreteam in the source root and read how to use that.

> On a different note: why would endianness be an issue here? 32-bit
> variables with 64-bit alignment should still be in the same place in
> memory, right?

With explicit padding, yes.

> Any reason not to use __packed for this structure and not deal with
> this whole compat mess?

Have been suggested that explicit padding is better approach.
(See my answer to Kent)

> I also noticed that my change will only allow user-space to read one
> event at a time which seems to be a regression with regard to the
> current implementation. I probably need to address this too.

Yes, but we have to have ABI v2 in place.

[1]: https://github.com/andy-shev/buildroot/tree/intel

--
With Best Regards,
Andy Shevchenko

2019-12-19 13:08:24

by Bartosz Golaszewski

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

wt., 10 gru 2019 o 18:00 Andy Shevchenko <[email protected]> napisał(a):
>
> > On a different note: why would endianness be an issue here? 32-bit
> > variables with 64-bit alignment should still be in the same place in
> > memory, right?
>
> With explicit padding, yes.
>
> > Any reason not to use __packed for this structure and not deal with
> > this whole compat mess?
>
> Have been suggested that explicit padding is better approach.
> (See my answer to Kent)
>
> > I also noticed that my change will only allow user-space to read one
> > event at a time which seems to be a regression with regard to the
> > current implementation. I probably need to address this too.
>
> Yes, but we have to have ABI v2 in place.

Hi Andy,

I was playing with some ideas for the new ABI and noticed that on
64-bit architecture the size of struct gpiochip_info is reported to be
68 bytes, not 72 as I would expect. Is implicit alignment padding not
applied to a struct if there's a non-64bit-aligned 32-bit field at the
end of it? Is there something I'm missing here?

Bart

2019-12-19 13:15:29

by Kent Gibson

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

On Thu, Dec 19, 2019 at 02:05:19PM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 18:00 Andy Shevchenko <[email protected]> napisał(a):
> >
> > > On a different note: why would endianness be an issue here? 32-bit
> > > variables with 64-bit alignment should still be in the same place in
> > > memory, right?
> >
> > With explicit padding, yes.
> >
> > > Any reason not to use __packed for this structure and not deal with
> > > this whole compat mess?
> >
> > Have been suggested that explicit padding is better approach.
> > (See my answer to Kent)
> >
> > > I also noticed that my change will only allow user-space to read one
> > > event at a time which seems to be a regression with regard to the
> > > current implementation. I probably need to address this too.
> >
> > Yes, but we have to have ABI v2 in place.
>
> Hi Andy,
>
> I was playing with some ideas for the new ABI and noticed that on
> 64-bit architecture the size of struct gpiochip_info is reported to be
> 68 bytes, not 72 as I would expect. Is implicit alignment padding not
> applied to a struct if there's a non-64bit-aligned 32-bit field at the
> end of it? Is there something I'm missing here?
>

Struct alignment is based on the size of the largest element.
The largest element of struct gpiopchip_info is a __u32, so the struct
gets 32-bit alignment, even on 64-bit.

The structs with the problems all contain a __u64, and so get padded out
to a 64-bit boundary.

Cheers,
Kent.

2019-12-19 13:18:20

by Bartosz Golaszewski

Subject: Re: [PATCH v2 10/11] gpiolib: add new ioctl() for monitoring changes in line info

czw., 19 gru 2019 o 14:13 Kent Gibson <[email protected]> napisał(a):
>
> On Thu, Dec 19, 2019 at 02:05:19PM +0100, Bartosz Golaszewski wrote:
> > wt., 10 gru 2019 o 18:00 Andy Shevchenko <[email protected]> napisał(a):
> > >
> > > > On a different note: why would endianness be an issue here? 32-bit
> > > > variables with 64-bit alignment should still be in the same place in
> > > > memory, right?
> > >
> > > With explicit padding, yes.
> > >
> > > > Any reason not to use __packed for this structure and not deal with
> > > > this whole compat mess?
> > >
> > > Have been suggested that explicit padding is better approach.
> > > (See my answer to Kent)
> > >
> > > > I also noticed that my change will only allow user-space to read one
> > > > event at a time which seems to be a regression with regard to the
> > > > current implementation. I probably need to address this too.
> > >
> > > Yes, but we have to have ABI v2 in place.
> >
> > Hi Andy,
> >
> > I was playing with some ideas for the new ABI and noticed that on
> > 64-bit architecture the size of struct gpiochip_info is reported to be
> > 68 bytes, not 72 as I would expect. Is implicit alignment padding not
> > applied to a struct if there's a non-64bit-aligned 32-bit field at the
> > end of it? Is there something I'm missing here?
> >
>
> Struct alignment is based on the size of the largest element.
> The largest element of struct gpiopchip_info is a __u32, so the struct
> gets 32-bit alignment, even on 64-bit.
>
> The structs with the problems all contain a __u64, and so get padded out
> to a 64-bit boundary.
>

Thanks for the clarification, now it makes sense. I assumed memory
alignment depends on the architecture. I need to educate myself more
on this subject I guess.

Bart