Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 27 Nov 2019 23:24:10 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Bartosz Golaszewski <brgl@bgdev.pl>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [PATCH 7/8] gpiolib: add new ioctl() for monitoring changes in
 line info
Message-ID: <20191127152410.GA24936@sol>
References: <20191127133510.10614-1-brgl@bgdev.pl>
 <20191127133510.10614-8-brgl@bgdev.pl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191127133510.10614-8-brgl@bgdev.pl>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 27, 2019 at 02:35:09PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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 retrieve
> a file-descriptor for given GPIO lines which can be polled for line status
> change events.
> 
> 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 <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c    | 218 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpio/gpiolib.h    |   1 +
>  include/uapi/linux/gpio.h |  36 +++++++
>  3 files changed, 255 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d094b1be334d..be5df4bdf44b 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;
>  }
> @@ -1148,6 +1151,212 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>  	return ret;
>  }
>  
> +struct linechanged_fd_state {
> +	struct gpio_device *gdev;
> +	struct gpio_desc *descs[GPIOHANDLES_MAX];
> +	size_t numdescs;
> +	wait_queue_head_t waitqueue;
> +	DECLARE_KFIFO(events, struct gpioline_changed, 16);
> +	struct mutex lock;
> +	struct notifier_block changed_nb;
> +};
> +
> +static int linechanged_fd_release(struct inode *inode, struct file *filep)
> +{
> +	struct linechanged_fd_state *lc_state = filep->private_data;
> +
> +	atomic_notifier_chain_unregister(&lc_state->gdev->notifier,
> +					 &lc_state->changed_nb);
> +	put_device(&lc_state->gdev->dev);
> +	kfree(lc_state);
> +
> +	return 0;
> +}
> +
> +static __poll_t linechanged_fd_poll(struct file *filep,
> +				    struct poll_table_struct *pollt)
> +{
> +	struct linechanged_fd_state *lc_state = filep->private_data;
> +	__poll_t events = 0;
> +
> +	poll_wait(filep, &lc_state->waitqueue, pollt);
> +
> +	mutex_lock(&lc_state->lock);
> +	if (!kfifo_is_empty(&lc_state->events))
> +		events = EPOLLIN | EPOLLRDNORM;
> +	mutex_unlock(&lc_state->lock);
> +
> +	return events;
> +}
> +
> +static ssize_t linechanged_fd_read(struct file *filep, char __user *buf,
> +				   size_t count, loff_t *off)
> +{
> +	struct linechanged_fd_state *lc_state = filep->private_data;
> +	unsigned int copied;
> +	int ret;
> +
> +	if (count < sizeof(struct gpioline_changed))
> +		return -EINVAL;
> +
> +	do {
> +		mutex_lock(&lc_state->lock);
> +		if (kfifo_is_empty(&lc_state->events)) {
> +			mutex_unlock(&lc_state->lock);
> +			if (filep->f_flags & O_NONBLOCK)
> +				return -EAGAIN;
> +
> +			ret = wait_event_interruptible(lc_state->waitqueue,
> +					!kfifo_is_empty(&lc_state->events));
> +			if (ret)
> +				return ret;
> +		} else {
> +			mutex_unlock(&lc_state->lock);
> +		}
> +
> +		if (mutex_lock_interruptible(&lc_state->lock))
> +			return -ERESTARTSYS;
> +
> +		ret = kfifo_to_user(&lc_state->events, buf, count, &copied);
> +		mutex_unlock(&lc_state->lock);
> +		if (ret)
> +			return ret;
> +
> +		if (copied == 0 && (filep->f_flags & O_NONBLOCK))
> +			return -EAGAIN;
> +	} while (copied == 0);
> +
> +	return copied;
> +}
> +
> +static const struct file_operations linechanged_fd_fileops = {
> +	.release = linechanged_fd_release,
> +	.owner = THIS_MODULE,
> +	.llseek = noop_llseek,
> +	.poll = linechanged_fd_poll,
> +	.read = linechanged_fd_read,
> +};
> +
> +static struct linechanged_fd_state *
> +to_linechanged_fd_state(struct notifier_block *nb)
> +{
> +	return container_of(nb, struct linechanged_fd_state, changed_nb);
> +}
> +
> +static int linechanged_fd_notify(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct linechanged_fd_state *lc_state = to_linechanged_fd_state(nb);
> +	struct gpio_desc *desc = data;
> +	struct gpioline_changed chg;
> +	int i, ret;
> +
> +	for (i = 0; i < lc_state->numdescs; i++) {
> +		/* Are we watching this desc? */
> +		if (desc == lc_state->descs[i]) {
> +			/* Yes - prepare the event. */
> +			memset(&chg, 0, sizeof(chg));
> +			chg.line_offset = gpio_chip_hwgpio(desc);
> +			chg.event_type = action;
> +
> +			mutex_lock(&lc_state->lock);
> +			ret = kfifo_put(&lc_state->events, chg);
> +			mutex_unlock(&lc_state->lock);
> +			if (ret)
> +				wake_up_poll(&lc_state->waitqueue, EPOLLIN);
> +
> +			return NOTIFY_OK;
> +		}
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int linechanged_fd_create(struct gpio_device *gdev, void __user *ip)
> +{
> +	struct gpioline_changed_fd_request changed_req;
> +	struct linechanged_fd_state *lc_state;
> +	struct gpio_desc *desc;
> +	struct file *file;
> +	int ret, i, fd;
> +	u32 offset;
> +
> +	ret = copy_from_user(&changed_req, ip, sizeof(changed_req));
> +	if (ret)
> +		return -EFAULT;
> +
> +	if ((changed_req.num_lines == 0) ||
> +	    (changed_req.num_lines > GPIOHANDLES_MAX))
> +		return -EINVAL;
> +
> +	lc_state = kzalloc(sizeof(*lc_state), GFP_KERNEL);
> +	if (!lc_state)
> +		return -ENOMEM;
> +
> +	lc_state->gdev = gdev;
> +	get_device(&gdev->dev);
> +
> +	for (i = 0; i < changed_req.num_lines; i++) {
> +		offset = changed_req.lineoffsets[i];
> +		desc = gpiochip_get_desc(gdev->chip, offset);
> +		if (IS_ERR(desc)) {
> +			ret = PTR_ERR(desc);
> +			goto out_free_lc_state;
> +		}
> +
> +		lc_state->descs[i] = desc;
> +	}
> +
> +	lc_state->numdescs = changed_req.num_lines;
> +
> +	init_waitqueue_head(&lc_state->waitqueue);
> +	INIT_KFIFO(lc_state->events);
> +	mutex_init(&lc_state->lock);
> +
> +	lc_state->changed_nb.notifier_call = linechanged_fd_notify;
> +
> +	ret = atomic_notifier_chain_register(&gdev->notifier,
> +					     &lc_state->changed_nb);
> +	if (ret)
> +		goto out_free_lc_state;
> +
> +	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> +	if (fd < 0) {
> +		ret = fd;
> +		goto out_unregister_notifier;
> +	}
> +
> +	file = anon_inode_getfile("gpio-line-changed-fd",
> +				  &linechanged_fd_fileops,
> +				  lc_state, O_RDONLY | O_CLOEXEC);
> +	if (IS_ERR(file)) {
> +		ret = PTR_ERR(file);
> +		goto out_put_unused_fd;
> +	}
> +
> +	changed_req.fd = fd;
> +	ret = copy_to_user(ip, &changed_req, sizeof(changed_req));
> +	if (ret) {
> +		fput(file);
> +		put_unused_fd(fd);
> +		return -EFAULT;
> +	}
> +
> +	fd_install(fd, file);
> +
> +	return 0;
> +
> +out_put_unused_fd:
> +	put_unused_fd(fd);
> +out_unregister_notifier:
> +	atomic_notifier_chain_unregister(&gdev->notifier,
> +					 &lc_state->changed_nb);
> +out_free_lc_state:
> +	kfree(lc_state);
> +
> +	return ret;
> +}
> +
>  /*
>   * gpio_ioctl() - ioctl handler for the GPIO chardev
>   */
> @@ -1238,6 +1447,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		return linehandle_create(gdev, ip);
>  	} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
>  		return lineevent_create(gdev, ip);
> +	} else if (cmd == GPIO_GET_LINECHANGED_FD_IOCTL) {
> +		return linechanged_fd_create(gdev, ip);
>  	}
>  	return -EINVAL;
>  }
> @@ -1499,6 +1710,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
> @@ -2837,6 +3050,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
>  		spin_lock_irqsave(&gpio_lock, flags);
>  	}
>  done:
> +	atomic_notifier_call_chain(&desc->gdev->notifier,
> +				   GPIOLINE_CHANGED_REQUESTED, desc);
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  	return ret;
>  }
> @@ -2934,6 +3149,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>  		ret = true;
>  	}
>  
> +	atomic_notifier_call_chain(&desc->gdev->notifier,
> +				   GPIOLINE_CHANGED_RELEASED, desc);
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  	return ret;
>  }
> @@ -3097,6 +3314,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..8e3969616cfe 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
>  	/*
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 799cf823d493..c61429467dd4 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -59,6 +59,40 @@ struct gpioline_info {
>  /* Maximum number of requested handles */
>  #define GPIOHANDLES_MAX 64
>  
> +/**
> + * struct gpioline_changed_fd_request - Information about a linechanged fd
> + * request
> + * @lineoffsets: an array of desired lines, specified by offset index for the
> + * associated GPIO device
> + * @num_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
> + */
> +struct gpioline_changed_fd_request {
> +	__u32 lineoffsets[GPIOHANDLES_MAX];
> +	__u32 num_lines;
> +	int fd;
> +};
> +

