[PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Wake up all three wake queues (the one associated with the character
device file, the one for V1 line events and the V2 line request one)
when the underlying GPIO device is unregistered. This way we won't get
stuck in poll() after the chip is gone as user-space will be forced to
go back into a new system call and will see that gdev->chip is NULL.

v1 -> v2:
- not much is left from v1, this time we don't repurpose the existing
  gpio_device notifier but add a new one so that cdev structures don't
  get unwanted events

Bartosz Golaszewski (6):
  gpiolib: rename the gpio_device notifier
  gpio: cdev: open-code to_gpio_chardev_data()
  gpiolib: add a second blocking notifier to struct gpio_device
  gpio: cdev: wake up chardev poll() on device unbind
  gpio: cdev: wake up linereq poll() on device unbind
  gpio: cdev: wake up lineevent poll() on device unbind

 drivers/gpio/gpiolib-cdev.c | 101 ++++++++++++++++++++++++++++++------
 drivers/gpio/gpiolib.c      |   7 +--
 drivers/gpio/gpiolib.h      |   9 ++--
 3 files changed, 94 insertions(+), 23 deletions(-)

-- 
2.39.2

[PATCH v2 1/6] gpiolib: rename the gpio_device notifier

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Change the generic "notifier" name to "line_state_notifier" in order to
reflect its purpose in preparation for adding a second notifier which
will be used to notify wait queues about device unregistering.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 16 ++++++++--------
 drivers/gpio/gpiolib.c      |  6 +++---
 drivers/gpio/gpiolib.h      |  6 +++---
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0a33971c964c..9ee8604f32e1 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -230,7 +230,7 @@ static long linehandle_set_config(struct linehandle_state *lh,
 				return ret;
 		}
 
-		blocking_notifier_call_chain(&desc->gdev->notifier,
+		blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 					     GPIO_V2_LINE_CHANGED_CONFIG,
 					     desc);
 	}
@@ -414,7 +414,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 				goto out_free_lh;
 		}
 
-		blocking_notifier_call_chain(&desc->gdev->notifier,
+		blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 					     GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
 		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
@@ -1407,7 +1407,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
 
 		WRITE_ONCE(line->edflags, edflags);
 
-		blocking_notifier_call_chain(&desc->gdev->notifier,
+		blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 					     GPIO_V2_LINE_CHANGED_CONFIG,
 					     desc);
 	}
@@ -1720,7 +1720,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 
 		lr->lines[i].edflags = edflags;
 
