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 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 | 393 ++++++++++++++++++++++--------------
drivers/gpio/gpiolib.c | 3 -
drivers/gpio/gpiolib.h | 5 -
3 files changed, 246 insertions(+), 155 deletions(-)
--
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 b956664f8649..37794de691f3 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1383,9 +1383,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;
}
}
@@ -1394,6 +1403,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;
@@ -1414,6 +1424,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]);
@@ -1439,14 +1450,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;
}
}
@@ -1454,7 +1476,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 Fri, Dec 15, 2023 at 10:38:00AM +0800, Kent Gibson 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.
This version LGTM, few minor comments here and there, but feel free to add
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko