gpio: cdev: bail out of poll() if the device goes down

PATCH In review Bartosz Golaszewski 4 participants 19 messages Aug 17, 2023 – Aug 21, 2023

Subsystems (2)

GPIO GPIO uapi

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #0 lore raw

v2 cover gpio: cdev: bail out of poll() if the device goes down

From: Bartosz Golaszewski <[email protected]>

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(-)

Signature · 1 line

2.39.2

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #1 lore raw

PATCH v2 1/6 gpiolib: rename the gpio_device notifier

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 16 ++++++++--------
 drivers/gpio/gpiolib.c      |  6 +++---
 drivers/gpio/gpiolib.h      |  6 +++---
 3 files changed, 14 insertions(+), 14 deletions(-)

drivers/gpio/gpiolib-cdev.c +8 -8

index 0a33971c964c..9ee8604f32e1 100644
@@ -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);

drivers/gpio/gpiolib.c +3 -3

index 76e0c38026c3..769ca2333b7a 100644
@@ -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;

drivers/gpio/gpiolib.h +3 -3

index cca81375f127..7dd06ef45d44 100644
@@ -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

Signature · 1 line

2.39.2

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #2 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

drivers/gpio/gpiolib-cdev.c +3 -7

index 9ee8604f32e1..df30d17f9e29 100644
@@ -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;

Signature · 1 line

2.39.2

Andy Shevchenko

2023-08-18 10:29 UTC #3 lore raw

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

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

...

drivers/gpio/gpiolib-cdev.c static long gpio_ioctl_compat(struct file *file, unsigned int cmd, · line 2499

Quoted 3 lines

drivers/gpio/gpiolib-cdev.c +3 -0

+	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);

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #4 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 1 +
 drivers/gpio/gpiolib.c      | 1 +
 drivers/gpio/gpiolib.h      | 3 +++
 3 files changed, 5 insertions(+)

drivers/gpio/gpiolib-cdev.c +1 -0

index df30d17f9e29..f95e1b110311 100644
@@ -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);
 }

drivers/gpio/gpiolib.c +1 -0

index 769ca2333b7a..0737952882cd 100644
@@ -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

drivers/gpio/gpiolib.h +3 -0

index 7dd06ef45d44..54012605b4a4 100644
@@ -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

Signature · 1 line

2.39.2

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #5 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

drivers/gpio/gpiolib-cdev.c +27 -2

index f95e1b110311..8b081c9b8d27 100644
@@ -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);

Signature · 1 line

2.39.2

Andy Shevchenko

2023-08-18 10:34 UTC #6 lore raw

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

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

...

drivers/gpio/gpiolib-cdev.c static int lineinfo_changed_notify(struct notifier_block *nb, · line 2527

Quoted 3 lines

drivers/gpio/gpiolib-cdev.c +3 -0

+	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);

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-21 12:43 UTC #7 lore raw

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

Quoted 12 lines · Andy Shevchenko

Quoted 7 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
...

Quoted 8 lines

drivers/gpio/gpiolib-cdev.c static int lineinfo_changed_notify(struct notifier_block *nb, · line 2527
Quoted 3 lines
drivers/gpio/gpiolib-cdev.c +3 -0
+     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

Bartosz Golaszewski

author

2023-08-21 13:04 UTC #8 lore raw

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

Quoted 22 lines · Bartosz Golaszewski

Quoted 19 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
...
drivers/gpio/gpiolib-cdev.c static int lineinfo_changed_notify(struct notifier_block *nb, · line 2527
Quoted 3 lines
drivers/gpio/gpiolib-cdev.c +3 -0
+     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);

Quoted 7 lines

Quoted 1 line
?
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

Bartosz Golaszewski

author

2023-08-21 13:34 UTC #9 lore raw

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

In reply to Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

Quoted 7 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.

Quoted 9 lines

...
drivers/gpio/gpiolib-cdev.c static int lineinfo_changed_notify(struct notifier_block *nb, · line 2527
Quoted 3 lines
drivers/gpio/gpiolib-cdev.c +3 -0
+     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

Quoted 7 lines

?
--
With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #10 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

drivers/gpio/gpiolib-cdev.c +23 -0

index 8b081c9b8d27..4c3b186bc638 100644
@@ -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;

Signature · 1 line

2.39.2

Bartosz Golaszewski

author

2023-08-17 18:51 UTC #11 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/gpio/gpiolib-cdev.c +22 -0

index 4c3b186bc638..76a71471c724 100644
@@ -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,

Signature · 1 line

2.39.2

Kent Gibson

reviewer Reviewed-by on the v2 series

2023-08-18 00:17 UTC #12 lore raw

In reply to Bartosz Golaszewski

Quoted 21 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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

Quoted 5 lines

 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 <[email protected]>

Cheers,
Kent.

Linus Walleij

Reviewed-by on the v2 series

2023-08-18 07:21 UTC #13 lore raw

In reply to Bartosz Golaszewski

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 4 lines

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 <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

2023-08-18 10:35 UTC #14 lore raw

In reply to Bartosz Golaszewski

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-18 12:07 UTC #15 lore raw

Quoted 4 lines · Andy Shevchenko

Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>

Quoted 9 lines

Quoted 5 lines
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

Andy Shevchenko

2023-08-18 13:30 UTC #16 lore raw

Quoted 12 lines · Bartosz Golaszewski

Quoted 10 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.

Quoted 6 lines

Quoted 2 lines
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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-18 13:37 UTC #17 lore raw

Quoted 21 lines · Andy Shevchenko

Quoted 12 lines · Bartosz Golaszewski
Quoted 10 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 7 lines

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

Bartosz Golaszewski

author

2023-08-18 19:17 UTC #18 lore raw

Quoted 32 lines · Bartosz Golaszewski

Quoted 21 lines · Andy Shevchenko
Quoted 12 lines · Bartosz Golaszewski
Quoted 10 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 7 lines
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.

Quoted 7 lines

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