diff mbox series

[v2] gpiolib: cdev: document that line eflags are shared

Message ID 20201014062921.79112-1-warthog618@gmail.com
State Accepted
Commit 3ffb7c45d193c771d7fc7722be0a45bc196f25f9
Headers show
Series [v2] gpiolib: cdev: document that line eflags are shared | expand

Commit Message

Kent Gibson Oct. 14, 2020, 6:29 a.m. UTC
The line.eflags field is shared so document this fact and highlight it
throughout using READ_ONCE() and WRITE_ONCE() accessors.

Also use a local copy of the eflags in edge_irq_thread() to ensure
consistent control flow even if eflags changes.  This is only a defensive
measure as edge_irq_thread() is currently disabled when the eflags are
changed.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

Changes for v2:
 - fixed typo in commit comment.
 
 drivers/gpio/gpiolib-cdev.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

Comments

Andy Shevchenko Oct. 16, 2020, 2:24 p.m. UTC | #1
On Wed, Oct 14, 2020 at 12:21 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> The line.eflags field is shared so document this fact and highlight it
> throughout using READ_ONCE() and WRITE_ONCE() accessors.
>
> Also use a local copy of the eflags in edge_irq_thread() to ensure
> consistent control flow even if eflags changes.  This is only a defensive
> measure as edge_irq_thread() is currently disabled when the eflags are
> changed.

