[PATCH v2 0/5] gpiolib: cdev: relocate debounce_period_us

[PATCH v2 0/5] gpiolib: cdev: relocate debounce_period_us

[Kent Gibson]

This series contains minor improvements to gpiolib-cdev.

Patch 1 replaces discrete lock/unlock calls around critical sections
with scoped_guard().  Excludes desc_gpio_to_lineinfo() as delaying
the change until patch 4 produces a cleaner change.

The banner change is relocating the debounce_period_us from gpiolib's
struct gpio_desc to cdev's struct line.  Patch 2 stores the field
locally in cdev.  Patch 3 removes the now unused field from gpiolib.

Patch 4 is somewhat related and removes a FIXME from
gpio_desc_to_lineinfo().  The FIXME relates to a race condition in
the calculation of the used  flag, but I would assert that from
the userspace perspective the read operation itself is inherently racy.
The line being reported as unused in the info provides no guarantee -
it just an indicator that requesting the line is likely to succeed -
assuming the line is not otherwise requested in the meantime.
Give the overall operation is racy, trying to stamp out an unlikely
race within the operation is pointless. Accept it as a possibility
that has negligible side-effects and reduce the number of locks held
simultaneously and the duration that the gpio_lock is held.

Patch 5 is unrelated to debounce or info, but addresses Andy's
recent complaint that the linereq get/set values functions are
confusing and under documented.
Figured I may as well add that while I was in there.

Changes v1 -> v2:
 (changes are to patch 2 unless otherwise noted)
 - adopt scoped_guard() for critical sections, inserting patch 1 and
   updating patch 2 and 4.
 - move rb_node field to beginning of struct line.
 - merge struct supinfo into supinfo var declaration.
 - move rb_tree field to beginning of struct supinfo.
 - replace pr_warn() with WARN().
 - drop explicit int to bool conversion in line_is_supplemental().
 - use continue to bypass cleanup in linereq_free().
 - fix typo in commit message (patch 4)

Kent Gibson (5):
  gpiolib: cdev: adopt scoped_guard()
  gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
  gpiolib: remove debounce_period_us from struct gpio_desc
  gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()
  gpiolib: cdev: improve documentation of get/set values

 drivers/gpio/gpiolib-cdev.c | 403 +++++++++++++++++++++++-------------
 drivers/gpio/gpiolib.c      |   3 -
 drivers/gpio/gpiolib.h      |   5 -
 3 files changed, 260 insertions(+), 151 deletions(-)

--
2.39.2

[PATCH v2 1/5] gpiolib: cdev: adopt scoped_guard()

[Kent Gibson]

