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 <[email protected]>
---
Changes for v2:
- fixed typo in commit comment.
drivers/gpio/gpiolib-cdev.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
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;
}
--
2.28.0
On Wed, Oct 14, 2020 at 12:21 PM Kent Gibson <[email protected]> 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) {
?
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 16, 2020 at 05:24:14PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 14, 2020 at 12:21 PM Kent Gibson <[email protected]> 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.
On Wed, Oct 14, 2020 at 8:29 AM Kent Gibson <[email protected]> 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 <[email protected]>
> ---
>
Patch applied, thanks!
Bartosz