Skip to contentSkip to conversation

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

PATCH Superseded Bartosz Golaszewski 4 participants 13 messages Aug 16, 2023 – Aug 17, 2023

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

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.

Bartosz Golaszewski (5):
gpio: cdev: ignore notifications other than line status changes
gpio: cdev: rename the notifier block and notify callback
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 | 127 +++++++++++++++++++++++++++++-------
 drivers/gpio/gpiolib.h      |   3 +-
 2 files changed, 105 insertions(+), 25 deletions(-)
Signature · 1 line
2.39.2
#1 lore raw

PATCH 1/5 gpio: cdev: ignore notifications other than line status changes

From: Bartosz Golaszewski <[email protected]>

In preparation for extending the role of the GPIO device's blocking notifier, make sure the callback used by the character device's file descriptor data checks the action argument and only reacts to one of the line state change values. Also: relax the kerneldoc describing the notifier as it will have more responsibilities soon.

---
 drivers/gpio/gpiolib-cdev.c | 35 +++++++++++++++++++++++------------
 drivers/gpio/gpiolib.h      |  3 +--
 2 files changed, 24 insertions(+), 14 deletions(-)
drivers/gpio/gpiolib-cdev.c +23 -12
index 0a33971c964c..062521e1a9e0 100644
@@ -2502,22 +2502,33 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 {
 	struct gpio_chardev_data *cdev = to_gpio_chardev_data(nb);
 	struct gpio_v2_line_info_changed chg;
-	struct gpio_desc *desc = data;
+	struct gpio_desc *desc;
 	int ret;
 
-	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
-		return NOTIFY_DONE;
+	switch (action) {
+	case GPIO_V2_LINE_CHANGED_REQUESTED:
+	case GPIO_V2_LINE_CHANGED_RELEASED:
+	case GPIO_V2_LINE_CHANGED_CONFIG:
+		desc = data;
 
-	memset(&chg, 0, sizeof(chg));
-	chg.event_type = action;
-	chg.timestamp_ns = ktime_get_ns();
-	gpio_desc_to_lineinfo(desc, &chg.info);
+		if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
+			return NOTIFY_DONE;
 
-	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
-	if (ret)
-		wake_up_poll(&cdev->wait, EPOLLIN);
-	else
-		pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
+		memset(&chg, 0, sizeof(chg));
+		chg.event_type = action;
+		chg.timestamp_ns = ktime_get_ns();
+		gpio_desc_to_lineinfo(desc, &chg.info);
+
+		ret = kfifo_in_spinlocked(&cdev->events, &chg, 1,
+					  &cdev->wait.lock);
+		if (ret)
+			wake_up_poll(&cdev->wait, EPOLLIN);
+		else
+			pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
 
 	return NOTIFY_OK;
 }
drivers/gpio/gpiolib.h +1 -2
index cca81375f127..de7b3b60f7ca 100644
@@ -38,8 +38,7 @@
  * 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
+ * @notifier: used to notify subscribers about gpio_device events
  * @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
Signature · 1 line
2.39.2
#2 lore raw

PATCH 2/5 gpio: cdev: rename the notifier block and notify callback

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Rename the notifier block in struct gpio_chardev_data and its callback to a more generic name as it will soon gain more functionality than only handling line state changes.

---
 drivers/gpio/gpiolib-cdev.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpiolib-cdev.c +8 -11
index 062521e1a9e0..660d2e057451 100644
@@ -2319,7 +2319,7 @@ struct gpio_chardev_data {
 	struct gpio_device *gdev;
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpio_v2_line_info_changed, 32);
-	struct notifier_block lineinfo_changed_nb;
+	struct notifier_block nb;
 	unsigned long *watched_lines;
 #ifdef CONFIG_GPIO_CDEV_V1
 	atomic_t watch_abi_version;
@@ -2494,11 +2494,11 @@ static long gpio_ioctl_compat(struct file *file, unsigned int cmd,
 static struct gpio_chardev_data *
 to_gpio_chardev_data(struct notifier_block *nb)
 {
-	return container_of(nb, struct gpio_chardev_data, lineinfo_changed_nb);
+	return container_of(nb, struct gpio_chardev_data, nb);
 }
 
-static int lineinfo_changed_notify(struct notifier_block *nb,
-				   unsigned long action, void *data)
+static int gpio_chardev_notify(struct notifier_block *nb, unsigned long action,
+			       void *data)
 {
 	struct gpio_chardev_data *cdev = to_gpio_chardev_data(nb);
 	struct gpio_v2_line_info_changed chg;
@@ -2681,9 +2681,8 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	INIT_KFIFO(cdev->events);
 	cdev->gdev = gpio_device_get(gdev);
 
-	cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
-	ret = blocking_notifier_chain_register(&gdev->notifier,
-					       &cdev->lineinfo_changed_nb);
+	cdev->nb.notifier_call = gpio_chardev_notify;
+	ret = blocking_notifier_chain_register(&gdev->notifier, &cdev->nb);
 	if (ret)
 		goto out_free_bitmap;
 
@@ -2698,8 +2697,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	return ret;
 
 out_unregister_notifier:
-	blocking_notifier_chain_unregister(&gdev->notifier,
-					   &cdev->lineinfo_changed_nb);
+	blocking_notifier_chain_unregister(&gdev->notifier, &cdev->nb);
 out_free_bitmap:
 	gpio_device_put(gdev);
 	bitmap_free(cdev->watched_lines);
@@ -2722,8 +2720,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,
-					   &cdev->lineinfo_changed_nb);
+	blocking_notifier_chain_unregister(&gdev->notifier, &cdev->nb);
 	gpio_device_put(gdev);
 	kfree(cdev);
 
Signature · 1 line
2.39.2
#3 lore raw

PATCH 3/5 gpio: cdev: wake up chardev poll() on device unbind

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Emit a new notifier event right after the character device was unregistered and receive it in the notifier owned by the descriptor associated with the character device file. Upon reception, 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/gpio/gpiolib-cdev.c +11 -0
index 660d2e057451..eb8c0cb71da4 100644
@@ -32,6 +32,12 @@
 #include "gpiolib.h"
 #include "gpiolib-cdev.h"
 
+/*
+ * Let's use a higher value for gpio_device notifications to not conflict with
+ * UAPI defines that can also serve as notifier action arguments.
+ */
+#define GPIO_CDEV_UNREGISTERED	1000
+
 /*
  * Array sizes must ensure 64-bit alignment and not create holes in the
  * struct packing.
@@ -2526,6 +2532,9 @@ static int gpio_chardev_notify(struct notifier_block *nb, unsigned long action,
 		else
 			pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n");
 		break;
+	case GPIO_CDEV_UNREGISTERED:
+		wake_up_poll(&cdev->wait, EPOLLIN | EPOLLERR);
+		break;
 	default:
 		return NOTIFY_DONE;
 	}
@@ -2761,4 +2770,6 @@ 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->notifier,
+				     GPIO_CDEV_UNREGISTERED, NULL);
 }
Signature · 1 line
2.39.2
#4 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Extend struct linereq with a notifier block and use it to receive the GPIO device unregister event. Upon reception, 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 | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
drivers/gpio/gpiolib-cdev.c +32 -0
index eb8c0cb71da4..0b21ea04fa52 100644
@@ -563,6 +563,7 @@ struct line {
  * @wait: wait queue that handles blocking reads of events
  * @event_buffer_size: the number of elements allocated in @events
  * @events: KFIFO for the GPIO events
+ * @nb: notifier block for receiving gpio_device notifications
  * @seqno: the sequence number for edge events generated on all lines in
  * this line request.  Note that this is not used when @num_lines is 1, as
  * the line_seqno is then the same and is cheaper to calculate.
@@ -577,11 +578,17 @@ struct linereq {
 	wait_queue_head_t wait;
 	u32 event_buffer_size;
 	DECLARE_KFIFO_PTR(events, struct gpio_v2_line_event);
+	struct notifier_block nb;
 	atomic_t seqno;
 	struct mutex config_mutex;
 	struct line lines[];
 };
 
+static struct linereq *to_linereq(struct notifier_block *nb)
+{
+	return container_of(nb, struct linereq, nb);
+}
+
 #define GPIO_V2_LINE_BIAS_FLAGS \
 	(GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
 	 GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
@@ -1573,6 +1580,10 @@ static void linereq_free(struct linereq *lr)
 {
 	unsigned int i;
 
+	if (lr->nb.notifier_call)
+		blocking_notifier_chain_unregister(&lr->gdev->notifier,
+						   &lr->nb);
+
 	for (i = 0; i < lr->num_lines; i++) {
 		if (lr->lines[i].desc) {
 			edge_detector_stop(&lr->lines[i]);
@@ -1623,6 +1634,22 @@ static const struct file_operations line_fileops = {
 #endif
 };
 
+static int linereq_notify(struct notifier_block *nb, unsigned long action,
+			  void *data)
+{
+	struct linereq *lr = to_linereq(nb);
+
+	switch (action) {
+	case GPIO_CDEV_UNREGISTERED:
+		wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
 static int linereq_create(struct gpio_device *gdev, void __user *ip)
 {
 	struct gpio_v2_line_request ulr;
@@ -1733,6 +1760,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 			offset);
 	}
 
+	lr->nb.notifier_call = linereq_notify;
+	ret = blocking_notifier_chain_register(&gdev->notifier, &lr->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
#5 lore raw

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

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Extend struct lineevent_data with a notifier block and use it to receive the GPIO device unregister event. Upon reception, 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 | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
drivers/gpio/gpiolib-cdev.c +30 -0
index 0b21ea04fa52..bb6a011f7857 100644
@@ -1830,9 +1830,15 @@ struct lineevent_state {
 	int irq;
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
+	struct notifier_block nb;
 	u64 timestamp;
 };
 
+static struct lineevent_state *to_lineevent_state(struct notifier_block *nb)
+{
+	return container_of(nb, struct lineevent_state, nb);
+}
+
 #define GPIOEVENT_REQUEST_VALID_FLAGS \
 	(GPIOEVENT_REQUEST_RISING_EDGE | \
 	GPIOEVENT_REQUEST_FALLING_EDGE)
@@ -1947,6 +1953,9 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 
 static void lineevent_free(struct lineevent_state *le)
 {
+	if (le->nb.notifier_call)
+		blocking_notifier_chain_unregister(&le->gdev->notifier,
+						   &le->nb);
 	if (le->irq)
 		free_irq(le->irq, le);
 	if (le->desc)
@@ -2084,6 +2093,22 @@ static irqreturn_t lineevent_irq_handler(int irq, void *p)
 	return IRQ_WAKE_THREAD;
 }
 
+static int lineevent_notify(struct notifier_block *nb, unsigned long action,
+			    void *data)
+{
+	struct lineevent_state *le = to_lineevent_state(nb);
+
+	switch (action) {
+	case GPIO_CDEV_UNREGISTERED:
+		wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
 static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 {
 	struct gpioevent_request eventreq;
@@ -2175,6 +2200,11 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	INIT_KFIFO(le->events);
 	init_waitqueue_head(&le->wait);
 
+	le->nb.notifier_call = lineevent_notify;
+	ret = blocking_notifier_chain_register(&gdev->notifier, &le->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
#6 lore raw

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

kernel test robot noticed the following build warnings:

Hi Bartosz,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on linus/master v6.5-rc6 next-20230817] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-cdev-ignore-notifications-other-than-line-status-changes/20230816-202408
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20230816122032.15548-6-brgl%40bgdev.pl
patch subject: [PATCH 5/5] gpio: cdev: wake up lineevent poll() on device unbind
config: x86_64-randconfig-x016-20230817 (https://download.01.org/0day-ci/archive/20230817/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230817/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/gpio/gpiolib-cdev.c:1835: warning: Function parameter or member 'nb' not described in 'lineevent_state'

vim +1835 drivers/gpio/gpiolib-cdev.c

925ca36913fc7d Kent Gibson         2020-06-16  1807  
925ca36913fc7d Kent Gibson         2020-06-16  1808  /*
925ca36913fc7d Kent Gibson         2020-06-16  1809   * GPIO line event management
925ca36913fc7d Kent Gibson         2020-06-16  1810   */
925ca36913fc7d Kent Gibson         2020-06-16  1811  
925ca36913fc7d Kent Gibson         2020-06-16  1812  /**
925ca36913fc7d Kent Gibson         2020-06-16  1813   * struct lineevent_state - contains the state of a userspace event
925ca36913fc7d Kent Gibson         2020-06-16  1814   * @gdev: the GPIO device the event pertains to
925ca36913fc7d Kent Gibson         2020-06-16  1815   * @label: consumer label used to tag descriptors
925ca36913fc7d Kent Gibson         2020-06-16  1816   * @desc: the GPIO descriptor held by this event
925ca36913fc7d Kent Gibson         2020-06-16  1817   * @eflags: the event flags this line was requested with
925ca36913fc7d Kent Gibson         2020-06-16  1818   * @irq: the interrupt that trigger in response to events on this GPIO
925ca36913fc7d Kent Gibson         2020-06-16  1819   * @wait: wait queue that handles blocking reads of events
925ca36913fc7d Kent Gibson         2020-06-16  1820   * @events: KFIFO for the GPIO events
925ca36913fc7d Kent Gibson         2020-06-16  1821   * @timestamp: cache for the timestamp storing it between hardirq
925ca36913fc7d Kent Gibson         2020-06-16  1822   * and IRQ thread, used to bring the timestamp close to the actual
925ca36913fc7d Kent Gibson         2020-06-16  1823   * event
925ca36913fc7d Kent Gibson         2020-06-16  1824   */
925ca36913fc7d Kent Gibson         2020-06-16  1825  struct lineevent_state {
925ca36913fc7d Kent Gibson         2020-06-16  1826  	struct gpio_device *gdev;
925ca36913fc7d Kent Gibson         2020-06-16  1827  	const char *label;
925ca36913fc7d Kent Gibson         2020-06-16  1828  	struct gpio_desc *desc;
925ca36913fc7d Kent Gibson         2020-06-16  1829  	u32 eflags;
925ca36913fc7d Kent Gibson         2020-06-16  1830  	int irq;
925ca36913fc7d Kent Gibson         2020-06-16  1831  	wait_queue_head_t wait;
925ca36913fc7d Kent Gibson         2020-06-16  1832  	DECLARE_KFIFO(events, struct gpioevent_data, 16);
35c059f377b978 Bartosz Golaszewski 2023-08-16  1833  	struct notifier_block nb;
925ca36913fc7d Kent Gibson         2020-06-16  1834  	u64 timestamp;

925ca36913fc7d Kent Gibson 2020-06-16 @1835 };
925ca36913fc7d Kent Gibson 2020-06-16 1836

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Reviewed-by on the v1 series
#7 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 6 lines

Bartosz Golaszewski (5):
gpio: cdev: ignore notifications other than line status changes
gpio: cdev: rename the notifier block and notify callback
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

I see why this is needed and while the whole notification chain is a bit clunky I really cannot think about anything better so:

Yours,
Linus Walleij

reviewer
#8 lore raw
Quoted 17 lines · Linus Walleij
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 6 lines

Bartosz Golaszewski (5):
gpio: cdev: ignore notifications other than line status changes
gpio: cdev: rename the notifier block and notify callback
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 4 lines

I see why this is needed and while the whole notification chain is a bit clunky I really cannot think about anything better so:

The issue I have is with the repurposing/reuse of the existing notifier block that sends line changed events to the chardev.
Correct me if I'm wrong, but now all line requests will receive those events as well.
They have no business receiving those events, and it scales badly.

My preference would be for a separate nb for the chip removal to keep those two classes of events distinct.

Cheers,
Kent.

#9 lore raw
Quoted 2 lines · Kent Gibson

My preference would be for a separate nb for the chip removal to keep those two classes of events distinct.

That's a good point. Bart do you think you can rework it as such?

Yours,
Linus Walleij

#10 lore raw
In reply to Kent Gibson
Quoted 24 lines · Kent Gibson
Quoted 17 lines · Linus Walleij
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 6 lines

Bartosz Golaszewski (5):
gpio: cdev: ignore notifications other than line status changes
gpio: cdev: rename the notifier block and notify callback
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 4 lines

I see why this is needed and while the whole notification chain is a bit clunky I really cannot think about anything better so:

Quoted 9 lines

The issue I have is with the repurposing/reuse of the existing notifier block that sends line changed events to the chardev.
Correct me if I'm wrong, but now all line requests will receive those events as well.
They have no business receiving those events, and it scales badly.

My preference would be for a separate nb for the chip removal to keep those two classes of events distinct.

I would normally agree if there was a risk of abuse of those notifications by drivers but this is all private to gpiolib. And line requests that receive line state notifications simply ignore them. This isn't a bottleneck codepath IMO so where's the issue? We would be using a second notifier head of 40 bytes to struct gpio_device for no reason.

It's your code for most part so if you insist, I can rework it but I'm more in favor of repurposing the existing notifier.

Bartosz

reviewer
#11 lore raw
Quoted 35 lines · Bartosz Golaszewski
Quoted 24 lines · Kent Gibson
Quoted 17 lines · Linus Walleij
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 6 lines

Bartosz Golaszewski (5):
gpio: cdev: ignore notifications other than line status changes
gpio: cdev: rename the notifier block and notify callback
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 4 lines

I see why this is needed and while the whole notification chain is a bit clunky I really cannot think about anything better so:

Quoted 9 lines

The issue I have is with the repurposing/reuse of the existing notifier block that sends line changed events to the chardev.
Correct me if I'm wrong, but now all line requests will receive those events as well.
They have no business receiving those events, and it scales badly.

My preference would be for a separate nb for the chip removal to keep those two classes of events distinct.

Quoted 7 lines

I would normally agree if there was a risk of abuse of those notifications by drivers but this is all private to gpiolib. And line requests that receive line state notifications simply ignore them. This isn't a bottleneck codepath IMO so where's the issue? We would be using a second notifier head of 40 bytes to struct gpio_device for no reason.

Yeah, this is a space/time trade-off, and you've gone with space over time. I would select time over space.
40 bytes per device is negligable, and there is never a case where the line request wants to see a change event - it either relates to a different request, or it was triggered by the request itself. Is there an echo in here ;-)?

Cheers,
Kent.

#12 lore raw
Quoted 45 lines · Kent Gibson
Quoted 35 lines · Bartosz Golaszewski
Quoted 24 lines · Kent Gibson
Quoted 17 lines · Linus Walleij
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 6 lines

Bartosz Golaszewski (5):
gpio: cdev: ignore notifications other than line status changes
gpio: cdev: rename the notifier block and notify callback
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 4 lines

I see why this is needed and while the whole notification chain is a bit clunky I really cannot think about anything better so:

Quoted 9 lines

The issue I have is with the repurposing/reuse of the existing notifier block that sends line changed events to the chardev.
Correct me if I'm wrong, but now all line requests will receive those events as well.
They have no business receiving those events, and it scales badly.

My preference would be for a separate nb for the chip removal to keep those two classes of events distinct.

Quoted 7 lines

I would normally agree if there was a risk of abuse of those notifications by drivers but this is all private to gpiolib. And line requests that receive line state notifications simply ignore them. This isn't a bottleneck codepath IMO so where's the issue? We would be using a second notifier head of 40 bytes to struct gpio_device for no reason.

Quoted 7 lines

Yeah, this is a space/time trade-off, and you've gone with space over time. I would select time over space.
40 bytes per device is negligable, and there is never a case where the line request wants to see a change event - it either relates to a different request, or it was triggered by the request itself. Is there an echo in here ;-)?

Ok, I'll rework it for v2.

Bart