Use scoped_guard for critical sections rather than distinct
lock/unlock pairs.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 140 +++++++++++++++---------------------
 1 file changed, 57 insertions(+), 83 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 02ffda6c1e51..d03698812f61 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -625,13 +625,13 @@ static void linereq_put_event(struct linereq *lr,
 {
 	bool overflow = false;
 
-	spin_lock(&lr->wait.lock);
-	if (kfifo_is_full(&lr->events)) {
-		overflow = true;
-		kfifo_skip(&lr->events);
+	scoped_guard(spinlock, &lr->wait.lock) {
+		if (kfifo_is_full(&lr->events)) {
+			overflow = true;
+			kfifo_skip(&lr->events);
+		}
+		kfifo_in(&lr->events, le, 1);
 	}
-	kfifo_in(&lr->events, le, 1);
-	spin_unlock(&lr->wait.lock);
 	if (!overflow)
 		wake_up_poll(&lr->wait, EPOLLIN);
 	else
@@ -1370,11 +1370,8 @@ static long linereq_set_values(struct linereq *lr, void __user *ip)
 	if (copy_from_user(&lv, ip, sizeof(lv)))
 		return -EFAULT;
 
-	mutex_lock(&lr->config_mutex);
-
-	ret = linereq_set_values_unlocked(lr, &lv);
-
-	mutex_unlock(&lr->config_mutex);
+	scoped_guard(mutex, &lr->config_mutex)
+		ret = linereq_set_values_unlocked(lr, &lv);
 
 	return ret;
 }
@@ -1434,11 +1431,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
 	if (ret)
 		return ret;
 
-	mutex_lock(&lr->config_mutex);
-
-	ret = linereq_set_config_unlocked(lr, &lc);
-
-	mutex_unlock(&lr->config_mutex);
+	scoped_guard(mutex, &lr->config_mutex)
+		ret = linereq_set_config_unlocked(lr, &lc);
 
 	return ret;
 }
@@ -1522,28 +1516,22 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
 		return -EINVAL;
 
 	do {
-		spin_lock(&lr->wait.lock);
-		if (kfifo_is_empty(&lr->events)) {
-			if (bytes_read) {
-				spin_unlock(&lr->wait.lock);
-				return bytes_read;
-			}
-
-			if (file->f_flags & O_NONBLOCK) {
-				spin_unlock(&lr->wait.lock);
-				return -EAGAIN;
+		scoped_guard(spinlock, &lr->wait.lock) {
+			if (kfifo_is_empty(&lr->events)) {
+				if (bytes_read)
+					return bytes_read;
+
+				if (file->f_flags & O_NONBLOCK)
+					return -EAGAIN;
+
+				ret = wait_event_interruptible_locked(lr->wait,
+						!kfifo_is_empty(&lr->events));
+				if (ret)
+					return ret;
 			}
 
-			ret = wait_event_interruptible_locked(lr->wait,
-					!kfifo_is_empty(&lr->events));
-			if (ret) {
-				spin_unlock(&lr->wait.lock);
-				return ret;
-			}
+			ret = kfifo_out(&lr->events, &le, 1);
 		}
-
-		ret = kfifo_out(&lr->events, &le, 1);
-		spin_unlock(&lr->wait.lock);
 		if (ret != 1) {
 			/*
 			 * This should never happen - we were holding the
@@ -1888,28 +1876,22 @@ static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
 		return -EINVAL;
 
 	do {
-		spin_lock(&le->wait.lock);
-		if (kfifo_is_empty(&le->events)) {
-			if (bytes_read) {
-				spin_unlock(&le->wait.lock);
-				return bytes_read;
+		scoped_guard(spinlock, &le->wait.lock) {
+			if (kfifo_is_empty(&le->events)) {
+				if (bytes_read)
+					return bytes_read;
+
+				if (file->f_flags & O_NONBLOCK)
+					return -EAGAIN;
+
+				ret = wait_event_interruptible_locked(le->wait,
+						!kfifo_is_empty(&le->events));
+				if (ret)
+					return ret;
 			}
 
-			if (file->f_flags & O_NONBLOCK) {
-				spin_unlock(&le->wait.lock);
-				return -EAGAIN;
-			}
-
-			ret = wait_event_interruptible_locked(le->wait,
-					!kfifo_is_empty(&le->events));
-			if (ret) {
-				spin_unlock(&le->wait.lock);
-				return ret;
-			}
+			ret = kfifo_out(&le->events, &ge, 1);
 		}
-
-		ret = kfifo_out(&le->events, &ge, 1);
-		spin_unlock(&le->wait.lock);
 		if (ret != 1) {
 			/*
 			 * This should never happen - we were holding the lock
@@ -2613,38 +2595,30 @@ static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
 #endif
 
 	do {
-		spin_lock(&cdev->wait.lock);
-		if (kfifo_is_empty(&cdev->events)) {
-			if (bytes_read) {
-				spin_unlock(&cdev->wait.lock);
-				return bytes_read;
-			}
-
-			if (file->f_flags & O_NONBLOCK) {
-				spin_unlock(&cdev->wait.lock);
-				return -EAGAIN;
-			}
-
-			ret = wait_event_interruptible_locked(cdev->wait,
-					!kfifo_is_empty(&cdev->events));
-			if (ret) {
-				spin_unlock(&cdev->wait.lock);
-				return ret;
+		scoped_guard(spinlock, &cdev->wait.lock) {
+			if (kfifo_is_empty(&cdev->events)) {
+				if (bytes_read)
+					return bytes_read;
+
+				if (file->f_flags & O_NONBLOCK)
+					return -EAGAIN;
+
+				ret = wait_event_interruptible_locked(cdev->wait,
+						!kfifo_is_empty(&cdev->events));
+				if (ret)
+					return ret;
 			}
-		}
 #ifdef CONFIG_GPIO_CDEV_V1
-		/* must be after kfifo check so watch_abi_version is set */
-		if (atomic_read(&cdev->watch_abi_version) == 2)
-			event_size = sizeof(struct gpio_v2_line_info_changed);
-		else
-			event_size = sizeof(struct gpioline_info_changed);
-		if (count < event_size) {
-			spin_unlock(&cdev->wait.lock);
-			return -EINVAL;
-		}
+			/* must be after kfifo check so watch_abi_version is set */
+			if (atomic_read(&cdev->watch_abi_version) == 2)
+				event_size = sizeof(struct gpio_v2_line_info_changed);
+			else
+				event_size = sizeof(struct gpioline_info_changed);
+			if (count < event_size)
+				return -EINVAL;
 #endif
-		ret = kfifo_out(&cdev->events, &event, 1);
-		spin_unlock(&cdev->wait.lock);
+			ret = kfifo_out(&cdev->events, &event, 1);
+		}
 		if (ret != 1) {
 			ret = -EIO;
 			break;
-- 
2.39.2

Re: [PATCH v2 1/5] gpiolib: cdev: adopt scoped_guard()

[Kent Gibson]

On Thu, Dec 14, 2023 at 05:58:10PM +0800, Kent Gibson wrote:
> Use scoped_guard for critical sections rather than distinct
> lock/unlock pairs.
>

It seems using guard() can further simplify cases where the scope extends
to the end of the function, so I'll replace those cases when I do v3.

Cheers,
Kent.

Re: [PATCH v2 1/5] gpiolib: cdev: adopt scoped_guard()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 07:50:44PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:58:10PM +0800, Kent Gibson wrote:
> > Use scoped_guard for critical sections rather than distinct
> > lock/unlock pairs.
> 
> It seems using guard() can further simplify cases where the scope extends
> to the end of the function, so I'll replace those cases when I do v3.

Ah, we are on the same page here!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] gpiolib: cdev: adopt scoped_guard()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 05:58:10PM +0800, Kent Gibson wrote:
> Use scoped_guard for critical sections rather than distinct

scoped_guard()

> lock/unlock pairs.

...

> -	mutex_lock(&lr->config_mutex);
> -
> -	ret = linereq_set_values_unlocked(lr, &lv);
> -
> -	mutex_unlock(&lr->config_mutex);
> +	scoped_guard(mutex, &lr->config_mutex)
> +		ret = linereq_set_values_unlocked(lr, &lv);
>  
>  	return ret;

In this case it can be guard()

	guard(...)(...);

	return linereq_...

...

> -	mutex_lock(&lr->config_mutex);
> -
> -	ret = linereq_set_config_unlocked(lr, &lc);
> -
> -	mutex_unlock(&lr->config_mutex);
> +	scoped_guard(mutex, &lr->config_mutex)
> +		ret = linereq_set_config_unlocked(lr, &lc);
>  
>  	return ret;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson]

Store the debounce period for a requested line locally, rather than in
the debounce_period_us field in the gpiolib struct gpio_desc.

Add a global tree of lines containing supplemental line information
to make the debounce period available to be reported by the
GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++-----
 1 file changed, 145 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d03698812f61..7da3b3706547 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -21,6 +21,7 @@
 #include <linux/mutex.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/poll.h>
+#include <linux/rbtree.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/timekeeping.h>
@@ -461,6 +462,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 
 /**
  * struct line - contains the state of a requested line
+ * @node: to store the object in supinfo if supplemental
  * @desc: the GPIO descriptor for this line.
  * @req: the corresponding line request
  * @irq: the interrupt triggered in response to events on this GPIO
@@ -473,6 +475,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @line_seqno: the seqno for the current edge event in the sequence of
  * events for this line.
  * @work: the worker that implements software debouncing
+ * @debounce_period_us: the debounce period in microseconds
  * @sw_debounced: flag indicating if the software debouncer is active
  * @level: the current debounced physical level of the line
  * @hdesc: the Hardware Timestamp Engine (HTE) descriptor
@@ -481,6 +484,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @last_seqno: the last sequence number before debounce period expires
  */
 struct line {
+	struct rb_node node;
 	struct gpio_desc *desc;
 	/*
 	 * -- edge detector specific fields --
@@ -514,6 +518,15 @@ struct line {
 	 * -- debouncer specific fields --
 	 */
 	struct delayed_work work;
+	/*
+	 * debounce_period_us is accessed by debounce_irq_handler() and
+	 * process_hw_ts() which are disabled when modified by
+	 * debounce_setup(), edge_detector_setup() or edge_detector_stop()
+	 * or can live with a stale version when updated by
+	 * edge_detector_update().
+	 * The modifying functions are themselves mutually exclusive.
+	 */
+	unsigned int debounce_period_us;
 	/*
 	 * sw_debounce is accessed by linereq_set_config(), which is the
 	 * only setter, and linereq_get_values(), which can live with a
@@ -546,6 +559,19 @@ struct line {
 #endif /* CONFIG_HTE */
 };
 
+/*
+ * Used to populate gpio_v2_line_info with cdev specific fields not contained
+ * in the struct gpio_desc.
+ * A line is determined to contain supplemental information by
+ * line_is_supplemental().
+ */
+static struct {
+	/* a rbtree of the struct lines containing the supplemental info */
+	struct rb_root tree;
+	/* covers tree */
+	spinlock_t lock;
+} supinfo;
+
 /**
  * struct linereq - contains the state of a userspace line request
  * @gdev: the GPIO device the line request pertains to
@@ -575,6 +601,100 @@ struct linereq {
 	struct line lines[] __counted_by(num_lines);
 };
 
+static void supinfo_init(void)
+{
+	supinfo.tree = RB_ROOT;
+	spin_lock_init(&supinfo.lock);
+}
+
+static void supinfo_insert(struct line *line)
+{
+	struct rb_node **new = &(supinfo.tree.rb_node), *parent = NULL;
+	struct line *entry;
+
+	scoped_guard(spinlock, &supinfo.lock) {
+		while (*new) {
+			entry = container_of(*new, struct line, node);
+
+			parent = *new;
+			if (line->desc < entry->desc) {
+				new = &((*new)->rb_left);
+			} else if (line->desc > entry->desc) {
+				new = &((*new)->rb_right);
+			} else {
+				/* this should never happen */
+				WARN(1, "duplicate line inserted");
+				return;
+			}
+		}
+
+		rb_link_node(&line->node, parent, new);
+		rb_insert_color(&line->node, &supinfo.tree);
+	}
+}
+
+static void supinfo_erase(struct line *line)
+{
+	scoped_guard(spinlock, &supinfo.lock)
+		rb_erase(&line->node, &supinfo.tree);
+}
+
+static struct line *supinfo_find(struct gpio_desc *desc)
+{
+	struct rb_node *node = supinfo.tree.rb_node;
+	struct line *line;
+
+	while (node) {
+		line = container_of(node, struct line, node);
+		if (desc < line->desc)
+			node = node->rb_left;
+		else if (desc > line->desc)
+			node = node->rb_right;
+		else
+			return line;
+	}
+	return NULL;
+}
+
+static void supinfo_to_lineinfo(struct gpio_desc *desc,
+				struct gpio_v2_line_info *info)
+{
+	struct gpio_v2_line_attribute *attr;
+	struct line *line;
+
+	scoped_guard(spinlock, &supinfo.lock) {
+		line = supinfo_find(desc);
+		if (line) {
+			attr = &info->attrs[info->num_attrs];
+			attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
+			attr->debounce_period_us =
+				READ_ONCE(line->debounce_period_us);
+			info->num_attrs++;
+		}
+	}
+}
+
+static inline bool line_is_supplemental(struct line *line)
+{
+	return READ_ONCE(line->debounce_period_us);
+}
+
+static void line_set_debounce_period(struct line *line,
+				     unsigned int debounce_period_us)
+{
+	bool was_suppl = line_is_supplemental(line);
+
+	WRITE_ONCE(line->debounce_period_us, debounce_period_us);
+
+	if (line_is_supplemental(line) == was_suppl)
+		return;
+
+	if (was_suppl)
+		supinfo_erase(line);
+	else
+		supinfo_insert(line);
+}
+
 #define GPIO_V2_LINE_BIAS_FLAGS \
 	(GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
 	 GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
@@ -723,7 +843,7 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p)
 		line->total_discard_seq++;
 		line->last_seqno = ts->seq;
 		mod_delayed_work(system_wq, &line->work,
-		  usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
+		  usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
 	} else {
 		if (unlikely(ts->seq < line->line_seqno))
 			return HTE_CB_HANDLED;
@@ -864,7 +984,7 @@ static irqreturn_t debounce_irq_handler(int irq, void *p)
 	struct line *line = p;
 
 	mod_delayed_work(system_wq, &line->work,
-		usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
+		usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
 
 	return IRQ_HANDLED;
 }
@@ -946,7 +1066,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 	/* try hardware */
 	ret = gpiod_set_debounce(line->desc, debounce_period_us);
 	if (!ret) {
-		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
+		line_set_debounce_period(line, debounce_period_us);
 		return ret;
 	}
 	if (ret != -ENOTSUPP)
@@ -1025,8 +1145,7 @@ static void edge_detector_stop(struct line *line)
 	cancel_delayed_work_sync(&line->work);
 	WRITE_ONCE(line->sw_debounced, 0);
 	WRITE_ONCE(line->edflags, 0);
-	if (line->desc)
-		WRITE_ONCE(line->desc->debounce_period_us, 0);
+	line_set_debounce_period(line, 0);
 	/* do not change line->level - see comment in debounced_value() */
 }
 