> -       if (line->eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
> -                            GPIO_V2_LINE_FLAG_EDGE_FALLING)) {
> +       eflags = READ_ONCE(line->eflags);
> +       if (eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
> +                      GPIO_V2_LINE_FLAG_EDGE_FALLING)) {

Hmm... side note: perhaps at some point

#define GPIO_V2_LINE_FLAG_EDGE_BOTH  \
        (GPIO_V2_LINE_FLAG_EDGE_RISING | GPIO_V2_LINE_FLAG_EDGE_FALLING)

       if (eflags == GPIO_V2_LINE_FLAG_EDGE_BOTH) {

?
Kent Gibson Oct. 16, 2020, 11:39 p.m. UTC | #2
On Fri, Oct 16, 2020 at 05:24:14PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 14, 2020 at 12:21 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The line.eflags field is shared so document this fact and highlight it
> > throughout using READ_ONCE() and WRITE_ONCE() accessors.
> >
> > Also use a local copy of the eflags in edge_irq_thread() to ensure
> > consistent control flow even if eflags changes.  This is only a defensive
> > measure as edge_irq_thread() is currently disabled when the eflags are
> > changed.
> 
> > -       if (line->eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
> > -                            GPIO_V2_LINE_FLAG_EDGE_FALLING)) {
> > +       eflags = READ_ONCE(line->eflags);
> > +       if (eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
> > +                      GPIO_V2_LINE_FLAG_EDGE_FALLING)) {
> 
> Hmm... side note: perhaps at some point
> 
> #define GPIO_V2_LINE_FLAG_EDGE_BOTH  \
>         (GPIO_V2_LINE_FLAG_EDGE_RISING | GPIO_V2_LINE_FLAG_EDGE_FALLING)
> 
>        if (eflags == GPIO_V2_LINE_FLAG_EDGE_BOTH) {
> 
> ?

Yeah, that would make sense.  I think I used GPIO_V2_LINE_EDGE_FLAGS,
which is defined the same as your GPIO_V2_LINE_FLAG_EDGE_BOTH, here at
some point, but that just looked wrong.

The GPIO_V2_LINE_FLAG_EDGE_BOTH does read better.  I'll add it to the
todo list.

Cheers,
Kent.
Bartosz Golaszewski Oct. 26, 2020, 2:26 p.m. UTC | #3
On Wed, Oct 14, 2020 at 8:29 AM Kent Gibson <warthog618@gmail.com> wrote:
>

> The line.eflags field is shared so document this fact and highlight it

> throughout using READ_ONCE() and WRITE_ONCE() accessors.

>

> Also use a local copy of the eflags in edge_irq_thread() to ensure

> consistent control flow even if eflags changes.  This is only a defensive

> measure as edge_irq_thread() is currently disabled when the eflags are

> changed.

>

> Signed-off-by: Kent Gibson <warthog618@gmail.com>

> ---

>


Patch applied, thanks!

Bartosz
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 192721f829a3..678de9264617 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -428,6 +428,12 @@  struct line {
 	 */
 	struct linereq *req;
 	unsigned int irq;
+	/*
+	 * eflags is set by edge_detector_setup(), edge_detector_stop() and
+	 * edge_detector_update(), which are themselves mutually exclusive,
+	 * and is accessed by edge_irq_thread() and debounce_work_func(),
+	 * which can both live with a slightly stale value.
+	 */
 	u64 eflags;
 	/*
 	 * timestamp_ns and req_seqno are accessed only by
@@ -534,6 +540,7 @@  static irqreturn_t edge_irq_thread(int irq, void *p)
 	struct line *line = p;
 	struct linereq *lr = line->req;
 	struct gpio_v2_line_event le;
+	u64 eflags;
 
 	/* Do not leak kernel stack to userspace */
 	memset(&le, 0, sizeof(le));
@@ -552,8 +559,9 @@  static irqreturn_t edge_irq_thread(int irq, void *p)
 	}
 	line->timestamp_ns = 0;
 
-	if (line->eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
-			     GPIO_V2_LINE_FLAG_EDGE_FALLING)) {
+	eflags = READ_ONCE(line->eflags);
+	if (eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
+		       GPIO_V2_LINE_FLAG_EDGE_FALLING)) {
 		int level = gpiod_get_value_cansleep(line->desc);
 
 		if (level)
@@ -562,10 +570,10 @@  static irqreturn_t edge_irq_thread(int irq, void *p)
 		else
 			/* Emit high-to-low event */
 			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
-	} else if (line->eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) {
+	} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) {
 		/* Emit low-to-high event */
 		le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
-	} else if (line->eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) {
+	} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) {
 		/* Emit high-to-low event */
 		le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
 	} else {
@@ -634,6 +642,7 @@  static void debounce_work_func(struct work_struct *work)
 	struct line *line = container_of(work, struct line, work.work);
 	struct linereq *lr;
 	int level;
+	u64 eflags;
 
 	level = gpiod_get_raw_value_cansleep(line->desc);
 	if (level < 0) {
@@ -647,7 +656,8 @@  static void debounce_work_func(struct work_struct *work)
 	WRITE_ONCE(line->level, level);
 
 	/* -- edge detection -- */
-	if (!line->eflags)
+	eflags = READ_ONCE(line->eflags);
+	if (!eflags)
 		return;
 
 	/* switch from physical level to logical - if they differ */
@@ -655,8 +665,8 @@  static void debounce_work_func(struct work_struct *work)
 		level = !level;
 
 	/* ignore edges that are not being monitored */
-	if (((line->eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) && !level) ||
-	    ((line->eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) && level))
+	if (((eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) && !level) ||
+	    ((eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) && level))
 		return;
 
 	/* Do not leak kernel stack to userspace */
@@ -755,7 +765,7 @@  static void edge_detector_stop(struct line *line)
 
 	cancel_delayed_work_sync(&line->work);
 	WRITE_ONCE(line->sw_debounced, 0);
-	line->eflags = 0;
+	WRITE_ONCE(line->eflags, 0);
 	/* do not change line->level - see comment in debounced_value() */
 }
 
@@ -774,7 +784,7 @@  static int edge_detector_setup(struct line *line,
 		if (ret)
 			return ret;
 	}
-	line->eflags = eflags;
+	WRITE_ONCE(line->eflags, eflags);
 	if (gpio_v2_line_config_debounced(lc, line_idx)) {
 		debounce_period_us = gpio_v2_line_config_debounce_period(lc, line_idx);
 		ret = debounce_setup(line, debounce_period_us);
@@ -817,13 +827,13 @@  static int edge_detector_update(struct line *line,
 	unsigned int debounce_period_us =
 		gpio_v2_line_config_debounce_period(lc, line_idx);
 
-	if ((line->eflags == eflags) && !polarity_change &&
+	if ((READ_ONCE(line->eflags) == eflags) && !polarity_change &&
 	    (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
 		return 0;
 
 	/* sw debounced and still will be...*/
 	if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
-		line->eflags = eflags;
+		WRITE_ONCE(line->eflags, eflags);
 		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
 		return 0;
 	}