mbox series

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

Message ID 20231214095814.132400-1-warthog618@gmail.com
Headers show
Series gpiolib: cdev: relocate debounce_period_us | expand

Message

Kent Gibson Dec. 14, 2023, 9:58 a.m. UTC
This series contains minor improvements to gpiolib-cdev.

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

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

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

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

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

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

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

--
2.39.2

Comments

Kent Gibson Dec. 14, 2023, 11:50 a.m. UTC | #1
On Thu, Dec 14, 2023 at 05:58:10PM +0800, Kent Gibson wrote:
> Use scoped_guard for critical sections rather than distinct
> lock/unlock pairs.
>

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

Cheers,
Kent.
Andy Shevchenko Dec. 14, 2023, 2:53 p.m. UTC | #2
On Thu, Dec 14, 2023 at 05:58:10PM +0800, Kent Gibson wrote:
> Use scoped_guard for critical sections rather than distinct

scoped_guard()

> lock/unlock pairs.

...

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

In this case it can be guard()

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

	return linereq_...

...

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

Ditto.