@@ -1051,7 +1170,7 @@ static int edge_detector_setup(struct line *line,
 		ret = debounce_setup(line, debounce_period_us);
 		if (ret)
 			return ret;
-		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
+		line_set_debounce_period(line, debounce_period_us);
 	}
 
 	/* detection disabled or sw debouncer will provide edge detection */
@@ -1093,12 +1212,12 @@ static int edge_detector_update(struct line *line,
 			gpio_v2_line_config_debounce_period(lc, line_idx);
 
 	if ((active_edflags == edflags) &&
-	    (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
+	    (READ_ONCE(line->debounce_period_us) == debounce_period_us))
 		return 0;
 
 	/* sw debounced and still will be...*/
 	if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
-		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
+		line_set_debounce_period(line, debounce_period_us);
 		return 0;
 	}
 
@@ -1561,6 +1680,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 
 static void linereq_free(struct linereq *lr)
 {
+	struct line *line;
 	unsigned int i;
 
 	if (lr->device_unregistered_nb.notifier_call)
@@ -1568,10 +1688,14 @@ static void linereq_free(struct linereq *lr)
 						   &lr->device_unregistered_nb);
 
 	for (i = 0; i < lr->num_lines; i++) {
-		if (lr->lines[i].desc) {
-			edge_detector_stop(&lr->lines[i]);
-			gpiod_free(lr->lines[i].desc);
-		}
+		line = &lr->lines[i];
+		if (!line->desc)
+			continue;
+
+		edge_detector_stop(line);
+		if (line_is_supplemental(line))
+			supinfo_erase(line);
+		gpiod_free(line->desc);
 	}
 	kfifo_free(&lr->events);
 	kfree(lr->label);
@@ -2256,8 +2380,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	struct gpio_chip *gc = desc->gdev->chip;
 	bool ok_for_pinctrl;
 	unsigned long flags;
-	u32 debounce_period_us;
-	unsigned int num_attrs = 0;
 
 	memset(info, 0, sizeof(*info));
 	info->offset = gpio_chip_hwgpio(desc);
@@ -2323,14 +2445,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
 		info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
 
-	debounce_period_us = READ_ONCE(desc->debounce_period_us);
-	if (debounce_period_us) {
-		info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
-		info->attrs[num_attrs].debounce_period_us = debounce_period_us;
-		num_attrs++;
-	}
-	info->num_attrs = num_attrs;
-
 	spin_unlock_irqrestore(&gpio_lock, flags);
 }
 
@@ -2437,6 +2551,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
 			return -EBUSY;
 	}
 	gpio_desc_to_lineinfo(desc, &lineinfo);
+	supinfo_to_lineinfo(desc, &lineinfo);
 
 	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
 		if (watch)
@@ -2527,6 +2642,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	chg.event_type = action;
 	chg.timestamp_ns = ktime_get_ns();
 	gpio_desc_to_lineinfo(desc, &chg.info);
+	supinfo_to_lineinfo(desc, &chg.info);
 
 	ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
 	if (ret)
@@ -2786,3 +2902,10 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
 	cdev_device_del(&gdev->chrdev, &gdev->dev);
 	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
 }
+
+static int __init gpiolib_cdev_init(void)
+{
+	supinfo_init();
+	return 0;
+}
+postcore_initcall(gpiolib_cdev_init);
-- 
2.39.2

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Bartosz Golaszewski]

On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Store the debounce period for a requested line locally, rather than in
> the debounce_period_us field in the gpiolib struct gpio_desc.
>
> Add a global tree of lines containing supplemental line information
> to make the debounce period available to be reported by the
> GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++-----
>  1 file changed, 145 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index d03698812f61..7da3b3706547 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -21,6 +21,7 @@
>  #include <linux/mutex.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/poll.h>
> +#include <linux/rbtree.h>
>  #include <linux/seq_file.h>
>  #include <linux/spinlock.h>
>  #include <linux/timekeeping.h>
> @@ -461,6 +462,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>
>  /**
>   * struct line - contains the state of a requested line
> + * @node: to store the object in supinfo if supplemental
>   * @desc: the GPIO descriptor for this line.
>   * @req: the corresponding line request
>   * @irq: the interrupt triggered in response to events on this GPIO
> @@ -473,6 +475,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>   * @line_seqno: the seqno for the current edge event in the sequence of
>   * events for this line.
>   * @work: the worker that implements software debouncing
> + * @debounce_period_us: the debounce period in microseconds
>   * @sw_debounced: flag indicating if the software debouncer is active
>   * @level: the current debounced physical level of the line
>   * @hdesc: the Hardware Timestamp Engine (HTE) descriptor
> @@ -481,6 +484,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>   * @last_seqno: the last sequence number before debounce period expires
>   */
>  struct line {
> +       struct rb_node node;
>         struct gpio_desc *desc;
>         /*
>          * -- edge detector specific fields --
> @@ -514,6 +518,15 @@ struct line {
>          * -- debouncer specific fields --
>          */
>         struct delayed_work work;
> +       /*
> +        * debounce_period_us is accessed by debounce_irq_handler() and
> +        * process_hw_ts() which are disabled when modified by
> +        * debounce_setup(), edge_detector_setup() or edge_detector_stop()
> +        * or can live with a stale version when updated by
> +        * edge_detector_update().
> +        * The modifying functions are themselves mutually exclusive.
> +        */
> +       unsigned int debounce_period_us;
>         /*
>          * sw_debounce is accessed by linereq_set_config(), which is the
>          * only setter, and linereq_get_values(), which can live with a
> @@ -546,6 +559,19 @@ struct line {
>  #endif /* CONFIG_HTE */
>  };
>
> +/*
> + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> + * in the struct gpio_desc.
> + * A line is determined to contain supplemental information by
> + * line_is_supplemental().
> + */
> +static struct {
> +       /* a rbtree of the struct lines containing the supplemental info */
> +       struct rb_root tree;
> +       /* covers tree */
> +       spinlock_t lock;

Looks like this is never taken from atomic context? Can this be a mutex instead?

> +} supinfo;
> +
>  /**
>   * struct linereq - contains the state of a userspace line request
>   * @gdev: the GPIO device the line request pertains to
> @@ -575,6 +601,100 @@ struct linereq {
>         struct line lines[] __counted_by(num_lines);
>  };
>
> +static void supinfo_init(void)
> +{
> +       supinfo.tree = RB_ROOT;
> +       spin_lock_init(&supinfo.lock);
> +}
> +
> +static void supinfo_insert(struct line *line)
> +{
> +       struct rb_node **new = &(supinfo.tree.rb_node), *parent = NULL;
> +       struct line *entry;
> +
> +       scoped_guard(spinlock, &supinfo.lock) {
> +               while (*new) {
> +                       entry = container_of(*new, struct line, node);
> +
> +                       parent = *new;
> +                       if (line->desc < entry->desc) {
> +                               new = &((*new)->rb_left);
> +                       } else if (line->desc > entry->desc) {
> +                               new = &((*new)->rb_right);
> +                       } else {
> +                               /* this should never happen */
> +                               WARN(1, "duplicate line inserted");
> +                               return;
> +                       }
> +               }
> +
> +               rb_link_node(&line->node, parent, new);
> +               rb_insert_color(&line->node, &supinfo.tree);
> +       }
> +}
> +
> +static void supinfo_erase(struct line *line)
> +{
> +       scoped_guard(spinlock, &supinfo.lock)
> +               rb_erase(&line->node, &supinfo.tree);
> +}
> +
> +static struct line *supinfo_find(struct gpio_desc *desc)
> +{
> +       struct rb_node *node = supinfo.tree.rb_node;
> +       struct line *line;
> +
> +       while (node) {
> +               line = container_of(node, struct line, node);
> +               if (desc < line->desc)
> +                       node = node->rb_left;
> +               else if (desc > line->desc)
> +                       node = node->rb_right;
> +               else
> +                       return line;
> +       }
> +       return NULL;
> +}
> +
> +static void supinfo_to_lineinfo(struct gpio_desc *desc,
> +                               struct gpio_v2_line_info *info)
> +{
> +       struct gpio_v2_line_attribute *attr;
> +       struct line *line;
> +
> +       scoped_guard(spinlock, &supinfo.lock) {
> +               line = supinfo_find(desc);
> +               if (line) {
> +                       attr = &info->attrs[info->num_attrs];
> +                       attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> +                       attr->debounce_period_us =
> +                               READ_ONCE(line->debounce_period_us);
> +                       info->num_attrs++;
> +               }
> +       }
> +}
> +
> +static inline bool line_is_supplemental(struct line *line)

