This series contains minor improvements to gpiolib-cdev.
The banner change is relocating the debounce_period_us from gpiolib's
struct gpio_desc to cdev's struct line. Patch 1 stores the field
locally in cdev. Patch 2 removes the now unused field from gpiolib.
Patch 3 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.
Given 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.
Patches 1 and 3 introduce usage of guard() and scoped_guard() to cdev.
Patch 4 replaces any remaining discrete lock/unlock calls around
critical sections with guard() or scoped_guard().
Patch 5 is unrelated to debounce or info, but addresses Andy's
recent lamentation 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 v3 -> v4:
(changes other than using --histogram are to patch 1)
- use --histogram to generate patches.
- include cleanup.h.
- make supinfo_lock static.
- immediately return from supinfo_to_lineinfo() if line not found.
Changes v2 -> v3:
- reorder patches to move full adoption of guard()/scoped_guard() to
patch 4.
- use guard() rather than scoped_guard() where the scope extends to the
end of the function.
- split supinfo into supinfo_tree and supinfo_lock (patch 1).
- rename flags to dflags in gpio_desc_to_lineinfo() (patch 3).
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: 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: fully adopt guard() and scoped_guard()
gpiolib: cdev: improve documentation of get/set values
drivers/gpio/gpiolib-cdev.c | 391 +++++++++++++++++++++++-------------
drivers/gpio/gpiolib.c | 3 -
drivers/gpio/gpiolib.h | 5 -
3 files changed, 246 insertions(+), 153 deletions(-)
--
2.39.2
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
1 file changed, 132 insertions(+), 22 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 02ffda6c1e51..47197f6339c4 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -5,6 +5,7 @@
#include <linux/bitmap.h>
#include <linux/build_bug.h>
#include <linux/cdev.h>
+#include <linux/cleanup.h>
#include <linux/compat.h>
#include <linux/compiler.h>
#include <linux/device.h>
@@ -21,6 +22,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 +463,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_tree 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 +476,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 +485,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 +519,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 +560,17 @@ struct line {
#endif /* CONFIG_HTE */
};
+/*
+ * a rbtree of the struct lines containing supplemental info.
+ * 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 rb_root supinfo_tree = RB_ROOT;
+/* covers supinfo_tree */
+static DEFINE_SPINLOCK(supinfo_lock);
+
/**
* struct linereq - contains the state of a userspace line request
* @gdev: the GPIO device the line request pertains to
@@ -575,6 +600,95 @@ struct linereq {
struct line lines[] __counted_by(num_lines);
};
+static void supinfo_insert(struct line *line)
+{
+ struct rb_node **new = &(supinfo_tree.rb_node), *parent = NULL;
+ struct line *entry;
+
+ 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)
+{
+ 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;
+
+ guard(spinlock)(&supinfo_lock);
+
+ line = supinfo_find(desc);
+ if (!line)
+ return;
+
+ 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 +837,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 +978,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 +1060,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 +1139,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 +1164,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 +1206,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;
}
@@ -1573,6 +1686,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)
@@ -1580,10 +1694,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);
@@ -2274,8 +2392,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);
@@ -2341,14 +2457,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);
}
@@ -2455,6 +2563,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)
@@ -2545,6 +2654,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)
--
2.39.2
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 4e190be75dc2..ca2216621619 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2344,9 +2344,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
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 74 ++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 38 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 47197f6339c4..8c174dda622d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2390,74 +2390,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;
+ unsigned long dflags;
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));
+ dflags = 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, &dflags) ||
+ test_bit(FLAG_IS_HOGGED, &dflags) ||
+ test_bit(FLAG_USED_AS_IRQ, &dflags) ||
+ test_bit(FLAG_EXPORT, &dflags) ||
+ test_bit(FLAG_SYSFS, &dflags) ||
!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, &dflags))
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, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW;
- if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
+ if (test_bit(FLAG_OPEN_DRAIN, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN;
- if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
+ if (test_bit(FLAG_OPEN_SOURCE, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE;
- if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+ if (test_bit(FLAG_BIAS_DISABLE, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED;
- if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+ if (test_bit(FLAG_PULL_DOWN, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
- if (test_bit(FLAG_PULL_UP, &desc->flags))
+ if (test_bit(FLAG_PULL_UP, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
- if (test_bit(FLAG_EDGE_RISING, &desc->flags))
+ if (test_bit(FLAG_EDGE_RISING, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING;
- if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
+ if (test_bit(FLAG_EDGE_FALLING, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING;
- if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags))
+ if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &dflags))
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, &dflags))
info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
-
- spin_unlock_irqrestore(&gpio_lock, flags);
}
struct gpio_chardev_data {
--
2.39.2
Use guard() or scoped_guard() for critical sections rather than
discrete lock/unlock pairs.
Signed-off-by: Kent Gibson <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 139 ++++++++++++++----------------------
1 file changed, 55 insertions(+), 84 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8c174dda622d..68fa9714e643 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -739,13 +739,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
@@ -1478,18 +1478,13 @@ static long linereq_set_values_unlocked(struct linereq *lr,
static long linereq_set_values(struct linereq *lr, void __user *ip)
{
struct gpio_v2_line_values lv;
- int ret;
if (copy_from_user(&lv, ip, sizeof(lv)))
return -EFAULT;
- mutex_lock(&lr->config_mutex);
+ guard(mutex)(&lr->config_mutex);
- ret = linereq_set_values_unlocked(lr, &lv);
-
- mutex_unlock(&lr->config_mutex);
-
- return ret;
+ return linereq_set_values_unlocked(lr, &lv);
}
static long linereq_set_config_unlocked(struct linereq *lr,
@@ -1547,13 +1542,9 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
if (ret)
return ret;
- mutex_lock(&lr->config_mutex);
+ guard(mutex)(&lr->config_mutex);
- ret = linereq_set_config_unlocked(lr, &lc);
-
- mutex_unlock(&lr->config_mutex);
-
- return ret;
+ return linereq_set_config_unlocked(lr, &lc);
}
static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
@@ -1635,28 +1626,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;
+ 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;
}
- if (file->f_flags & O_NONBLOCK) {
- spin_unlock(&lr->wait.lock);
- return -EAGAIN;
- }
-
- 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
@@ -2006,28 +1991,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
@@ -2721,38 +2700,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;
- }
+ scoped_guard(spinlock, &cdev->wait.lock) {
+ if (kfifo_is_empty(&cdev->events)) {
+ if (bytes_read)
+ return bytes_read;
- if (file->f_flags & O_NONBLOCK) {
- spin_unlock(&cdev->wait.lock);
- return -EAGAIN;
- }
+ if (file->f_flags & O_NONBLOCK)
+ return -EAGAIN;
- ret = wait_event_interruptible_locked(cdev->wait,
- !kfifo_is_empty(&cdev->events));
- if (ret) {
- spin_unlock(&cdev->wait.lock);
- return ret;
+ 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
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 68fa9714e643..d4953787f361 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1385,9 +1385,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;
}
}
@@ -1396,6 +1405,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;
@@ -1416,6 +1426,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]);
@@ -1441,14 +1452,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;
}
}
@@ -1456,7 +1478,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
On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <[email protected]> wrote:
>
> This series contains minor improvements to gpiolib-cdev.
>
> The banner change is relocating the debounce_period_us from gpiolib's
> struct gpio_desc to cdev's struct line. Patch 1 stores the field
> locally in cdev. Patch 2 removes the now unused field from gpiolib.
>
> Patch 3 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.
> Given 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.
>
> Patches 1 and 3 introduce usage of guard() and scoped_guard() to cdev.
> Patch 4 replaces any remaining discrete lock/unlock calls around
> critical sections with guard() or scoped_guard().
>
> Patch 5 is unrelated to debounce or info, but addresses Andy's
> recent lamentation 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 v3 -> v4:
> (changes other than using --histogram are to patch 1)
> - use --histogram to generate patches.
> - include cleanup.h.
> - make supinfo_lock static.
> - immediately return from supinfo_to_lineinfo() if line not found.
>
> Changes v2 -> v3:
> - reorder patches to move full adoption of guard()/scoped_guard() to
> patch 4.
> - use guard() rather than scoped_guard() where the scope extends to the
> end of the function.
> - split supinfo into supinfo_tree and supinfo_lock (patch 1).
> - rename flags to dflags in gpio_desc_to_lineinfo() (patch 3).
>
> 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: 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: fully adopt guard() and scoped_guard()
> gpiolib: cdev: improve documentation of get/set values
>
> drivers/gpio/gpiolib-cdev.c | 391 +++++++++++++++++++++++-------------
> drivers/gpio/gpiolib.c | 3 -
> drivers/gpio/gpiolib.h | 5 -
> 3 files changed, 246 insertions(+), 153 deletions(-)
>
> --
> 2.39.2
>
I just have two minor nits for patch 1/5, other than that it's ready to go.
Bart
On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <[email protected]> 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 <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
> 1 file changed, 132 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 02ffda6c1e51..47197f6339c4 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -5,6 +5,7 @@
> #include <linux/bitmap.h>
> #include <linux/build_bug.h>
> #include <linux/cdev.h>
> +#include <linux/cleanup.h>
> #include <linux/compat.h>
> #include <linux/compiler.h>
> #include <linux/device.h>
> @@ -21,6 +22,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 +463,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_tree 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 +476,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 +485,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 +519,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 +560,17 @@ struct line {
> #endif /* CONFIG_HTE */
> };
>
> +/*
> + * a rbtree of the struct lines containing supplemental info.
> + * 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 rb_root supinfo_tree = RB_ROOT;
> +/* covers supinfo_tree */
> +static DEFINE_SPINLOCK(supinfo_lock);
> +
> /**
> * struct linereq - contains the state of a userspace line request
> * @gdev: the GPIO device the line request pertains to
> @@ -575,6 +600,95 @@ struct linereq {
> struct line lines[] __counted_by(num_lines);
> };
>
> +static void supinfo_insert(struct line *line)
> +{
> + struct rb_node **new = &(supinfo_tree.rb_node), *parent = NULL;
> + struct line *entry;
> +
> + 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)
> +{
> + 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;
> +
> + guard(spinlock)(&supinfo_lock);
> +
> + line = supinfo_find(desc);
> + if (!line)
> + return;
> +
> + 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)
Under v2 I suggested naming this line_has_suppinfo(). Any reason not
to do it? I think it's more logical than saying "line is
supplemental". The latter makes it seem as if certain line objects
would "supplement" some third party with something. What this really
checks is: does this line contain additional information.
> +{
> + 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);
Could you add a comment here saying it's called with the config mutex
taken as at first glance it looks racy but actually isn't?
Bart
> +}
> +
> #define GPIO_V2_LINE_BIAS_FLAGS \
> (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
> GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
> @@ -723,7 +837,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 +978,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 +1060,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 +1139,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 +1164,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 +1206,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;
> }
>
> @@ -1573,6 +1686,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)
> @@ -1580,10 +1694,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);
> @@ -2274,8 +2392,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);
> @@ -2341,14 +2457,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);
> }
>
> @@ -2455,6 +2563,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)
> @@ -2545,6 +2654,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)
> --
> 2.39.2
>
On Mon, Dec 18, 2023 at 04:24:50PM +0100, Bartosz Golaszewski wrote:
> On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <[email protected]> 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 <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > ---
> > drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
> > 1 file changed, 132 insertions(+), 22 deletions(-)
> >
> > +static inline bool line_is_supplemental(struct line *line)
>
> Under v2 I suggested naming this line_has_suppinfo(). Any reason not
> to do it? I think it's more logical than saying "line is
> supplemental". The latter makes it seem as if certain line objects
> would "supplement" some third party with something. What this really
> checks is: does this line contain additional information.
>
My bad - responded to your first comment and then missed the rest.
Agreed - the naming could be better. Will fix for v5.
> > +{
> > + 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);
>
> Could you add a comment here saying it's called with the config mutex
> taken as at first glance it looks racy but actually isn't?
>
Sure. Though it is also covered by the gdev->sem you added, right?
So the config_mutex is now redundant?
Should I document it is covered by both?
Or drop the config_mutex entirely?
And you wanted some comments to explain the logic?
I thought this is a common "has it changed" pattern, and so didn't
require additional explanation, but I guess not as common as I thought.
Cheers,
Kent.
On Mon, Dec 18, 2023 at 4:40 PM Kent Gibson <[email protected]> wrote:
>
> On Mon, Dec 18, 2023 at 04:24:50PM +0100, Bartosz Golaszewski wrote:
> > On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <[email protected]> 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 <[email protected]>
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> > > ---
> > > drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
> > > 1 file changed, 132 insertions(+), 22 deletions(-)
> > >
> > > +static inline bool line_is_supplemental(struct line *line)
> >
> > Under v2 I suggested naming this line_has_suppinfo(). Any reason not
> > to do it? I think it's more logical than saying "line is
> > supplemental". The latter makes it seem as if certain line objects
> > would "supplement" some third party with something. What this really
> > checks is: does this line contain additional information.
> >
>
>
> My bad - responded to your first comment and then missed the rest.
>
> Agreed - the naming could be better. Will fix for v5.
>
> > > +{
> > > + 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);
> >
> > Could you add a comment here saying it's called with the config mutex
> > taken as at first glance it looks racy but actually isn't?
> >
>
> Sure. Though it is also covered by the gdev->sem you added, right?
> So the config_mutex is now redundant?
> Should I document it is covered by both?
> Or drop the config_mutex entirely?
>
No! The semaphore is a read-write semaphore and we can have multiple
readers at once. This semaphore only protects us from the chip being
set to NULL during a system call. It will also go away once I get the
descriptor access serialized (or not - I'm figuring out a lockless
approach) and finally use SRCU to protect gdev->chip.
> And you wanted some comments to explain the logic?
> I thought this is a common "has it changed" pattern, and so didn't
> require additional explanation, but I guess not as common as I thought.
>
If line_set_debounce_period() could be called concurrently for the
same line, this approach would be racy. It cannot but I want a comment
here as I fear that if in the future we add some more attributes that
constitute "supplemental info" and which may be changed outside of the
config lock then this would in fact become racy.
Bart
> Cheers,
> Kent.
On Mon, Dec 18, 2023 at 05:01:32PM +0100, Bartosz Golaszewski wrote:
> On Mon, Dec 18, 2023 at 4:40 PM Kent Gibson <[email protected]> wrote:
> >
> > On Mon, Dec 18, 2023 at 04:24:50PM +0100, Bartosz Golaszewski wrote:
> > > On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <[email protected]> wrote:
> > > >
> > > > +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);
> > >
> > > Could you add a comment here saying it's called with the config mutex
> > > taken as at first glance it looks racy but actually isn't?
> > >
> >
> > Sure. Though it is also covered by the gdev->sem you added, right?
> > So the config_mutex is now redundant?
> > Should I document it is covered by both?
> > Or drop the config_mutex entirely?
> >
>
> No! The semaphore is a read-write semaphore and we can have multiple
> readers at once. This semaphore only protects us from the chip being
> set to NULL during a system call. It will also go away once I get the
> descriptor access serialized (or not - I'm figuring out a lockless
> approach) and finally use SRCU to protect gdev->chip.
>
Ah, so it is.
> > And you wanted some comments to explain the logic?
> > I thought this is a common "has it changed" pattern, and so didn't
> > require additional explanation, but I guess not as common as I thought.
> >
>
> If line_set_debounce_period() could be called concurrently for the
> same line, this approach would be racy. It cannot but I want a comment
> here as I fear that if in the future we add some more attributes that
> constitute "supplemental info" and which may be changed outside of the
> config lock then this would in fact become racy.
>
If any line config is going to be changed from the userspace side then
it will be by the SET_CONFIG ioctl, and so be covered by the config_mutex,
but it can't hurt to explicitly document it here as well.
Cheers,
Kent.