Wouldn't the most common case be to watch all the lines on a chip?
How about an easy way to do that, say num_lines=0?

> +/* Possible line status change events */
> +enum {
> +	GPIOLINE_CHANGED_REQUESTED = 1,
> +	GPIOLINE_CHANGED_RELEASED,
> +	GPIOLINE_CHANGED_CONFIG,
> +};
> +
> +/**
> + * struct gpioline_changed - Information about a change in status
> + * of a GPIO line
> + * @line_offset: offset of the line that changed relative to the gpiochip
> + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> + * and GPIOLINE_CHANGED_CONFIG
> + */
> +struct gpioline_changed {
> +	__u32 line_offset;
> +	__u32 event_type;
> +};
> +

Rather than sending an event type, and requiring userspace to poll
LINEINFO, which is racy, how about passing the updated info flags here?
A change in the state of the GPIOLINE_FLAG_KERNEL implies the
GPIOLINE_CHANGED_REQUESTED or GPIOLINE_CHANGED_RELEASED, so the
event_type is then redundant.
Userspace would then only have to poll LINEINFO if they were interested
in the consumer on GPIOLINE_CHANGED_REQUESTED.

To sync kernel and userspace state the current state of each line 
should be returned immediately via the fd as soon as the fd is created,
and then subsequently on any change.

And a timestamp might be useful, as per gpioevent_data?

Kent.

>  /* Linerequest flags */
>  #define GPIOHANDLE_REQUEST_INPUT	(1UL << 0)
>  #define GPIOHANDLE_REQUEST_OUTPUT	(1UL << 1)
> @@ -176,6 +210,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_LINECHANGED_FD_IOCTL \
> +		_IOWR(0xB4, 0x0b, struct gpioline_changed_fd_request)
>  #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
>