I would call this function line_has_suppinfo().

> +{
> +       return READ_ONCE(line->debounce_period_us);
> +}
> +
> +static void line_set_debounce_period(struct line *line,
> +                                    unsigned int debounce_period_us)
> +{
> +       bool was_suppl = line_is_supplemental(line);

This logic could use some comments, it took me a while to figure out
what it's doing.

> +
> +       WRITE_ONCE(line->debounce_period_us, debounce_period_us);
> +
> +       if (line_is_supplemental(line) == was_suppl)
> +               return;
> +
> +       if (was_suppl)
> +               supinfo_erase(line);
> +       else
> +               supinfo_insert(line);
> +}
> +
>  #define GPIO_V2_LINE_BIAS_FLAGS \
>         (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
>          GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
> @@ -723,7 +843,7 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p)
>                 line->total_discard_seq++;
>                 line->last_seqno = ts->seq;
>                 mod_delayed_work(system_wq, &line->work,
> -                 usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
> +                 usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
>         } else {
>                 if (unlikely(ts->seq < line->line_seqno))
>                         return HTE_CB_HANDLED;
> @@ -864,7 +984,7 @@ static irqreturn_t debounce_irq_handler(int irq, void *p)
>         struct line *line = p;
>
>         mod_delayed_work(system_wq, &line->work,
> -               usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
> +               usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
>
>         return IRQ_HANDLED;
>  }
> @@ -946,7 +1066,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
>         /* try hardware */
>         ret = gpiod_set_debounce(line->desc, debounce_period_us);
>         if (!ret) {
> -               WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> +               line_set_debounce_period(line, debounce_period_us);
>                 return ret;
>         }
>         if (ret != -ENOTSUPP)
> @@ -1025,8 +1145,7 @@ static void edge_detector_stop(struct line *line)
>         cancel_delayed_work_sync(&line->work);
>         WRITE_ONCE(line->sw_debounced, 0);
>         WRITE_ONCE(line->edflags, 0);
> -       if (line->desc)
> -               WRITE_ONCE(line->desc->debounce_period_us, 0);
> +       line_set_debounce_period(line, 0);
>         /* do not change line->level - see comment in debounced_value() */
>  }
>
> @@ -1051,7 +1170,7 @@ static int edge_detector_setup(struct line *line,
>                 ret = debounce_setup(line, debounce_period_us);
>                 if (ret)
>                         return ret;
> -               WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> +               line_set_debounce_period(line, debounce_period_us);
>         }
>
>         /* detection disabled or sw debouncer will provide edge detection */
> @@ -1093,12 +1212,12 @@ static int edge_detector_update(struct line *line,
>                         gpio_v2_line_config_debounce_period(lc, line_idx);
>
>         if ((active_edflags == edflags) &&
> -           (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
> +           (READ_ONCE(line->debounce_period_us) == debounce_period_us))
>                 return 0;
>
>         /* sw debounced and still will be...*/
>         if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
> -               WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> +               line_set_debounce_period(line, debounce_period_us);
>                 return 0;
>         }
>
> @@ -1561,6 +1680,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
>
>  static void linereq_free(struct linereq *lr)
>  {
> +       struct line *line;
>         unsigned int i;
>
>         if (lr->device_unregistered_nb.notifier_call)
> @@ -1568,10 +1688,14 @@ static void linereq_free(struct linereq *lr)
>                                                    &lr->device_unregistered_nb);
>
>         for (i = 0; i < lr->num_lines; i++) {
> -               if (lr->lines[i].desc) {
> -                       edge_detector_stop(&lr->lines[i]);
> -                       gpiod_free(lr->lines[i].desc);
> -               }
> +               line = &lr->lines[i];
> +               if (!line->desc)
> +                       continue;
> +
> +               edge_detector_stop(line);
> +               if (line_is_supplemental(line))
> +                       supinfo_erase(line);
> +               gpiod_free(line->desc);
>         }
>         kfifo_free(&lr->events);
>         kfree(lr->label);
> @@ -2256,8 +2380,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>         struct gpio_chip *gc = desc->gdev->chip;
>         bool ok_for_pinctrl;
>         unsigned long flags;
> -       u32 debounce_period_us;
> -       unsigned int num_attrs = 0;
>
>         memset(info, 0, sizeof(*info));
>         info->offset = gpio_chip_hwgpio(desc);
> @@ -2323,14 +2445,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>         else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
>                 info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
>
> -       debounce_period_us = READ_ONCE(desc->debounce_period_us);
> -       if (debounce_period_us) {
> -               info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> -               info->attrs[num_attrs].debounce_period_us = debounce_period_us;
> -               num_attrs++;
> -       }
> -       info->num_attrs = num_attrs;
> -
>         spin_unlock_irqrestore(&gpio_lock, flags);
>  }
>
> @@ -2437,6 +2551,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
>                         return -EBUSY;
>         }
>         gpio_desc_to_lineinfo(desc, &lineinfo);
> +       supinfo_to_lineinfo(desc, &lineinfo);
>
>         if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
>                 if (watch)
> @@ -2527,6 +2642,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
>         chg.event_type = action;
>         chg.timestamp_ns = ktime_get_ns();
>         gpio_desc_to_lineinfo(desc, &chg.info);
> +       supinfo_to_lineinfo(desc, &chg.info);
>
>         ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
>         if (ret)
> @@ -2786,3 +2902,10 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
>         cdev_device_del(&gdev->chrdev, &gdev->dev);
>         blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
>  }
> +
> +static int __init gpiolib_cdev_init(void)
> +{
> +       supinfo_init();
> +       return 0;
> +}
> +postcore_initcall(gpiolib_cdev_init);
> --
> 2.39.2
>

Bart

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson]

On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> >
> > +/*
> > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > + * in the struct gpio_desc.
> > + * A line is determined to contain supplemental information by
> > + * line_is_supplemental().
> > + */
> > +static struct {
> > +       /* a rbtree of the struct lines containing the supplemental info */
> > +       struct rb_root tree;
> > +       /* covers tree */
> > +       spinlock_t lock;
>
> Looks like this is never taken from atomic context? Can this be a mutex instead?
>

Correct, only from thread context.

Can be a mutex, but it only covers tree lookups which should be quick
as the tree is kept minimal, and I wouldn't expect it to ever get to the
mutex slowpath, so a spinlock seemed more appropriate.

Cheers,
Kent.

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Bartosz Golaszewski]

On Thu, Dec 14, 2023 at 3:45 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > >
> > > +/*
> > > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > > + * in the struct gpio_desc.
> > > + * A line is determined to contain supplemental information by
> > > + * line_is_supplemental().
> > > + */
> > > +static struct {
> > > +       /* a rbtree of the struct lines containing the supplemental info */
> > > +       struct rb_root tree;
> > > +       /* covers tree */
> > > +       spinlock_t lock;
> >
> > Looks like this is never taken from atomic context? Can this be a mutex instead?
> >
>
> Correct, only from thread context.
>
> Can be a mutex, but it only covers tree lookups which should be quick
> as the tree is kept minimal, and I wouldn't expect it to ever get to the
> mutex slowpath, so a spinlock seemed more appropriate.
>

Fair enough.

Bart

> Cheers,
> Kent.

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson]

