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

[PATCH 0/5] 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.

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

-- 
2.39.2

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

[Bartosz Golaszewski]

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

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.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0a33971c964c..062521e1a9e0 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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;
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index cca81375f127..de7b3b60f7ca 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -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
-- 
2.39.2

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

[Bartosz Golaszewski]

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

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.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 062521e1a9e0..660d2e057451 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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);
 
-- 
2.39.2

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

[Bartosz Golaszewski]

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

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.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 660d2e057451..eb8c0cb71da4 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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);
 }
-- 
2.39.2

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

[Bartosz Golaszewski]

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

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.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index eb8c0cb71da4..0b21ea04fa52 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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;
-- 
2.39.2

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

[Bartosz Golaszewski]

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

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.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0b21ea04fa52..bb6a011f7857 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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,
-- 
2.39.2

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

[kernel test robot]

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/202308171736.864z7YNW-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171736.864z7YNW-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171736.864z7YNW-lkp@intel.com/

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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Linus Walleij]

On Wed, Aug 16, 2023 at 2:20 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.
>
> 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:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Kent Gibson]

On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote:
> On Wed, Aug 16, 2023 at 2:20 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.
> >
> > 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:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 

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.

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

[Linus Walleij]

On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote:

> 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

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

[Bartosz Golaszewski]

On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote:
> > On Wed, Aug 16, 2023 at 2:20 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.
> > >
> > > 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:
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >
>
> 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

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

[Kent Gibson]

On Thu, Aug 17, 2023 at 09:27:37AM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote:
> > > On Wed, Aug 16, 2023 at 2:20 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.
> > > >
> > > > 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:
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> >
> > 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.
> 

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.

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

[Bartosz Golaszewski]

On Thu, Aug 17, 2023 at 9:37 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Aug 17, 2023 at 09:27:37AM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 17, 2023 at 6:41 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Wed, Aug 16, 2023 at 11:41:06PM +0200, Linus Walleij wrote:
> > > > On Wed, Aug 16, 2023 at 2:20 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.
> > > > >
> > > > > 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:
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > >
> > >
> > > 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.
> >
>
> 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