-		blocking_notifier_call_chain(&desc->gdev->notifier,
+		blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 					     GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
 		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
@@ -2117,7 +2117,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	if (ret)
 		goto out_free_le;
 
-	blocking_notifier_call_chain(&desc->gdev->notifier,
+	blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 				     GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
 	irq = gpiod_to_irq(desc);
@@ -2671,7 +2671,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	cdev->gdev = gpio_device_get(gdev);
 
 	cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
-	ret = blocking_notifier_chain_register(&gdev->notifier,
+	ret = blocking_notifier_chain_register(&gdev->line_state_notifier,
 					       &cdev->lineinfo_changed_nb);
 	if (ret)
 		goto out_free_bitmap;
@@ -2687,7 +2687,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	return ret;
 
 out_unregister_notifier:
-	blocking_notifier_chain_unregister(&gdev->notifier,
+	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
 					   &cdev->lineinfo_changed_nb);
 out_free_bitmap:
 	gpio_device_put(gdev);
@@ -2711,7 +2711,7 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
 	struct gpio_device *gdev = cdev->gdev;
 
 	bitmap_free(cdev->watched_lines);
-	blocking_notifier_chain_unregister(&gdev->notifier,
+	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
 					   &cdev->lineinfo_changed_nb);
 	gpio_device_put(gdev);
 	kfree(cdev);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 76e0c38026c3..769ca2333b7a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -841,7 +841,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	init_rwsem(&gdev->sem);
 
 #ifdef CONFIG_PINCTRL
@@ -2159,7 +2159,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 	}
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	blocking_notifier_call_chain(&desc->gdev->notifier,
+	blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 				     GPIOLINE_CHANGED_RELEASED, desc);
 
 	return ret;
@@ -3995,7 +3995,7 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 		return ERR_PTR(ret);
 	}
 
-	blocking_notifier_call_chain(&desc->gdev->notifier,
+	blocking_notifier_call_chain(&desc->gdev->line_state_notifier,
 				     GPIOLINE_CHANGED_REQUESTED, desc);
 
 	return desc;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index cca81375f127..7dd06ef45d44 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -38,8 +38,8 @@
  * or name of the IP component in a System on Chip.
  * @data: per-instance data assigned by the driver
  * @list: links gpio_device:s together for traversal
- * @notifier: used to notify subscribers about lines being requested, released
- *            or reconfigured
+ * @line_state_notifier: used to notify subscribers about lines being
+ *                       requested, released or reconfigured
  * @sem: protects the structure from a NULL-pointer dereference of @chip by
  *       user-space operations when the device gets unregistered during
  *       a hot-unplug event
@@ -63,7 +63,7 @@ struct gpio_device {
 	const char		*label;
 	void			*data;
 	struct list_head        list;
-	struct blocking_notifier_head notifier;
+	struct blocking_notifier_head line_state_notifier;
 	struct rw_semaphore	sem;
 
 #ifdef CONFIG_PINCTRL
-- 
2.39.2

[PATCH v2 2/6] gpio: cdev: open-code to_gpio_chardev_data()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This function is a wrapper around container_of(). It's used only once and
we will have a second notifier soon, so instead of having two flavors of
this helper, let's just open-code where needed.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9ee8604f32e1..df30d17f9e29 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2491,16 +2491,12 @@ static long gpio_ioctl_compat(struct file *file, 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 *cdev = to_gpio_chardev_data(nb);
+	struct gpio_chardev_data *cdev = container_of(nb,
+						      struct gpio_chardev_data,
+						      lineinfo_changed_nb);
 	struct gpio_v2_line_info_changed chg;
 	struct gpio_desc *desc = data;
 	int ret;
-- 
2.39.2

[PATCH v2 3/6] gpiolib: add a second blocking notifier to struct gpio_device

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a new blocking notifier to struct gpio_device and use it to notify
subscribers about the GPIO device being unregistered from the device
model.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 1 +
 drivers/gpio/gpiolib.c      | 1 +
 drivers/gpio/gpiolib.h      | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index df30d17f9e29..f95e1b110311 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2749,4 +2749,5 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 void gpiolib_cdev_unregister(struct gpio_device *gdev)
 {
 	cdev_device_del(&gdev->chrdev, &gdev->dev);
+	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 769ca2333b7a..0737952882cd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -842,6 +842,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
 	init_rwsem(&gdev->sem);
 
 #ifdef CONFIG_PINCTRL
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 7dd06ef45d44..54012605b4a4 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -40,6 +40,8 @@
  * @list: links gpio_device:s together for traversal
  * @line_state_notifier: used to notify subscribers about lines being
  *                       requested, released or reconfigured
+ * @device_notifier: used to notify character device wait queues about the GPIO
+ *                   device being unregistered
  * @sem: protects the structure from a NULL-pointer dereference of @chip by
  *       user-space operations when the device gets unregistered during
  *       a hot-unplug event
@@ -64,6 +66,7 @@ struct gpio_device {
 	void			*data;
 	struct list_head        list;
 	struct blocking_notifier_head line_state_notifier;
+	struct blocking_notifier_head device_notifier;
 	struct rw_semaphore	sem;
 
 #ifdef CONFIG_PINCTRL
-- 
2.39.2

[PATCH v2 4/6] gpio: cdev: wake up chardev poll() on device unbind

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a notifier block to the gpio_chardev_data structure and register it
with the gpio_device's device notifier. Upon reception of an event, wake
up the wait queue so that the user-space be forced out of poll() and need
to go into a new system call which will then fail due to the chip being
gone.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f95e1b110311..8b081c9b8d27 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2320,6 +2320,7 @@ struct gpio_chardev_data {
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpio_v2_line_info_changed, 32);
 	struct notifier_block lineinfo_changed_nb;
+	struct notifier_block device_unregistered_nb;
 	unsigned long *watched_lines;
 #ifdef CONFIG_GPIO_CDEV_V1
 	atomic_t watch_abi_version;
@@ -2518,6 +2519,18 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static int gpio_device_unregistered_notify(struct notifier_block *nb,
+					   unsigned long action, void *data)
+{
+	struct gpio_chardev_data *cdev = container_of(nb,
+						      struct gpio_chardev_data,
+						      device_unregistered_nb);
+
+	wake_up_poll(&cdev->wait, EPOLLIN | EPOLLERR);
+
+	return NOTIFY_OK;
+}
+
 static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
 					     struct poll_table_struct *pollt)
 {
@@ -2672,17 +2685,27 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	if (ret)
 		goto out_free_bitmap;
 
+	cdev->device_unregistered_nb.notifier_call =
+					gpio_device_unregistered_notify;
+	ret = blocking_notifier_chain_register(&gdev->device_notifier,
+					       &cdev->device_unregistered_nb);
+	if (ret)
+		goto out_unregister_line_notifier;
+
 	file->private_data = cdev;
 
 	ret = nonseekable_open(inode, file);
 	if (ret)
-		goto out_unregister_notifier;
+		goto out_unregister_device_notifier;
 
 	up_read(&gdev->sem);
 
 	return ret;
 
-out_unregister_notifier:
+out_unregister_device_notifier:
+	blocking_notifier_chain_unregister(&gdev->device_notifier,
+					   &cdev->device_unregistered_nb);
+out_unregister_line_notifier:
 	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
 					   &cdev->lineinfo_changed_nb);
 out_free_bitmap:
@@ -2707,6 +2730,8 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
 	struct gpio_device *gdev = cdev->gdev;
 
 	bitmap_free(cdev->watched_lines);
+	blocking_notifier_chain_unregister(&gdev->device_notifier,
+					   &cdev->device_unregistered_nb);
 	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
 					   &cdev->lineinfo_changed_nb);
 	gpio_device_put(gdev);
-- 
2.39.2

[PATCH v2 5/6] gpio: cdev: wake up linereq poll() on device unbind

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a notifier block to the linereq structure and register it with the
gpio_device's device notifier. Upon reception of an event, wake up the
wait queue so that the user-space be forced out of poll() and need to go
into a new system call which will then fail due to the chip being gone.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8b081c9b8d27..4c3b186bc638 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -555,6 +555,7 @@ struct line {
  * @label: consumer label used to tag GPIO descriptors
  * @num_lines: the number of lines in the lines array
  * @wait: wait queue that handles blocking reads of events
+ * @device_unregistered_nb: notifier block for receiving gdev unregister events
  * @event_buffer_size: the number of elements allocated in @events
  * @events: KFIFO for the GPIO events
  * @seqno: the sequence number for edge events generated on all lines in
@@ -569,6 +570,7 @@ struct linereq {
 	const char *label;
 	u32 num_lines;
 	wait_queue_head_t wait;
+	struct notifier_block device_unregistered_nb;
 	u32 event_buffer_size;
 	DECLARE_KFIFO_PTR(events, struct gpio_v2_line_event);
 	atomic_t seqno;
@@ -610,6 +612,17 @@ struct linereq {
 	 GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \
 	 GPIO_V2_LINE_EDGE_FLAGS)
 
+static int linereq_unregistered_notify(struct notifier_block *nb,
+				       unsigned long action, void *data)
+{
+	struct linereq *lr = container_of(nb, struct linereq,
+					  device_unregistered_nb);
+
+	wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
+
+	return NOTIFY_OK;
+}
+
 static void linereq_put_event(struct linereq *lr,
 			      struct gpio_v2_line_event *le)
 {
@@ -1567,6 +1580,10 @@ static void linereq_free(struct linereq *lr)
 {
 	unsigned int i;
 
+	if (lr->device_unregistered_nb.notifier_call)
+		blocking_notifier_chain_unregister(&lr->gdev->device_notifier,
+						   &lr->device_unregistered_nb);
+
 	for (i = 0; i < lr->num_lines; i++) {
 		if (lr->lines[i].desc) {
 			edge_detector_stop(&lr->lines[i]);
@@ -1727,6 +1744,12 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 			offset);
 	}
 
+	lr->device_unregistered_nb.notifier_call = linereq_unregistered_notify;
+	ret = blocking_notifier_chain_register(&gdev->device_notifier,
+					       &lr->device_unregistered_nb);
+	if (ret)
+		goto out_free_linereq;
+
 	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
 	if (fd < 0) {
 		ret = fd;
-- 
2.39.2

[PATCH v2 6/6] gpio: cdev: wake up lineevent poll() on device unbind

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a notifier block to the lineevent_state structure and register it
with the gpio_device's device notifier. Upon reception of an event, wake
up the wait queue so that the user-space be forced out of poll() and
need to go into a new system call which will then fail due to the chip
being gone.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 4c3b186bc638..76a71471c724 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1802,6 +1802,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
  * @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
+ * @device_unregistered_nb: notifier block for receiving gdev unregister events
  * @events: KFIFO for the GPIO events
  * @timestamp: cache for the timestamp storing it between hardirq
  * and IRQ thread, used to bring the timestamp close to the actual
@@ -1814,6 +1815,7 @@ struct lineevent_state {
 	u32 eflags;
 	int irq;
 	wait_queue_head_t wait;
+	struct notifier_block device_unregistered_nb;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
 	u64 timestamp;
 };
@@ -1847,6 +1849,17 @@ static __poll_t lineevent_poll(struct file *file,
 	return call_poll_locked(file, wait, le->gdev, lineevent_poll_unlocked);
 }
 
+static int lineevent_unregistered_notify(struct notifier_block *nb,
+					 unsigned long action, void *data)
+{
+	struct lineevent_state *le = container_of(nb, struct lineevent_state,
+						  device_unregistered_nb);
+
+	wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
+
+	return NOTIFY_OK;
+}
+
 struct compat_gpioeevent_data {
 	compat_u64	timestamp;
 	u32		id;
@@ -1932,6 +1945,9 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 
 static void lineevent_free(struct lineevent_state *le)
 {
+	if (le->device_unregistered_nb.notifier_call)
+		blocking_notifier_chain_unregister(&le->gdev->device_notifier,
+						   &le->device_unregistered_nb);
 	if (le->irq)
 		free_irq(le->irq, le);
 	if (le->desc)
@@ -2160,6 +2176,12 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	INIT_KFIFO(le->events);
 	init_waitqueue_head(&le->wait);
 
+	le->device_unregistered_nb.notifier_call = lineevent_unregistered_notify;
+	ret = blocking_notifier_chain_register(&gdev->device_notifier,
+					       &le->device_unregistered_nb);
+	if (ret)
+		goto out_free_le;
+
 	/* Request a thread to read the events */
 	ret = request_threaded_irq(irq,
 				   lineevent_irq_handler,
-- 
2.39.2

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Kent Gibson]

On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Wake up all three wake queues (the one associated with the character
> device file, the one for V1 line events and the V2 line request one)
> when the underlying GPIO device is unregistered. This way we won't get
> stuck in poll() after the chip is gone as user-space will be forced to
> go back into a new system call and will see that gdev->chip is NULL.
> 
> v1 -> v2:
> - not much is left from v1, this time we don't repurpose the existing
>   gpio_device notifier but add a new one so that cdev structures don't
>   get unwanted events
> 
> Bartosz Golaszewski (6):
>   gpiolib: rename the gpio_device notifier
>   gpio: cdev: open-code to_gpio_chardev_data()
>   gpiolib: add a second blocking notifier to struct gpio_device
>   gpio: cdev: wake up chardev poll() on device unbind
>   gpio: cdev: wake up linereq poll() on device unbind
>   gpio: cdev: wake up lineevent poll() on device unbind
> 
>  drivers/gpio/gpiolib-cdev.c | 101 ++++++++++++++++++++++++++++++------
>  drivers/gpio/gpiolib.c      |   7 +--
>  drivers/gpio/gpiolib.h      |   9 ++--
>  3 files changed, 94 insertions(+), 23 deletions(-)
> 

I'm happier with this version.

Reviewed-by: Kent Gibson <warthog618@gmail.com>

Cheers,
Kent.

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Linus Walleij]

On Thu, Aug 17, 2023 at 8:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Wake up all three wake queues (the one associated with the character
> device file, the one for V1 line events and the V2 line request one)
> when the underlying GPIO device is unregistered. This way we won't get
> stuck in poll() after the chip is gone as user-space will be forced to
> go back into a new system call and will see that gdev->chip is NULL.
>
> v1 -> v2:
> - not much is left from v1, this time we don't repurpose the existing
>   gpio_device notifier but add a new one so that cdev structures don't
>   get unwanted events

This is nice, thanks for quick respin!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 2/6] gpio: cdev: open-code to_gpio_chardev_data()

[Andy Shevchenko]

On Thu, Aug 17, 2023 at 08:49:54PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> This function is a wrapper around container_of(). It's used only once and
> we will have a second notifier soon, so instead of having two flavors of
> this helper, let's just open-code where needed.

...

> +	struct gpio_chardev_data *cdev = container_of(nb,
> +						      struct gpio_chardev_data,
> +						      lineinfo_changed_nb);

This way it's slightly better.

	struct gpio_chardev_data *cdev =
		container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] gpio: cdev: wake up chardev poll() on device unbind

[Andy Shevchenko]

On Thu, Aug 17, 2023 at 08:49:56PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a notifier block to the gpio_chardev_data structure and register it
> with the gpio_device's device notifier. Upon reception of an event, wake
> up the wait queue so that the user-space be forced out of poll() and need
> to go into a new system call which will then fail due to the chip being
> gone.

...

> +	struct gpio_chardev_data *cdev = container_of(nb,
> +						      struct gpio_chardev_data,
> +						      device_unregistered_nb);

	struct gpio_chardev_data *cdev =
		container_of(nb, struct gpio_chardev_data, device_unregistered_nb);

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Andy Shevchenko]

On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Wake up all three wake queues (the one associated with the character
> device file, the one for V1 line events and the V2 line request one)
> when the underlying GPIO device is unregistered. This way we won't get
> stuck in poll() after the chip is gone as user-space will be forced to
> go back into a new system call and will see that gdev->chip is NULL.

Why can't you use the global device unbind notifications and filter out
what you are interested in?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Bartosz Golaszewski]

On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Wake up all three wake queues (the one associated with the character
> > device file, the one for V1 line events and the V2 line request one)
> > when the underlying GPIO device is unregistered. This way we won't get
> > stuck in poll() after the chip is gone as user-space will be forced to
> > go back into a new system call and will see that gdev->chip is NULL.
>
> Why can't you use the global device unbind notifications and filter out
> what you are interested in?
>

There's no truly global device unbind notification - only per-bus.
GPIO devices can reside on any bus, there are no limitations and so
we'd have to subscribe to all of them.

Bart

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 02:06:21PM +0200, Bartosz Golaszewski wrote:
> On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Wake up all three wake queues (the one associated with the character
> > > device file, the one for V1 line events and the V2 line request one)
> > > when the underlying GPIO device is unregistered. This way we won't get
> > > stuck in poll() after the chip is gone as user-space will be forced to
> > > go back into a new system call and will see that gdev->chip is NULL.
> >
> > Why can't you use the global device unbind notifications and filter out
> > what you are interested in?
> 
> There's no truly global device unbind notification - only per-bus.
> GPIO devices can reside on any bus, there are no limitations and so
> we'd have to subscribe to all of them.

We have, but it requires a bit of code patching.
Look at device_platform_notify()/device_platform_notify_remove().

I noticed, btw, that platform_notify() and Co is a dead code :-)
Maybe it can be converted to a list and a manager of that list,
so specific cases can utilize it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Bartosz Golaszewski]

On Fri, Aug 18, 2023 at 3:29 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 18, 2023 at 02:06:21PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Wake up all three wake queues (the one associated with the character
> > > > device file, the one for V1 line events and the V2 line request one)
> > > > when the underlying GPIO device is unregistered. This way we won't get
> > > > stuck in poll() after the chip is gone as user-space will be forced to
> > > > go back into a new system call and will see that gdev->chip is NULL.
> > >
> > > Why can't you use the global device unbind notifications and filter out
> > > what you are interested in?
> >
> > There's no truly global device unbind notification - only per-bus.
> > GPIO devices can reside on any bus, there are no limitations and so
> > we'd have to subscribe to all of them.
>
> We have, but it requires a bit of code patching.
> Look at device_platform_notify()/device_platform_notify_remove().
>
> I noticed, btw, that platform_notify() and Co is a dead code :-)
> Maybe it can be converted to a list and a manager of that list,
> so specific cases can utilize it.
>

That's not going to happen anytime soon and I doubt Greg would like
the idea without more users interested in receiving these events. :(

This GPIO notifier is an implementation detail - once we do have
global notifiers, we can switch to using them instead.

Bart

Re: [PATCH v2 0/6] gpio: cdev: bail out of poll() if the device goes down

[Bartosz Golaszewski]

On Fri, Aug 18, 2023 at 3:36 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Aug 18, 2023 at 3:29 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Aug 18, 2023 at 02:06:21PM +0200, Bartosz Golaszewski wrote:
> > > On Fri, Aug 18, 2023 at 12:34 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Thu, Aug 17, 2023 at 08:49:52PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > Wake up all three wake queues (the one associated with the character
> > > > > device file, the one for V1 line events and the V2 line request one)
> > > > > when the underlying GPIO device is unregistered. This way we won't get
> > > > > stuck in poll() after the chip is gone as user-space will be forced to
> > > > > go back into a new system call and will see that gdev->chip is NULL.
> > > >
> > > > Why can't you use the global device unbind notifications and filter out
> > > > what you are interested in?
> > >
> > > There's no truly global device unbind notification - only per-bus.
> > > GPIO devices can reside on any bus, there are no limitations and so
> > > we'd have to subscribe to all of them.
> >
> > We have, but it requires a bit of code patching.
> > Look at device_platform_notify()/device_platform_notify_remove().
> >
> > I noticed, btw, that platform_notify() and Co is a dead code :-)
> > Maybe it can be converted to a list and a manager of that list,
> > so specific cases can utilize it.
> >
>
> That's not going to happen anytime soon and I doubt Greg would like
> the idea without more users interested in receiving these events. :(
>
> This GPIO notifier is an implementation detail - once we do have
> global notifiers, we can switch to using them instead.
>
> Bart

Seems to me the whole platform_notify() thing was meant for a
different purpose. There's no trace for it ever being used for
notifying users about driver bind and unbind events.

It appears to me too that adding a global notifier would be as simple
as extending the functionality of bus_notify() with a second, global
notifier chain. But I won't be the one to propose this to Greg KH. :)

Bart

Re: [PATCH v2 4/6] gpio: cdev: wake up chardev poll() on device unbind

[Bartosz Golaszewski]

On Fri, Aug 18, 2023 at 12:31 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 17, 2023 at 08:49:56PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Add a notifier block to the gpio_chardev_data structure and register it
> > with the gpio_device's device notifier. Upon reception of an event, wake
> > up the wait queue so that the user-space be forced out of poll() and need
> > to go into a new system call which will then fail due to the chip being
> > gone.
>
> ...
>
> > +     struct gpio_chardev_data *cdev = container_of(nb,
> > +                                                   struct gpio_chardev_data,
> > +                                                   device_unregistered_nb);
>
>         struct gpio_chardev_data *cdev =
>                 container_of(nb, struct gpio_chardev_data, device_unregistered_nb);
>
> ?

I could live with the other version but sure, why not.

I will send a v3 with a helper wrapper around
blocking_notifier_call_chain() for more brevity.

Bart

Re: [PATCH v2 4/6] gpio: cdev: wake up chardev poll() on device unbind

[Bartosz Golaszewski]

On Mon, Aug 21, 2023 at 2:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Aug 18, 2023 at 12:31 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Aug 17, 2023 at 08:49:56PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Add a notifier block to the gpio_chardev_data structure and register it
> > > with the gpio_device's device notifier. Upon reception of an event, wake
> > > up the wait queue so that the user-space be forced out of poll() and need
> > > to go into a new system call which will then fail due to the chip being
> > > gone.
> >
> > ...
> >
> > > +     struct gpio_chardev_data *cdev = container_of(nb,
> > > +                                                   struct gpio_chardev_data,
> > > +                                                   device_unregistered_nb);
> >
> >         struct gpio_chardev_data *cdev =
> >                 container_of(nb, struct gpio_chardev_data, device_unregistered_nb);
> >
> > ?
>
> I could live with the other version but sure, why not.
>
> I will send a v3 with a helper wrapper around
> blocking_notifier_call_chain() for more brevity.
>

Scratch that, I need coffee. This was supposed to go under the line
state notifications patch.

Bart

Re: [PATCH v2 4/6] gpio: cdev: wake up chardev poll() on device unbind

[Bartosz Golaszewski]

On Fri, Aug 18, 2023 at 12:31 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 17, 2023 at 08:49:56PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Add a notifier block to the gpio_chardev_data structure and register it
> > with the gpio_device's device notifier. Upon reception of an event, wake
> > up the wait queue so that the user-space be forced out of poll() and need
> > to go into a new system call which will then fail due to the chip being
> > gone.
>
> ...
>
> > +     struct gpio_chardev_data *cdev = container_of(nb,
> > +                                                   struct gpio_chardev_data,
> > +                                                   device_unregistered_nb);
>
>         struct gpio_chardev_data *cdev =
>                 container_of(nb, struct gpio_chardev_data, device_unregistered_nb);
>

Ah, this goes over 80 chars. I will leave it as it is, I will fix the
other one when applying.

Bart

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