On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 3:45 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > >
> > > > +/*
> > > > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > > > + * in the struct gpio_desc.
> > > > + * A line is determined to contain supplemental information by
> > > > + * line_is_supplemental().
> > > > + */
> > > > +static struct {
> > > > +       /* a rbtree of the struct lines containing the supplemental info */
> > > > +       struct rb_root tree;
> > > > +       /* covers tree */
> > > > +       spinlock_t lock;
> > >
> > > Looks like this is never taken from atomic context? Can this be a mutex instead?
> > >
> >
> > Correct, only from thread context.
> >
> > Can be a mutex, but it only covers tree lookups which should be quick
> > as the tree is kept minimal, and I wouldn't expect it to ever get to the
> > mutex slowpath, so a spinlock seemed more appropriate.
> >
>
> Fair enough.
>
> Bart
>

While I think of it, what tree should I be basing on?
These patches are based on v6.7-rc5, and I'm not aware of any other
changes they may contend with, but best to be on the right tree to be
sure.

Cheers,
Kent.

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> Store the debounce period for a requested line locally, rather than in
> the debounce_period_us field in the gpiolib struct gpio_desc.
> 
> Add a global tree of lines containing supplemental line information
> to make the debounce period available to be reported by the
> GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.

...

> +/*
> + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> + * in the struct gpio_desc.
> + * A line is determined to contain supplemental information by
> + * line_is_supplemental().
> + */
> +static struct {
> +	/* a rbtree of the struct lines containing the supplemental info */
> +	struct rb_root tree;
> +	/* covers tree */
> +	spinlock_t lock;
> +} supinfo;

...

> +static void supinfo_init(void)
> +{
> +	supinfo.tree = RB_ROOT;
> +	spin_lock_init(&supinfo.lock);
> +}

Can it be done statically?

supinfo = {
	.tree = RB_ROOT,
	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
};

...

> +static int __init gpiolib_cdev_init(void)
> +{
> +	supinfo_init();
> +	return 0;
> +}

A comment why it's postcore initcall?

/* postcore initcall is chosen because ... */

> +postcore_initcall(gpiolib_cdev_init);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > +/*
> > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > + * in the struct gpio_desc.
> > + * A line is determined to contain supplemental information by
> > + * line_is_supplemental().
> > + */
> > +static struct {
> > +	/* a rbtree of the struct lines containing the supplemental info */
> > +	struct rb_root tree;
> > +	/* covers tree */
> > +	spinlock_t lock;
> > +} supinfo;

Hmm... If I read the kernel-doc script it should support anonymous structs
and unions...

...

> > +static void supinfo_init(void)
> > +{
> > +	supinfo.tree = RB_ROOT;
> > +	spin_lock_init(&supinfo.lock);
> > +}
> 
> Can it be done statically?
> 
> supinfo = {
> 	.tree = RB_ROOT,
> 	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),

I even checked the current tree, we have 32 users of this pattern in drivers/.

> };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson]

On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
>
> ...
>
> > > +/*
> > > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > > + * in the struct gpio_desc.
> > > + * A line is determined to contain supplemental information by
> > > + * line_is_supplemental().
> > > + */
> > > +static struct {
> > > +	/* a rbtree of the struct lines containing the supplemental info */
> > > +	struct rb_root tree;
> > > +	/* covers tree */
> > > +	spinlock_t lock;
> > > +} supinfo;
>
> Hmm... If I read the kernel-doc script it should support anonymous structs
> and unions...
>
> ...
>
> > > +static void supinfo_init(void)
> > > +{
> > > +	supinfo.tree = RB_ROOT;
> > > +	spin_lock_init(&supinfo.lock);
> > > +}
> >
> > Can it be done statically?
> >
> > supinfo = {
> > 	.tree = RB_ROOT,
> > 	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> I even checked the current tree, we have 32 users of this pattern in drivers/.
>

Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
another hangover from when I was trying to create the supinfo per chip,
but now it is a global a static initialiser makes sense.

And I still haven't received the email you quote there.

Cheers,
Kent.

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Andy Shevchenko]

On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > > > +static void supinfo_init(void)
> > > > +{
> > > > +	supinfo.tree = RB_ROOT;
> > > > +	spin_lock_init(&supinfo.lock);
> > > > +}
> > >
> > > Can it be done statically?
> > >
> > > supinfo = {
> > > 	.tree = RB_ROOT,
> > > 	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> >
> > I even checked the current tree, we have 32 users of this pattern in drivers/.
> 
> Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> another hangover from when I was trying to create the supinfo per chip,
> but now it is a global a static initialiser makes sense.

Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
than above.

> And I still haven't received the email you quote there.

:-( I'm not sure we will get it, it most likely that I removed it already
and it has disappeared due to problems with email server...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Bartosz Golaszewski]

On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > > +static void supinfo_init(void)
> > > > > +{
> > > > > +       supinfo.tree = RB_ROOT;
> > > > > +       spin_lock_init(&supinfo.lock);
> > > > > +}
> > > >
> > > > Can it be done statically?
> > > >
> > > > supinfo = {
> > > >   .tree = RB_ROOT,
> > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),

Double underscore typically means it's private and shouldn't be used.

> > >
> > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> >
> > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > another hangover from when I was trying to create the supinfo per chip,
> > but now it is a global a static initialiser makes sense.
>
> Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> than above.

Yeah, so maybe we should use non-struct, global variables after all.

Bart

>
> > And I still haven't received the email you quote there.
>
> :-( I'm not sure we will get it, it most likely that I removed it already
> and it has disappeared due to problems with email server...
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson]

On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> >
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> >
> > ...
> >
> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > >   .tree = RB_ROOT,
> > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> Double underscore typically means it's private and shouldn't be used.
>

You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
all used in gpiolib.c?

> > > >
> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > >
> > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
>
> Yeah, so maybe we should use non-struct, global variables after all.
>

Despite the 32 cases cited that already use that pattern?
9 of which use __SPIN_LOCK_UNLOCKED().
Sounds like a pretty convincing argument to use the struct ;-).

But lets keep it as kosher as possible and split out the struct :-(.

Cheers,
Kent.

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Bartosz Golaszewski]

On Fri, Dec 15, 2023 at 2:04 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > >
> > > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> > >
> > > ...
> > >
> > > > > > > +static void supinfo_init(void)
> > > > > > > +{
> > > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > > +}
> > > > > >
> > > > > > Can it be done statically?
> > > > > >
> > > > > > supinfo = {
> > > > > >   .tree = RB_ROOT,
> > > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> >
> > Double underscore typically means it's private and shouldn't be used.
> >
>
> You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
> all used in gpiolib.c?
>

Touché. But this is just lack of strict naming conventions. :( Another
common use of leading underscores are "unlocked" (or in this case:
non-atomic) variants of functions.

> > > > >
> > > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > > >

As opposed to ~1200 uses of DEFINE_SPINLOCK if you really want to go there. :)

> > > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > > another hangover from when I was trying to create the supinfo per chip,
> > > > but now it is a global a static initialiser makes sense.
> > >
> > > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > > than above.
> >
> > Yeah, so maybe we should use non-struct, global variables after all.
> >
>
> Despite the 32 cases cited that already use that pattern?
> 9 of which use __SPIN_LOCK_UNLOCKED().
> Sounds like a pretty convincing argument to use the struct ;-).
>
> But lets keep it as kosher as possible and split out the struct :-(.
>

I'll leave it for you to decide, I don't have a strong opinion and the
entire file is your code so you should pick.

Bart

> Cheers,
> Kent.
>

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson]

