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. The first patch stores the
field locally in cdev. The second removes the now unused field from
gpiolib.
The third patch 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.
The fourth patch is unrelated to debounce or info, but addresses Andy's
recent assertion that the linereq get/set values functions are confusing
and under documented. Figured I may as well add that while I was in
there.
Kent Gibson (4):
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 | 257 ++++++++++++++++++++++++++++--------
drivers/gpio/gpiolib.c | 3 -
drivers/gpio/gpiolib.h | 5 -
3 files changed, 201 insertions(+), 64 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]>
---
drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++-----
1 file changed, 146 insertions(+), 21 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 02ffda6c1e51..7999c1a72cfa 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>
@@ -462,6 +463,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
/**
* struct line - contains the state of a requested line
* @desc: the GPIO descriptor for this line.
+ * @node: to store the object in supinfo if supplemental
* @req: the corresponding line request
* @irq: the interrupt triggered in response to events on this GPIO
* @edflags: the edge flags, GPIO_V2_LINE_FLAG_EDGE_RISING and/or
@@ -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
@@ -482,6 +485,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
*/
struct line {
struct gpio_desc *desc;
+ struct rb_node node;
/*
* -- 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,22 @@ struct line {
#endif /* CONFIG_HTE */
};
+/**
+ * struct supinfo - supplementary line 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().
+ * @lock: lock covering @tree
+ * @tree: a rbtree of the struct lines containing the supplemental info
+ */
+struct supinfo {
+ spinlock_t lock;
+ struct rb_root tree;
+};
+
+static struct supinfo supinfo;
+
/**
* struct linereq - contains the state of a userspace line request
* @gdev: the GPIO device the line request pertains to
@@ -575,6 +604,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;
+
+ spin_lock(&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 {
+ pr_warn("%s: duplicate line inserted\n", __func__);
+ goto out_unlock;
+ }
+ }
+
+ rb_link_node(&line->node, parent, new);
+ rb_insert_color(&line->node, &supinfo.tree);
+out_unlock:
+ spin_unlock(&supinfo.lock);
+}
+
+static void supinfo_erase(struct line *line)
+{
+ spin_lock(&supinfo.lock);
+ rb_erase(&line->node, &supinfo.tree);
+ spin_unlock(&supinfo.lock);
+}
+
+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;
+
+ spin_lock(&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++;
+ }
+ spin_unlock(&supinfo.lock);
+}
+
+static inline bool line_is_supplemental(struct line *line)
+{
+ return READ_ONCE(line->debounce_period_us) != 0;
+}
+
+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 +846,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 +987,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 +1069,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 +1148,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 +1173,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 +1215,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 +1695,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,9 +1703,12 @@ 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) {
+ edge_detector_stop(line);
+ if (line_is_supplemental(line))
+ supinfo_erase(line);
+ gpiod_free(line->desc);
}
}
kfifo_free(&lr->events);
@@ -2274,8 +2400,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 +2465,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 +2571,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 +2662,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)
@@ -2812,3 +2930,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
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 availabilty 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]>
---
drivers/gpio/gpiolib-cdev.c | 66 ++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 7999c1a72cfa..37f2c9acc770 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2398,22 +2398,12 @@ 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 iflags, 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);
-
- spin_lock_irqsave(&gpio_lock, flags);
+ spin_lock_irqsave(&gpio_lock, iflags);
if (desc->name)
strscpy(info->name, desc->name, sizeof(info->name));
@@ -2421,51 +2411,59 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
if (desc->label)
strscpy(info->consumer, desc->label, sizeof(info->consumer));
+ dflags = READ_ONCE(desc->flags);
+
+ spin_unlock_irqrestore(&gpio_lock, iflags);
+
/*
- * 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 incorrect then it looks to the user like they performed the read
+ * on the wrong 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
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]>
---
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 37f2c9acc770..6878da5056f9 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1394,9 +1394,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;
}
}
@@ -1405,6 +1414,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;
@@ -1425,6 +1435,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]);
@@ -1450,14 +1461,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;
}
}
@@ -1465,7 +1487,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
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]>
---
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
On Tue, Dec 12, 2023 at 6:43 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. The first patch stores the
> field locally in cdev. The second removes the now unused field from
> gpiolib.
>
> The third patch 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.
>
> The fourth patch is unrelated to debounce or info, but addresses Andy's
> recent assertion that the linereq get/set values functions are confusing
> and under documented. Figured I may as well add that while I was in
> there.
>
> Kent Gibson (4):
> 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 | 257 ++++++++++++++++++++++++++++--------
> drivers/gpio/gpiolib.c | 3 -
> drivers/gpio/gpiolib.h | 5 -
> 3 files changed, 201 insertions(+), 64 deletions(-)
>
> --
> 2.39.2
>
Patches 2-4 look fine, I was about to review patch 1 in detail but I
thought I'd just throw this one in here before we commit to a specific
solution.
For some reason I thought this would not work but I'm now considering
it as an alternative approach: is there anything wrong with adding
struct kref to struct line, allocating it separately per-line when
gpio_chardev_data is created, referencing it from struct linereq when
the line is being requested, and dropping the reference from
gpio_chardev_data and linereq when either is being removed? Other than
the increased number of allocations?
Bartosz
On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 12, 2023 at 6:43 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. The first patch stores the
> > field locally in cdev. The second removes the now unused field from
> > gpiolib.
> >
> > The third patch 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.
> >
> > The fourth patch is unrelated to debounce or info, but addresses Andy's
> > recent assertion that the linereq get/set values functions are confusing
> > and under documented. Figured I may as well add that while I was in
> > there.
> >
> > Kent Gibson (4):
> > 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 | 257 ++++++++++++++++++++++++++++--------
> > drivers/gpio/gpiolib.c | 3 -
> > drivers/gpio/gpiolib.h | 5 -
> > 3 files changed, 201 insertions(+), 64 deletions(-)
> >
> > --
> > 2.39.2
> >
>
> Patches 2-4 look fine, I was about to review patch 1 in detail but I
> thought I'd just throw this one in here before we commit to a specific
> solution.
>
> For some reason I thought this would not work but I'm now considering
> it as an alternative approach: is there anything wrong with adding
> struct kref to struct line, allocating it separately per-line when
> gpio_chardev_data is created, referencing it from struct linereq when
> the line is being requested, and dropping the reference from
> gpio_chardev_data and linereq when either is being removed? Other than
> the increased number of allocations?
>
The collection of struct line always has to be global, right, as both
gpio_chardev_data and linereq are ephemeral. e.g. if one process requests
a line and another checks the lineinfo, those will have distinct
gpio_chardev_data.
But the key issue is that the linereq and struct line lifetimes are
strictly tied - a struct line does not live beyond the containing linereq.
Leaving the struct line alive after the linereq is released is just wrong.
The line has been released back to gpiolib so there can be no
supplemental info left.
If you want any such info to persist beyond the line release then it
should be located in gpiolib itself, not cdev.
Cheers,
Kent.
On Wed, Dec 13, 2023 at 12:58 AM Kent Gibson <[email protected]> wrote:
>
> On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote:
[snip]
> >
> > Patches 2-4 look fine, I was about to review patch 1 in detail but I
> > thought I'd just throw this one in here before we commit to a specific
> > solution.
> >
> > For some reason I thought this would not work but I'm now considering
> > it as an alternative approach: is there anything wrong with adding
> > struct kref to struct line, allocating it separately per-line when
> > gpio_chardev_data is created, referencing it from struct linereq when
> > the line is being requested, and dropping the reference from
> > gpio_chardev_data and linereq when either is being removed? Other than
> > the increased number of allocations?
> >
>
> The collection of struct line always has to be global, right, as both
> gpio_chardev_data and linereq are ephemeral. e.g. if one process requests
> a line and another checks the lineinfo, those will have distinct
> gpio_chardev_data.
>
Strictly speaking at least the supplemental info has to be globally
accessible. But I get your point.
> But the key issue is that the linereq and struct line lifetimes are
> strictly tied - a struct line does not live beyond the containing linereq.
I was thinking about decoupling one from the other actually.
> Leaving the struct line alive after the linereq is released is just wrong.
> The line has been released back to gpiolib so there can be no
> supplemental info left.
Indeed.
> If you want any such info to persist beyond the line release then it
> should be located in gpiolib itself, not cdev.
>
Right, we even zero debounce_period_us anyway on line release - just
as if we released struct line.
Bart
On Wed, Dec 13, 2023 at 11:03:40AM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 13, 2023 at 12:58 AM Kent Gibson <[email protected]> wrote:
> >
> > On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote:
>
> [snip]
>
> > >
> > > Patches 2-4 look fine, I was about to review patch 1 in detail but I
> > > thought I'd just throw this one in here before we commit to a specific
> > > solution.
> > >
> > > For some reason I thought this would not work but I'm now considering
> > > it as an alternative approach: is there anything wrong with adding
> > > struct kref to struct line, allocating it separately per-line when
> > > gpio_chardev_data is created, referencing it from struct linereq when
> > > the line is being requested, and dropping the reference from
> > > gpio_chardev_data and linereq when either is being removed? Other than
> > > the increased number of allocations?
> > >
> >
> > The collection of struct line always has to be global, right, as both
> > gpio_chardev_data and linereq are ephemeral. e.g. if one process requests
> > a line and another checks the lineinfo, those will have distinct
> > gpio_chardev_data.
> >
>
> Strictly speaking at least the supplemental info has to be globally
> accessible. But I get your point.
>
> > But the key issue is that the linereq and struct line lifetimes are
> > strictly tied - a struct line does not live beyond the containing linereq.
>
> I was thinking about decoupling one from the other actually.
>
I was also headed down that path - making the supplemental info for each
line distinct from the struct line. But then I realised that the lifetime
is strictly tied to the linereq, as per the struct line, and there was no
benefit in a separate object - just more overhead.
Cheers,
Kent.
On Tue, Dec 12, 2023 at 01:42:50PM +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.
...
> struct line {
> struct gpio_desc *desc;
> + struct rb_node node;
If you swap them, would it benefit in a code generation (bloat-o-meter)?
> };
...
> +struct supinfo {
> + spinlock_t lock;
> + struct rb_root tree;
> +};
Same Q.
...
> +static struct supinfo supinfo;
Why supinfo should be a struct to begin with? Seems to me as an unneeded
complication.
...
> + pr_warn("%s: duplicate line inserted\n", __func__);
I hope at bare minimum we have pr_fmt(), but even though this is poor message
that might require some information about exact duplication (GPIO chip label /
name, line number, etc). Generally speaking the __func__ in non-debug messages
_usually_ is a symptom of poorly written message.
...
> +out_unlock:
> + spin_unlock(&supinfo.lock);
No use of cleanup.h?
...
> +static inline bool line_is_supplemental(struct line *line)
> +{
> + return READ_ONCE(line->debounce_period_us) != 0;
" != 0" is redundant.
> +}
...
> 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) {
Perhaps
if (!line->desc)
continue;
?
> + edge_detector_stop(line);
> + if (line_is_supplemental(line))
> + supinfo_erase(line);
> + gpiod_free(line->desc);
> }
> }
...
> +static int __init gpiolib_cdev_init(void)
> +{
> + supinfo_init();
> + return 0;
> +}
It's a good practice to explain initcalls (different to the default ones),
can you add a comment on top to explain the choice of this initcall, please?
> +postcore_initcall(gpiolib_cdev_init);
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 12, 2023 at 01:42:52PM +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 availabilty of the line from pinctrl if other checks
> pass, so avoiding the check for lines that are otherwise in use.
...
> - bool ok_for_pinctrl;
> - unsigned long flags;
> + unsigned long iflags, dflags;
With a preliminary conversion to cleanup.h this whole change becomes cleaner,
no?
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 03:56:27PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 12, 2023 at 01:42:52PM +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 availabilty of the line from pinctrl if other checks
> > pass, so avoiding the check for lines that are otherwise in use.
>
> ...
>
> > - bool ok_for_pinctrl;
> > - unsigned long flags;
> > + unsigned long iflags, dflags;
>
> With a preliminary conversion to cleanup.h this whole change becomes cleaner,
> no?
>
You mean the scoped guards? Dunno - haven't used them.
Care to provide more detail?
Cheers,
Kent.
On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 12, 2023 at 01:42:50PM +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.
>
> ...
>
> > struct line {
> > struct gpio_desc *desc;
> > + struct rb_node node;
>
> If you swap them, would it benefit in a code generation (bloat-o-meter)?
>
Didn't consider that placement within the scruct could impact code
generation.
Having the rb_nodes at the beginning of struct is preferable?
> > };
>
> ...
>
> > +struct supinfo {
> > + spinlock_t lock;
> > + struct rb_root tree;
> > +};
>
> Same Q.
>
Same - I tend to put locks before the field(s) they cover.
But if the node being first results in nicer code then happy to swap.
> ...
>
> > +static struct supinfo supinfo;
>
> Why supinfo should be a struct to begin with? Seems to me as an unneeded
> complication.
>
Yeah, that is a hangover from an earlier iteration where supinfo was
contained in other object rather than being a global.
Could merge the struct definition into the variable now.
> ...
>
> > + pr_warn("%s: duplicate line inserted\n", __func__);
>
> I hope at bare minimum we have pr_fmt(), but even though this is poor message
> that might require some information about exact duplication (GPIO chip label /
> name, line number, etc). Generally speaking the __func__ in non-debug messages
> _usually_ is a symptom of poorly written message.
>
> ...
Yeah, I wasn't sure about the best way to log here.
The details of chip or line etc don't add anything - seeing this error
means there is a logic error in the code - we have inserted a line
without erasing it. Knowing which chip or line it happened to occur on
wont help debug it. It should never happen, but you can't just leave it
unhandled, so I went with a basic log.
>
> > +out_unlock:
> > + spin_unlock(&supinfo.lock);
>
> No use of cleanup.h?
>
Again, that is new to me, so no not yet.
> ...
>
> > +static inline bool line_is_supplemental(struct line *line)
> > +{
> > + return READ_ONCE(line->debounce_period_us) != 0;
>
> " != 0" is redundant.
>
I prefer conversion from int to bool to be explicit, but if you
insist...
> > +}
>
> ...
>
> > 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) {
>
> Perhaps
>
> if (!line->desc)
> continue;
>
> ?
Seems reasonable - I was just going with what was already there.
>
> > + edge_detector_stop(line);
> > + if (line_is_supplemental(line))
> > + supinfo_erase(line);
> > + gpiod_free(line->desc);
> > }
> > }
>
> ...
>
> > +static int __init gpiolib_cdev_init(void)
> > +{
> > + supinfo_init();
> > + return 0;
> > +}
>
> It's a good practice to explain initcalls (different to the default ones),
> can you add a comment on top to explain the choice of this initcall, please?
>
Not sure what you mean. This section used gpiolib-sysfs as a template,
and that has no documentation.
> > +postcore_initcall(gpiolib_cdev_init);
>
Thanks for the review - always instructive.
Cheers,
Kent.
On Wed, Dec 13, 2023 at 10:07:28PM +0800, Kent Gibson wrote:
> On Wed, Dec 13, 2023 at 03:56:27PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 12, 2023 at 01:42:52PM +0800, Kent Gibson wrote:
...
> > > - unsigned long flags;
> > > + unsigned long iflags, dflags;
> >
> > With a preliminary conversion to cleanup.h this whole change becomes cleaner,
> > no?
>
> You mean the scoped guards? Dunno - haven't used them.
> Care to provide more detail?
Yes.
With use of those the flags will gone and you won't need iflags,
which is non-standard name for the spin lock flags variable anyway,
at all. Only new dflags will come here.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 10:07:28PM +0800, Kent Gibson wrote:
> On Wed, Dec 13, 2023 at 03:56:27PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 12, 2023 at 01:42:52PM +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 availabilty of the line from pinctrl if other checks
> > > pass, so avoiding the check for lines that are otherwise in use.
> >
> > ...
> >
> > > - bool ok_for_pinctrl;
> > > - unsigned long flags;
> > > + unsigned long iflags, dflags;
> >
> > With a preliminary conversion to cleanup.h this whole change becomes cleaner,
> > no?
> >
>
> You mean the scoped guards? Dunno - haven't used them.
> Care to provide more detail?
>
Ok, so changing the spin_lock/unlock to
scoped_guard(spinlock_irqsave, &gpio_lock) {
...
}
you no longer need the iflags at all, and can rename dflags to flags.
Got it.
Cheers,
Kent.
On Wed, Dec 13, 2023 at 11:11:34PM +0800, Kent Gibson wrote:
> On Wed, Dec 13, 2023 at 10:07:28PM +0800, Kent Gibson wrote:
> > On Wed, Dec 13, 2023 at 03:56:27PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 12, 2023 at 01:42:52PM +0800, Kent Gibson wrote:
...
> > > > - unsigned long flags;
> > > > + unsigned long iflags, dflags;
> > >
> > > With a preliminary conversion to cleanup.h this whole change becomes cleaner,
> > > no?
> >
> > You mean the scoped guards? Dunno - haven't used them.
> > Care to provide more detail?
>
> Ok, so changing the spin_lock/unlock to
>
> scoped_guard(spinlock_irqsave, &gpio_lock) {
> ...
> }
>
> you no longer need the iflags at all, and can rename dflags to flags.
Yes, but I prefer still dflags as it is a distinction from the lock flags
and we use lflags/dflags a lot in GPIO library, so reading the code will
give a hint about the semantics.
> Got it.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 12, 2023 at 01:42:50PM +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.
> >
> > ...
> >
> > > struct line {
> > > struct gpio_desc *desc;
> > > + struct rb_node node;
> >
> > If you swap them, would it benefit in a code generation (bloat-o-meter)?
> >
>
> Didn't consider that placement within the scruct could impact code
> generation.
> Having the rb_nodes at the beginning of struct is preferable?
>
I suppose it has something to do with 0 offset when using
container_of(). Not sure if that really matters though.
> > > };
> >
> > ...
> >
> > > +struct supinfo {
> > > + spinlock_t lock;
> > > + struct rb_root tree;
> > > +};
> >
> > Same Q.
> >
>
> Same - I tend to put locks before the field(s) they cover.
> But if the node being first results in nicer code then happy to swap.
>
> > ...
> >
> > > +static struct supinfo supinfo;
> >
> > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > complication.
> >
I think we should keep it as a struct but defined the following way:
struct {
spinlock_t lock;
struct rb_root tree;
} supinfo;
>
> Yeah, that is a hangover from an earlier iteration where supinfo was
> contained in other object rather than being a global.
> Could merge the struct definition into the variable now.
>
> > ...
> >
> > > + pr_warn("%s: duplicate line inserted\n", __func__);
> >
> > I hope at bare minimum we have pr_fmt(), but even though this is poor message
> > that might require some information about exact duplication (GPIO chip label /
> > name, line number, etc). Generally speaking the __func__ in non-debug messages
> > _usually_ is a symptom of poorly written message.
> >
> > ...
>
> Yeah, I wasn't sure about the best way to log here.
>
> The details of chip or line etc don't add anything - seeing this error
> means there is a logic error in the code - we have inserted a line
> without erasing it. Knowing which chip or line it happened to occur on
> wont help debug it. It should never happen, but you can't just leave it
> unhandled, so I went with a basic log.
>
We should yell loudly in that case - use one of the WARN() variants
that'll print a stack trace too and point you to the relevant line in
the code.
> >
> > > +out_unlock:
> > > + spin_unlock(&supinfo.lock);
> >
> > No use of cleanup.h?
> >
>
> Again, that is new to me, so no not yet.
>
Yep, please use a guard, they're awesome. :)
> > ...
> >
> > > +static inline bool line_is_supplemental(struct line *line)
> > > +{
> > > + return READ_ONCE(line->debounce_period_us) != 0;
> >
> > " != 0" is redundant.
> >
>
> I prefer conversion from int to bool to be explicit, but if you
> insist...
>
> > > +}
> >
> > ...
> >
> > > 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) {
> >
> > Perhaps
> >
> > if (!line->desc)
> > continue;
> >
> > ?
>
> Seems reasonable - I was just going with what was already there.
>
> >
> > > + edge_detector_stop(line);
> > > + if (line_is_supplemental(line))
> > > + supinfo_erase(line);
> > > + gpiod_free(line->desc);
> > > }
> > > }
> >
> > ...
> >
> > > +static int __init gpiolib_cdev_init(void)
> > > +{
> > > + supinfo_init();
> > > + return 0;
> > > +}
> >
> > It's a good practice to explain initcalls (different to the default ones),
> > can you add a comment on top to explain the choice of this initcall, please?
> >
>
> Not sure what you mean. This section used gpiolib-sysfs as a template,
> and that has no documentation.
>
> > > +postcore_initcall(gpiolib_cdev_init);
> >
>
> Thanks for the review - always instructive.
>
> Cheers,
> Kent.
Bart
On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> >
> > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 12, 2023 at 01:42:50PM +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.
> > >
> > > ...
> > >
> > > > struct line {
> > > > struct gpio_desc *desc;
> > > > + struct rb_node node;
> > >
> > > If you swap them, would it benefit in a code generation (bloat-o-meter)?
> > >
> >
> > Didn't consider that placement within the scruct could impact code
> > generation.
> > Having the rb_nodes at the beginning of struct is preferable?
> >
>
> I suppose it has something to do with 0 offset when using
> container_of(). Not sure if that really matters though.
>
There are other fields that get the container_of() treatment, but node
does look to be the one most used, so probably makes sense to put it
first.
> > > > };
> > >
> > > ...
> > >
> > > > +struct supinfo {
> > > > + spinlock_t lock;
> > > > + struct rb_root tree;
> > > > +};
> > >
> > > Same Q.
> > >
> >
> > Same - I tend to put locks before the field(s) they cover.
> > But if the node being first results in nicer code then happy to swap.
> >
> > > ...
> > >
> > > > +static struct supinfo supinfo;
> > >
> > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > complication.
> > >
>
> I think we should keep it as a struct but defined the following way:
>
> struct {
> spinlock_t lock;
> struct rb_root tree;
> } supinfo;
That is what I meant be merging the struct definition with the variable
definition. Or is there some other way to completely do away with the
struct that I'm missing?
> >
> > Yeah, that is a hangover from an earlier iteration where supinfo was
> > contained in other object rather than being a global.
> > Could merge the struct definition into the variable now.
> >
> > > ...
> > >
> > > > + pr_warn("%s: duplicate line inserted\n", __func__);
> > >
> > > I hope at bare minimum we have pr_fmt(), but even though this is poor message
> > > that might require some information about exact duplication (GPIO chip label /
> > > name, line number, etc). Generally speaking the __func__ in non-debug messages
> > > _usually_ is a symptom of poorly written message.
> > >
> > > ...
> >
> > Yeah, I wasn't sure about the best way to log here.
> >
> > The details of chip or line etc don't add anything - seeing this error
> > means there is a logic error in the code - we have inserted a line
> > without erasing it. Knowing which chip or line it happened to occur on
> > wont help debug it. It should never happen, but you can't just leave it
> > unhandled, so I went with a basic log.
> >
>
> We should yell loudly in that case - use one of the WARN() variants
> that'll print a stack trace too and point you to the relevant line in
> the code.
>
Ok, so any suggestion as to which WARN() variant would make the most sense?
> > >
> > > > +out_unlock:
> > > > + spin_unlock(&supinfo.lock);
> > >
> > > No use of cleanup.h?
> > >
> >
> > Again, that is new to me, so no not yet.
> >
>
> Yep, please use a guard, they're awesome. :)
>
Will do.
Thanks,
Kent.
On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote:
> On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote:
...
> > > > > +static struct supinfo supinfo;
> > > >
> > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > complication.
> >
> > I think we should keep it as a struct but defined the following way:
> >
> > struct {
> > spinlock_t lock;
> > struct rb_root tree;
> > } supinfo;
>
> That is what I meant be merging the struct definition with the variable
> definition. Or is there some other way to completely do away with the
> struct that I'm missing?
Look at the top of gpiolib.c:
static DEFINE_MUTEX(gpio_lookup_lock);
static LIST_HEAD(gpio_lookup_list);
In the similar way you can simply do
static DEFINE_SPINLOCK(gpio_sup_lock);
static struct rb_root gpio_sup_tree;
> > > Yeah, that is a hangover from an earlier iteration where supinfo was
> > > contained in other object rather than being a global.
> > > Could merge the struct definition into the variable now.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 4:59 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Dec 12, 2023 at 01:42:50PM +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.
> > > >
> > > > ...
> > > >
> > > > > struct line {
> > > > > struct gpio_desc *desc;
> > > > > + struct rb_node node;
> > > >
> > > > If you swap them, would it benefit in a code generation (bloat-o-meter)?
> > > >
> > >
> > > Didn't consider that placement within the scruct could impact code
> > > generation.
> > > Having the rb_nodes at the beginning of struct is preferable?
> > >
> >
> > I suppose it has something to do with 0 offset when using
> > container_of(). Not sure if that really matters though.
> >
>
> There are other fields that get the container_of() treatment, but node
> does look to be the one most used, so probably makes sense to put it
> first.
>
> > > > > };
> > > >
> > > > ...
> > > >
> > > > > +struct supinfo {
> > > > > + spinlock_t lock;
> > > > > + struct rb_root tree;
> > > > > +};
> > > >
> > > > Same Q.
> > > >
> > >
> > > Same - I tend to put locks before the field(s) they cover.
> > > But if the node being first results in nicer code then happy to swap.
> > >
> > > > ...
> > > >
> > > > > +static struct supinfo supinfo;
> > > >
> > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > complication.
> > > >
> >
> > I think we should keep it as a struct but defined the following way:
> >
> > struct {
> > spinlock_t lock;
> > struct rb_root tree;
> > } supinfo;
>
> That is what I meant be merging the struct definition with the variable
> definition. Or is there some other way to completely do away with the
> struct that I'm missing?
No, I also meant that.
>
> > >
> > > Yeah, that is a hangover from an earlier iteration where supinfo was
> > > contained in other object rather than being a global.
> > > Could merge the struct definition into the variable now.
> > >
> > > > ...
> > > >
> > > > > + pr_warn("%s: duplicate line inserted\n", __func__);
> > > >
> > > > I hope at bare minimum we have pr_fmt(), but even though this is poor message
> > > > that might require some information about exact duplication (GPIO chip label /
> > > > name, line number, etc). Generally speaking the __func__ in non-debug messages
> > > > _usually_ is a symptom of poorly written message.
> > > >
> > > > ...
> > >
> > > Yeah, I wasn't sure about the best way to log here.
> > >
> > > The details of chip or line etc don't add anything - seeing this error
> > > means there is a logic error in the code - we have inserted a line
> > > without erasing it. Knowing which chip or line it happened to occur on
> > > wont help debug it. It should never happen, but you can't just leave it
> > > unhandled, so I went with a basic log.
> > >
> >
> > We should yell loudly in that case - use one of the WARN() variants
> > that'll print a stack trace too and point you to the relevant line in
> > the code.
> >
>
> Ok, so any suggestion as to which WARN() variant would make the most sense?
>
Just a regular WARN(1, "msg ...");
> > > >
> > > > > +out_unlock:
> > > > > + spin_unlock(&supinfo.lock);
> > > >
> > > > No use of cleanup.h?
> > > >
> > >
> > > Again, that is new to me, so no not yet.
> > >
> >
> > Yep, please use a guard, they're awesome. :)
> >
>
> Will do.
>
> Thanks,
> Kent.
Bart
On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> >
>
> > >
> > > > +out_unlock:
> > > > + spin_unlock(&supinfo.lock);
> > >
> > > No use of cleanup.h?
> > >
> >
> > Again, that is new to me, so no not yet.
> >
>
> Yep, please use a guard, they're awesome. :)
>
Before I go nuts and switch everything over, is it ok to put a return
within the scoped_guard - it will automatically unlock on the way out?
Cheers,
Kent.
On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote:
> > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > > > +static struct supinfo supinfo;
> > > > >
> > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > > complication.
> > >
> > > I think we should keep it as a struct but defined the following way:
> > >
> > > struct {
> > > spinlock_t lock;
> > > struct rb_root tree;
> > > } supinfo;
> >
> > That is what I meant be merging the struct definition with the variable
> > definition. Or is there some other way to completely do away with the
> > struct that I'm missing?
>
> Look at the top of gpiolib.c:
>
> static DEFINE_MUTEX(gpio_lookup_lock);
> static LIST_HEAD(gpio_lookup_list);
>
> In the similar way you can simply do
>
> static DEFINE_SPINLOCK(gpio_sup_lock);
> static struct rb_root gpio_sup_tree;
>
The fact that this has been like this, doesn't mean it's the only
right way. IMO putting these into the same structure makes logical
sense.
Bart
> > > > Yeah, that is a hangover from an earlier iteration where supinfo was
> > > > contained in other object rather than being a global.
> > > > Could merge the struct definition into the variable now.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Wed, Dec 13, 2023 at 5:15 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > >
> >
> > > >
> > > > > +out_unlock:
> > > > > + spin_unlock(&supinfo.lock);
> > > >
> > > > No use of cleanup.h?
> > > >
> > >
> > > Again, that is new to me, so no not yet.
> > >
> >
> > Yep, please use a guard, they're awesome. :)
> >
>
> Before I go nuts and switch everything over, is it ok to put a return
> within the scoped_guard - it will automatically unlock on the way out?
>
Yes! This is precisely why they're so great.
Bart
> Cheers,
> Kent.
On Thu, Dec 14, 2023 at 12:15:10AM +0800, Kent Gibson wrote:
> On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > >
> > > > > +out_unlock:
> > > > > + spin_unlock(&supinfo.lock);
> > > >
> > > > No use of cleanup.h?
> > >
> > > Again, that is new to me, so no not yet.
> >
> > Yep, please use a guard, they're awesome. :)
>
> Before I go nuts and switch everything over, is it ok to put a return
> within the scoped_guard - it will automatically unlock on the way out?
Yes, should do that.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 05:15:38PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <[email protected]> wrote:
> > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote:
> > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote:
...
> > > > > > > +static struct supinfo supinfo;
> > > > > >
> > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > > > complication.
> > > >
> > > > I think we should keep it as a struct but defined the following way:
> > > >
> > > > struct {
> > > > spinlock_t lock;
> > > > struct rb_root tree;
> > > > } supinfo;
> > >
> > > That is what I meant be merging the struct definition with the variable
> > > definition. Or is there some other way to completely do away with the
> > > struct that I'm missing?
> >
> > Look at the top of gpiolib.c:
> >
> > static DEFINE_MUTEX(gpio_lookup_lock);
> > static LIST_HEAD(gpio_lookup_list);
> >
> > In the similar way you can simply do
> >
> > static DEFINE_SPINLOCK(gpio_sup_lock);
> > static struct rb_root gpio_sup_tree;
>
> The fact that this has been like this, doesn't mean it's the only
> right way. IMO putting these into the same structure makes logical
> sense.
I disagree on the struct like this on the grounds:
- it's global
- it's one time use
- it adds complications for no benefit
- it makes code uglier and longer
What we probably want to have is a singleton objects in C language or at least
inside Linux kernel project. But I'm not sure it's feasible.
> > > > > Yeah, that is a hangover from an earlier iteration where supinfo was
> > > > > contained in other object rather than being a global.
> > > > > Could merge the struct definition into the variable now.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 5:29 PM Andy Shevchenko <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 05:15:38PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <[email protected]> wrote:
> > > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote:
> > > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > > > > > +static struct supinfo supinfo;
> > > > > > >
> > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > > > > complication.
> > > > >
> > > > > I think we should keep it as a struct but defined the following way:
> > > > >
> > > > > struct {
> > > > > spinlock_t lock;
> > > > > struct rb_root tree;
> > > > > } supinfo;
> > > >
> > > > That is what I meant be merging the struct definition with the variable
> > > > definition. Or is there some other way to completely do away with the
> > > > struct that I'm missing?
> > >
> > > Look at the top of gpiolib.c:
> > >
> > > static DEFINE_MUTEX(gpio_lookup_lock);
> > > static LIST_HEAD(gpio_lookup_list);
> > >
> > > In the similar way you can simply do
> > >
> > > static DEFINE_SPINLOCK(gpio_sup_lock);
> > > static struct rb_root gpio_sup_tree;
> >
> > The fact that this has been like this, doesn't mean it's the only
> > right way. IMO putting these into the same structure makes logical
> > sense.
>
> I disagree on the struct like this on the grounds:
> - it's global
> - it's one time use
> - it adds complications for no benefit
> - it makes code uglier and longer
>
It boils down to supinfo.lock vs supinfo_lock. I do prefer the former
but it's a weak opinion, I won't die on that hill.
Bart
> What we probably want to have is a singleton objects in C language or at least
> inside Linux kernel project. But I'm not sure it's feasible.
>
> > > > > > Yeah, that is a hangover from an earlier iteration where supinfo was
> > > > > > contained in other object rather than being a global.
> > > > > > Could merge the struct definition into the variable now.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 13, 2023 at 5:29 PM Andy Shevchenko <[email protected]> wrote:
> > On Wed, Dec 13, 2023 at 05:15:38PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <[email protected]> wrote:
> > > > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote:
> > > > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <[email protected]> wrote:
> > > > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote:
> > > > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote:
...
> > > > > > > > > +static struct supinfo supinfo;
> > > > > > > >
> > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > > > > > complication.
> > > > > >
> > > > > > I think we should keep it as a struct but defined the following way:
> > > > > >
> > > > > > struct {
> > > > > > spinlock_t lock;
> > > > > > struct rb_root tree;
> > > > > > } supinfo;
> > > > >
> > > > > That is what I meant be merging the struct definition with the variable
> > > > > definition. Or is there some other way to completely do away with the
> > > > > struct that I'm missing?
> > > >
> > > > Look at the top of gpiolib.c:
> > > >
> > > > static DEFINE_MUTEX(gpio_lookup_lock);
> > > > static LIST_HEAD(gpio_lookup_list);
> > > >
> > > > In the similar way you can simply do
> > > >
> > > > static DEFINE_SPINLOCK(gpio_sup_lock);
> > > > static struct rb_root gpio_sup_tree;
> > >
> > > The fact that this has been like this, doesn't mean it's the only
> > > right way. IMO putting these into the same structure makes logical
> > > sense.
> >
> > I disagree on the struct like this on the grounds:
> > - it's global
> > - it's one time use
> > - it adds complications for no benefit
> > - it makes code uglier and longer
> >
>
> It boils down to supinfo.lock vs supinfo_lock. I do prefer the former
> but it's a weak opinion, I won't die on that hill.
Me neither, just showing rationale from my side.
> > What we probably want to have is a singleton objects in C language or at least
> > inside Linux kernel project. But I'm not sure it's feasible.
> >
> > > > > > > Yeah, that is a hangover from an earlier iteration where supinfo was
> > > > > > > contained in other object rather than being a global.
> > > > > > > Could merge the struct definition into the variable now.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > > > > > > > +static struct supinfo supinfo;
> > > > > > > > >
> > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded
> > > > > > > > > complication.
> > > > > > >
> > > > > > > I think we should keep it as a struct but defined the following way:
> > > > > > >
> > > > > > > struct {
> > > > > > > spinlock_t lock;
> > > > > > > struct rb_root tree;
> > > > > > > } supinfo;
> > > > > >
> > > > > > That is what I meant be merging the struct definition with the variable
> > > > > > definition. Or is there some other way to completely do away with the
> > > > > > struct that I'm missing?
> > > > >
> > > > > Look at the top of gpiolib.c:
> > > > >
> > > > > static DEFINE_MUTEX(gpio_lookup_lock);
> > > > > static LIST_HEAD(gpio_lookup_list);
> > > > >
> > > > > In the similar way you can simply do
> > > > >
> > > > > static DEFINE_SPINLOCK(gpio_sup_lock);
> > > > > static struct rb_root gpio_sup_tree;
> > > >
> > > > The fact that this has been like this, doesn't mean it's the only
> > > > right way. IMO putting these into the same structure makes logical
> > > > sense.
> > >
> > > I disagree on the struct like this on the grounds:
> > > - it's global
I dislike having the global at all - and now you want two :-(.
> > > - it's one time use
Its not about how many times it is instanciated, it is about
maintainability.
> > > - it adds complications for no benefit
It provides a placeholder for collective documentation and clarifies
scope for the reader.
How is it more complicated?
> > > - it makes code uglier and longer
> > >
What, swapping an underscore for a period?
And you would hope the generated code is essentially the same.
> >
> > It boils down to supinfo.lock vs supinfo_lock. I do prefer the former
> > but it's a weak opinion, I won't die on that hill.
>
> Me neither, just showing rationale from my side.
>
I can't recall the last time I intentionally used separate globals over a
struct, so if there are no strong opinions otherwise I'll leave it as a
struct.
Cheers,
Kent.
On Thu, Dec 14, 2023 at 08:18:01AM +0800, Kent Gibson wrote:
> On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > > - it adds complications for no benefit
>
> It provides a placeholder for collective documentation and clarifies
> scope for the reader.
Turns out kernel-doc can't deal with a struct variable declaration - it
needs the struct to be named.
So this doesn't parse:
static struct {
struct rb_root tree;
spinlock_t lock;
} supinfo;
but this does:
static struct supinfo {
struct rb_root tree;
spinlock_t lock;
} supinfo;
at which point I prefer the separate struct and var declarations as per
the patch.
Opinions?
Cheers,
Kent.
On Thu, Dec 14, 2023 at 3:15 AM Kent Gibson <[email protected]> wrote:
>
> On Thu, Dec 14, 2023 at 08:18:01AM +0800, Kent Gibson wrote:
> > On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote:
> > >
> > > ...
> > >
> > > > > - it adds complications for no benefit
> >
> > It provides a placeholder for collective documentation and clarifies
> > scope for the reader.
>
> Turns out kernel-doc can't deal with a struct variable declaration - it
> needs the struct to be named.
>
> So this doesn't parse:
>
> static struct {
> struct rb_root tree;
> spinlock_t lock;
> } supinfo;
>
> but this does:
>
> static struct supinfo {
> struct rb_root tree;
> spinlock_t lock;
> } supinfo;
>
> at which point I prefer the separate struct and var declarations as per
> the patch.
>
> Opinions?
>
Yeah, don't make it a kernel doc. It's a private structure, no need to
expose documentation for it in docs. Just use a regular comment - say
what it is and why it's here.
Bart
> Cheers,
> Kent.
>
On Thu, Dec 14, 2023 at 10:40:26AM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 3:15 AM Kent Gibson <[email protected]> wrote:
> > On Thu, Dec 14, 2023 at 08:18:01AM +0800, Kent Gibson wrote:
> > > On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote:
...
> > > > > > - it adds complications for no benefit
> > >
> > > It provides a placeholder for collective documentation and clarifies
> > > scope for the reader.
> >
> > Turns out kernel-doc can't deal with a struct variable declaration - it
> > needs the struct to be named.
> >
> > So this doesn't parse:
> >
> > static struct {
> > struct rb_root tree;
> > spinlock_t lock;
> > } supinfo;
> >
> > but this does:
> >
> > static struct supinfo {
> > struct rb_root tree;
> > spinlock_t lock;
> > } supinfo;
> >
> > at which point I prefer the separate struct and var declarations as per
> > the patch.
> >
> > Opinions?
>
> Yeah, don't make it a kernel doc. It's a private structure, no need to
> expose documentation for it in docs. Just use a regular comment - say
> what it is and why it's here.
I agree with Bart, make it plain comment if needed.
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 14, 2023 at 04:35:09PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 10:40:26AM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > Opinions?
> >
> > Yeah, don't make it a kernel doc. It's a private structure, no need to
> > expose documentation for it in docs. Just use a regular comment - say
> > what it is and why it's here.
>
> I agree with Bart, make it plain comment if needed.
>
It is a plain comment in v2.
Cheers,
Kent.