On Fri, Dec 15, 2023 at 09:07:48AM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 15, 2023 at 2:04 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > > >
> > > > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > +static void supinfo_init(void)
> > > > > > > > +{
> > > > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > > > +}
> > > > > > >
> > > > > > > Can it be done statically?
> > > > > > >
> > > > > > > supinfo = {
> > > > > > >   .tree = RB_ROOT,
> > > > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> > >
> > > Double underscore typically means it's private and shouldn't be used.
> > >
> >
> > You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
> > all used in gpiolib.c?
> >
>
> Touché. But this is just lack of strict naming conventions. :( Another
> common use of leading underscores are "unlocked" (or in this case:
> non-atomic) variants of functions.
>

Sorry, should've added a ;-) to the end of that one - not giving you a
hard time, just found it amusing.

> > > > > >
> > > > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > > > >
>
> As opposed to ~1200 uses of DEFINE_SPINLOCK if you really want to go there. :)
>

To be clear, that is Andy's quote you are replying to :-).

> > > > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > > > another hangover from when I was trying to create the supinfo per chip,
> > > > > but now it is a global a static initialiser makes sense.
> > > >
> > > > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > > > than above.
> > >
> > > Yeah, so maybe we should use non-struct, global variables after all.
> > >
> >
> > Despite the 32 cases cited that already use that pattern?
> > 9 of which use __SPIN_LOCK_UNLOCKED().
> > Sounds like a pretty convincing argument to use the struct ;-).
> >
> > But lets keep it as kosher as possible and split out the struct :-(.
> >
>
> I'll leave it for you to decide, I don't have a strong opinion and the
> entire file is your code so you should pick.
>

I've split it out in v3.

Cheers,
Kent.

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > >   .tree = RB_ROOT,
> > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> 
> Double underscore typically means it's private and shouldn't be used.

Right, but when you have a struct you have no other means to initialize this
directly.

> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.

See, there are users of the __ initializers.

> > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
> 
> Yeah, so maybe we should use non-struct, global variables after all.

At least this will allow to get rid of (questionable) initcall.

> > > And I still haven't received the email you quote there.
> >
> > :-( I'm not sure we will get it, it most likely that I removed it already
> > and it has disappeared due to problems with email server...

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/5] gpiolib: remove debounce_period_us from struct gpio_desc

[Kent Gibson]

cdev is the only user of the debounce_period_us field in
struct gpio_desc, and it no longer uses it, so remove it.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c | 3 ---
 drivers/gpio/gpiolib.h | 5 -----
 2 files changed, 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 95d2a7b2ea3e..b1e81a561141 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2330,9 +2330,6 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 #ifdef CONFIG_OF_DYNAMIC
 		desc->hog = NULL;
-#endif
-#ifdef CONFIG_GPIO_CDEV
-		WRITE_ONCE(desc->debounce_period_us, 0);
 #endif
 		ret = true;
 	}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3ccacf3c1288..a4a2520b5f31 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -147,7 +147,6 @@ void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
  * @label:		Name of the consumer
  * @name:		Line name
  * @hog:		Pointer to the device node that hogs this line (if any)
- * @debounce_period_us:	Debounce period in microseconds
  *
  * These are obtained using gpiod_get() and are preferable to the old
  * integer-based handles.
@@ -185,10 +184,6 @@ struct gpio_desc {
 #ifdef CONFIG_OF_DYNAMIC
 	struct device_node	*hog;
 #endif
-#ifdef CONFIG_GPIO_CDEV
-	/* debounce period in microseconds */
-	unsigned int		debounce_period_us;
-#endif
 };
 
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
-- 
2.39.2

[PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Kent Gibson]

Reduce the time holding the gpio_lock by snapshotting the desc flags,
rather than testing them individually while holding the lock.

Accept that the calculation of the used field is inherently racy, and
only check the availability of the line from pinctrl if other checks
pass, so avoiding the check for lines that are otherwise in use.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 72 ++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 7da3b3706547..73262305de0f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2378,74 +2378,72 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 				  struct gpio_v2_line_info *info)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
-	bool ok_for_pinctrl;
 	unsigned long flags;
 
 	memset(info, 0, sizeof(*info));
 	info->offset = gpio_chip_hwgpio(desc);
 
-	/*
-	 * This function takes a mutex so we must check this before taking
-	 * the spinlock.
-	 *
-	 * FIXME: find a non-racy way to retrieve this information. Maybe a
-	 * lock common to both frameworks?
-	 */
-	ok_for_pinctrl = pinctrl_gpio_can_use_line(gc, info->offset);
+	scoped_guard(spinlock_irqsave, &gpio_lock) {
+		if (desc->name)
+			strscpy(info->name, desc->name, sizeof(info->name));
 
-	spin_lock_irqsave(&gpio_lock, flags);
+		if (desc->label)
+			strscpy(info->consumer, desc->label,
+				sizeof(info->consumer));
 
-	if (desc->name)
-		strscpy(info->name, desc->name, sizeof(info->name));
-
-	if (desc->label)
-		strscpy(info->consumer, desc->label, sizeof(info->consumer));
+		flags = READ_ONCE(desc->flags);
+	}
 
 	/*
-	 * Userspace only need to know that the kernel is using this GPIO so
-	 * it can't use it.
+	 * Userspace only need know that the kernel is using this GPIO so it
+	 * can't use it.
+	 * The calculation of the used flag is slightly racy, as it may read
+	 * desc, gc and pinctrl state without a lock covering all three at
+	 * once.  Worst case if the line is in transition and the calculation
+	 * is inconsistent then it looks to the user like they performed the
+	 * read on the other side of the transition - but that can always
+	 * happen.
+	 * The definitive test that a line is available to userspace is to
+	 * request it.
 	 */
-	info->flags = 0;
-	if (test_bit(FLAG_REQUESTED, &desc->flags) ||
-	    test_bit(FLAG_IS_HOGGED, &desc->flags) ||
-	    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
-	    test_bit(FLAG_EXPORT, &desc->flags) ||
-	    test_bit(FLAG_SYSFS, &desc->flags) ||
+	if (test_bit(FLAG_REQUESTED, &flags) ||
+	    test_bit(FLAG_IS_HOGGED, &flags) ||
+	    test_bit(FLAG_USED_AS_IRQ, &flags) ||
+	    test_bit(FLAG_EXPORT, &flags) ||
+	    test_bit(FLAG_SYSFS, &flags) ||
 	    !gpiochip_line_is_valid(gc, info->offset) ||
-	    !ok_for_pinctrl)
+	    !pinctrl_gpio_can_use_line(gc, info->offset))
 		info->flags |= GPIO_V2_LINE_FLAG_USED;
 
-	if (test_bit(FLAG_IS_OUT, &desc->flags))
+	if (test_bit(FLAG_IS_OUT, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
 	else
 		info->flags |= GPIO_V2_LINE_FLAG_INPUT;
 
-	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+	if (test_bit(FLAG_ACTIVE_LOW, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW;
 
-	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
+	if (test_bit(FLAG_OPEN_DRAIN, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN;
-	if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
+	if (test_bit(FLAG_OPEN_SOURCE, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE;
 
-	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+	if (test_bit(FLAG_BIAS_DISABLE, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED;
-	if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+	if (test_bit(FLAG_PULL_DOWN, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
-	if (test_bit(FLAG_PULL_UP, &desc->flags))
+	if (test_bit(FLAG_PULL_UP, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
 
-	if (test_bit(FLAG_EDGE_RISING, &desc->flags))
+	if (test_bit(FLAG_EDGE_RISING, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING;
-	if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
+	if (test_bit(FLAG_EDGE_FALLING, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING;
 
-	if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags))
+	if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME;
-	else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
+	else if (test_bit(FLAG_EVENT_CLOCK_HTE, &flags))
 		info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
-
-	spin_unlock_irqrestore(&gpio_lock, flags);
 }
 
 struct gpio_chardev_data {
-- 
2.39.2

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
> Reduce the time holding the gpio_lock by snapshotting the desc flags,
> rather than testing them individually while holding the lock.
> 
> Accept that the calculation of the used field is inherently racy, and
> only check the availability of the line from pinctrl if other checks
> pass, so avoiding the check for lines that are otherwise in use.

...

> -	spin_lock_irqsave(&gpio_lock, flags);

Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
instead of spinlock)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Kent Gibson]

On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
> > Reduce the time holding the gpio_lock by snapshotting the desc flags,
> > rather than testing them individually while holding the lock.
> >
> > Accept that the calculation of the used field is inherently racy, and
> > only check the availability of the line from pinctrl if other checks
> > pass, so avoiding the check for lines that are otherwise in use.
>
> ...
>
> > -	spin_lock_irqsave(&gpio_lock, flags);
>
> Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> instead of spinlock)?
>

Read the cover letter.
Doing that made the change larger, as flags gets removed then restored.
I had also thought the flag tests would get indented then unindented, but
if we use guard() the indentation should remain unchanged.

Can do it in 1 if you are happy with the flags declaration being
removed in patch 1 and restored in 4.

Cheers,
Kent.

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
> > > Reduce the time holding the gpio_lock by snapshotting the desc flags,
> > > rather than testing them individually while holding the lock.
> > >
> > > Accept that the calculation of the used field is inherently racy, and
> > > only check the availability of the line from pinctrl if other checks
> > > pass, so avoiding the check for lines that are otherwise in use.

...

> > > -	spin_lock_irqsave(&gpio_lock, flags);
> >
> > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > instead of spinlock)?
> >
> 
> Read the cover letter.
> Doing that made the change larger, as flags gets removed then restored.
> I had also thought the flag tests would get indented then unindented, but
> if we use guard() the indentation should remain unchanged.

I'm fine with that as I pointed out (have you received that mail? I had
problems with my mail server) the dflags is better semantically, so restoration
with _different_ name is fine.

> Can do it in 1 if you are happy with the flags declaration being
> removed in patch 1 and restored in 4.

Definitely.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Kent Gibson]

On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
> > > > Reduce the time holding the gpio_lock by snapshotting the desc flags,
> > > > rather than testing them individually while holding the lock.
> > > >
> > > > Accept that the calculation of the used field is inherently racy, and
> > > > only check the availability of the line from pinctrl if other checks
> > > > pass, so avoiding the check for lines that are otherwise in use.
>
> ...
>
> > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > >
> > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > instead of spinlock)?
> > >
> >
> > Read the cover letter.
> > Doing that made the change larger, as flags gets removed then restored.
> > I had also thought the flag tests would get indented then unindented, but
> > if we use guard() the indentation should remain unchanged.
>
> I'm fine with that as I pointed out (have you received that mail? I had
> problems with my mail server) the dflags is better semantically, so restoration
> with _different_ name is fine.
>

I have noted that some of your replies have been delayed, and I can't be sure
of what I might not've received. I can't say I've seen one that mentions the
dflags name being preferable.

I prefer the plain flags name, if there is only one flag variable in the
function.

> > Can do it in 1 if you are happy with the flags declaration being
> > removed in patch 1 and restored in 4.
>
> Definitely.
>

Ok will re-arrange in v3.

Cheers,
Kent.

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Kent Gibson]

On Thu, Dec 14, 2023 at 11:34:44PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
> > > > > Reduce the time holding the gpio_lock by snapshotting the desc flags,
> > > > > rather than testing them individually while holding the lock.
> > > > >
> > > > > Accept that the calculation of the used field is inherently racy, and
> > > > > only check the availability of the line from pinctrl if other checks
> > > > > pass, so avoiding the check for lines that are otherwise in use.
> >
> > ...
> >
> > > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > > >
> > > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > > instead of spinlock)?
> > > >
> > >
> > > Read the cover letter.
> > > Doing that made the change larger, as flags gets removed then restored.
> > > I had also thought the flag tests would get indented then unindented, but
> > > if we use guard() the indentation should remain unchanged.
> >
> > I'm fine with that as I pointed out (have you received that mail? I had
> > problems with my mail server) the dflags is better semantically, so restoration
> > with _different_ name is fine.
> >
>
> I have noted that some of your replies have been delayed, and I can't be sure
> of what I might not've received. I can't say I've seen one that mentions the
> dflags name being preferable.
>
> I prefer the plain flags name, if there is only one flag variable in the
> function.
>
> > > Can do it in 1 if you are happy with the flags declaration being
> > > removed in patch 1 and restored in 4.
> >
> > Definitely.
> >
>
> Ok will re-arrange in v3.
>

Hang on - patch 4 has to use a scoped_guard(), so are you ok for patch 1
to introduce a guard(), to avoid changing the indentation, only to
replace it with a scoped_guard(), to perform the tests after releasing
the lock, in patch 4?

Cheers,
Kent.

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 11:46:54PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 11:34:44PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > > > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:

...

> > > > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > > > >
> > > > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > > > instead of spinlock)?
> > > >
> > > > Read the cover letter.
> > > > Doing that made the change larger, as flags gets removed then restored.
> > > > I had also thought the flag tests would get indented then unindented, but
> > > > if we use guard() the indentation should remain unchanged.
> > >
> > > I'm fine with that as I pointed out (have you received that mail? I had
> > > problems with my mail server) the dflags is better semantically, so restoration
> > > with _different_ name is fine.
> >
> > I have noted that some of your replies have been delayed, and I can't be sure
> > of what I might not've received. I can't say I've seen one that mentions the
> > dflags name being preferable.
> >
> > I prefer the plain flags name, if there is only one flag variable in the
> > function.
> >
> > > > Can do it in 1 if you are happy with the flags declaration being
> > > > removed in patch 1 and restored in 4.
> > >
> > > Definitely.
> >
> > Ok will re-arrange in v3.
> 
> Hang on - patch 4 has to use a scoped_guard(), so are you ok for patch 1
> to introduce a guard(), to avoid changing the indentation, only to
> replace it with a scoped_guard(), to perform the tests after releasing
> the lock, in patch 4?

Hmm... If we need to use scoped_guard() at the end, can we introduce it in
patch 1?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Kent Gibson]

On Thu, Dec 14, 2023 at 11:46:54PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 11:34:44PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > > > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
> > > > > > Reduce the time holding the gpio_lock by snapshotting the desc flags,
> > > > > > rather than testing them individually while holding the lock.
> > > > > >
> > > > > > Accept that the calculation of the used field is inherently racy, and
> > > > > > only check the availability of the line from pinctrl if other checks
> > > > > > pass, so avoiding the check for lines that are otherwise in use.
> > >
> > > ...
> > >
> > > > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > > > >
> > > > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > > > instead of spinlock)?
> > > > >
> > > >
> > > > Read the cover letter.
> > > > Doing that made the change larger, as flags gets removed then restored.
> > > > I had also thought the flag tests would get indented then unindented, but
> > > > if we use guard() the indentation should remain unchanged.
> > >
> > > I'm fine with that as I pointed out (have you received that mail? I had
> > > problems with my mail server) the dflags is better semantically, so restoration
> > > with _different_ name is fine.
> > >
> >
> > I have noted that some of your replies have been delayed, and I can't be sure
> > of what I might not've received. I can't say I've seen one that mentions the
> > dflags name being preferable.
> >
> > I prefer the plain flags name, if there is only one flag variable in the
> > function.
> >
> > > > Can do it in 1 if you are happy with the flags declaration being
> > > > removed in patch 1 and restored in 4.
> > >
> > > Definitely.
> > >
> >
> > Ok will re-arrange in v3.
> >
>
> Hang on - patch 4 has to use a scoped_guard(), so are you ok for patch 1
> to introduce a guard(), to avoid changing the indentation, only to
> replace it with a scoped_guard(), to perform the tests after releasing
> the lock, in patch 4?
>

Alternatively, I can move patch 4 to the top of the series.

Cheers,
Kent.

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 11:53:10PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 11:46:54PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 11:34:44PM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > > > > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:

...

> > > > > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > > > > >
> > > > > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > > > > instead of spinlock)?
> > > > >
> > > > > Read the cover letter.
> > > > > Doing that made the change larger, as flags gets removed then restored.
> > > > > I had also thought the flag tests would get indented then unindented, but
> > > > > if we use guard() the indentation should remain unchanged.
> > > >
> > > > I'm fine with that as I pointed out (have you received that mail? I had
> > > > problems with my mail server) the dflags is better semantically, so restoration
> > > > with _different_ name is fine.
> > >
> > > I have noted that some of your replies have been delayed, and I can't be sure
> > > of what I might not've received. I can't say I've seen one that mentions the
> > > dflags name being preferable.
> > >
> > > I prefer the plain flags name, if there is only one flag variable in the
> > > function.
> > >
> > > > > Can do it in 1 if you are happy with the flags declaration being
> > > > > removed in patch 1 and restored in 4.
> > > >
> > > > Definitely.
> > >
> > > Ok will re-arrange in v3.
> >
> > Hang on - patch 4 has to use a scoped_guard(), so are you ok for patch 1
> > to introduce a guard(), to avoid changing the indentation, only to
> > replace it with a scoped_guard(), to perform the tests after releasing
> > the lock, in patch 4?
> 
> Alternatively, I can move patch 4 to the top of the series.

I'm fine as long as we do one thing in one patch (not spread over a few
as it seems feasible).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 11:34:44PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:

...

> > > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > > >
> > > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > > instead of spinlock)?
> > >
> > > Read the cover letter.
> > > Doing that made the change larger, as flags gets removed then restored.
> > > I had also thought the flag tests would get indented then unindented, but
> > > if we use guard() the indentation should remain unchanged.
> >
> > I'm fine with that as I pointed out (have you received that mail? I had
> > problems with my mail server) the dflags is better semantically, so restoration
> > with _different_ name is fine.
> 
> I have noted that some of your replies have been delayed, and I can't be sure
> of what I might not've received. I can't say I've seen one that mentions the
> dflags name being preferable.
> 
> I prefer the plain flags name, if there is only one flag variable in the
> function.

I pointed out that lflags / dflags is kinda idiomatic internally to gpiolib*
code base. Using flags might feel misleading and otherwise will hint about
semantics of the variable. That said, I prefer it being named dflags.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()

[Kent Gibson]

On Thu, Dec 14, 2023 at 05:50:16PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 11:34:44PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:27:29PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 11:19:01PM +0800, Kent Gibson wrote:
> > > > On Thu, Dec 14, 2023 at 05:10:23PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:13PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > > > -	spin_lock_irqsave(&gpio_lock, flags);
> > > > >
> > > > > Shouldn't this be covered by patch 1 (I mean conversion to scoped_guard()
> > > > > instead of spinlock)?
> > > >
> > > > Read the cover letter.
> > > > Doing that made the change larger, as flags gets removed then restored.
> > > > I had also thought the flag tests would get indented then unindented, but
> > > > if we use guard() the indentation should remain unchanged.
> > >
> > > I'm fine with that as I pointed out (have you received that mail? I had
> > > problems with my mail server) the dflags is better semantically, so restoration
> > > with _different_ name is fine.
> >
> > I have noted that some of your replies have been delayed, and I can't be sure
> > of what I might not've received. I can't say I've seen one that mentions the
> > dflags name being preferable.
> >
> > I prefer the plain flags name, if there is only one flag variable in the
> > function.
>
> I pointed out that lflags / dflags is kinda idiomatic internally to gpiolib*
> code base. Using flags might feel misleading and otherwise will hint about
> semantics of the variable. That said, I prefer it being named dflags.
>

Application of that idiom in gpiolib-cdev looks to be mixed, but I guess
it can't hurt to lean into it.

Cheers,
Kent.

[PATCH v2 5/5] gpiolib: cdev: improve documentation of get/set values

[Kent Gibson]

Add documentation of the algorithm used to perform scatter/gather
of the requested lines and values in linereq_get_values() and
linereq_set_values_unlocked() to improve maintainability.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 73262305de0f..27cfed748b0a 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1391,9 +1391,18 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
 	if (copy_from_user(&lv, ip, sizeof(lv)))
 		return -EFAULT;
 
+	/*
+	 * gpiod_get_array_value_complex() requires compacted desc and val
+	 * arrays, rather than the sparse ones in lv.
+	 * Calculation of num_get and construction of the desc array is
+	 * optimized to avoid allocation for the desc array for the common
+	 * num_get == 1 case.
+	 */
+	/* scan requested lines to calculate the subset to get */
 	for (num_get = 0, i = 0; i < lr->num_lines; i++) {
 		if (lv.mask & BIT_ULL(i)) {
 			num_get++;
+			/* capture desc for the num_get == 1 case */
 			descs = &lr->lines[i].desc;
 		}
 	}
@@ -1402,6 +1411,7 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
 		return -EINVAL;
 
 	if (num_get != 1) {
+		/* build compacted desc array */
 		descs = kmalloc_array(num_get, sizeof(*descs), GFP_KERNEL);
 		if (!descs)
 			return -ENOMEM;
@@ -1422,6 +1432,7 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
 
 	lv.bits = 0;
 	for (didx = 0, i = 0; i < lr->num_lines; i++) {
+		/* unpack compacted vals for the response */
 		if (lv.mask & BIT_ULL(i)) {
 			if (lr->lines[i].sw_debounced)
 				val = debounced_value(&lr->lines[i]);
@@ -1447,14 +1458,25 @@ static long linereq_set_values_unlocked(struct linereq *lr,
 	unsigned int i, didx, num_set;
 	int ret;
 
+	/*
+	 * gpiod_set_array_value_complex() requires compacted desc and val
+	 * arrays, rather than the sparse ones in lv.
+	 * Calculation of num_set and construction of the descs and vals arrays
+	 * is optimized to minimize scanning the lv->mask, and to avoid
+	 * allocation for the desc array for the common num_set == 1 case.
+	 */
 	bitmap_zero(vals, GPIO_V2_LINES_MAX);
+	/* scan requested lines to determine the subset to be set */
 	for (num_set = 0, i = 0; i < lr->num_lines; i++) {
 		if (lv->mask & BIT_ULL(i)) {
+			/* setting inputs is not allowed */
 			if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags))
 				return -EPERM;
+			/* add to compacted values */
 			if (lv->bits & BIT_ULL(i))
 				__set_bit(num_set, vals);
 			num_set++;
+			/* capture desc for the num_set == 1 case */
 			descs = &lr->lines[i].desc;
 		}
 	}
@@ -1462,7 +1484,7 @@ static long linereq_set_values_unlocked(struct linereq *lr,
 		return -EINVAL;
 
 	if (num_set != 1) {
-		/* build compacted desc array and values */
+		/* build compacted desc array */
 		descs = kmalloc_array(num_set, sizeof(*descs), GFP_KERNEL);
 		if (!descs)
 			return -ENOMEM;
-- 
2.39.2

Re: [PATCH v2 5/5] gpiolib: cdev: improve documentation of get/set values

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 05:58:14PM +0800, Kent Gibson wrote:
> Add documentation of the algorithm used to perform scatter/gather
> of the requested lines and values in linereq_get_values() and
> linereq_set_values_unlocked() to improve maintainability.

I believe this is based on the old discussion in the thread where I proposed
the patch to convert this (unreadable in my opinion) code to use bitmap APIs.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] gpiolib: cdev: improve documentation of get/set values

[Kent Gibson]

On Thu, Dec 14, 2023 at 05:12:36PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:58:14PM +0800, Kent Gibson wrote:
> > Add documentation of the algorithm used to perform scatter/gather
> > of the requested lines and values in linereq_get_values() and
> > linereq_set_values_unlocked() to improve maintainability.
>
> I believe this is based on the old discussion in the thread where I proposed
> the patch to convert this (unreadable in my opinion) code to use bitmap APIs.
>

I believe you are correct - that the code is unreadable in your
opinion.  And I noted that you were the origin of this change in the
cover letter.

I'm not usually comforable with this level of hand holding, but I made
an exception in your case.

Cheers,
Kent.

Re: [PATCH v2 5/5] gpiolib: cdev: improve documentation of get/set values

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 11:23:01PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:12:36PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:58:14PM +0800, Kent Gibson wrote:
> > > Add documentation of the algorithm used to perform scatter/gather
> > > of the requested lines and values in linereq_get_values() and
> > > linereq_set_values_unlocked() to improve maintainability.
> >
> > I believe this is based on the old discussion in the thread where I proposed
> > the patch to convert this (unreadable in my opinion) code to use bitmap APIs.
> 
> I believe you are correct - that the code is unreadable in your
> opinion.  And I noted that you were the origin of this change in the
> cover letter.
> 
> I'm not usually comforable with this level of hand holding, but I made
> an exception in your case.

At least it helps somebody to understand that, thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 11:08:05PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:

...

> While I think of it, what tree should I be basing on?
> These patches are based on v6.7-rc5, and I'm not aware of any other
> changes they may contend with, but best to be on the right tree to be
> sure.

General rule is to base on the target subsystem tree. In this case
it's Bart's gpio/for-next AFAIU.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Bartosz Golaszewski]

On Thu, Dec 14, 2023 at 5:47 PM Andy Shevchenko <andy@kernel.org> wrote:
>
>
> On Thu, Dec 14, 2023 at 11:08:05PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > While I think of it, what tree should I be basing on?
> > These patches are based on v6.7-rc5, and I'm not aware of any other
> > changes they may contend with, but best to be on the right tree to be
> > sure.
>
> General rule is to base on the target subsystem tree. In this case
> it's Bart's gpio/for-next AFAIU.
>

Normally the patches should apply on top of

    git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next

Any conflicts between maintainer trees are handled upstream.

Bart

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > >   .tree = RB_ROOT,
> > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> 
> Double underscore typically means it's private and shouldn't be used.

Right, but when you have a struct you have no other means to initialize this
directly.

> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.

See, there are users of the __ initializers.

> > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
> 
> Yeah, so maybe we should use non-struct, global variables after all.

At least this will allow to get rid of (questionable) initcall.

> > > And I still haven't received the email you quote there.
> >
> > :-( I'm not sure we will get it, it most likely that I removed it already
> > and it has disappeared due to problems with email server...

-- 
With Best Regards,
Andy Shevchenko