gpiolib: cdev: document that line eflags are shared
Kent Gibson
reviewerPATCH v2 1/1 gpiolib: cdev: document that line eflags are shared
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.
---
Changes for v2:
- fixed typo in commit comment.
drivers/gpio/gpiolib-cdev.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
drivers/gpio/gpiolib-cdev.c +21 -11
@@ -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 ; }
Signature · 1 line
2.28.0
Quoted 9 lines · Kent Gibson
The line.eflags field is shared so document this fact and highlight it throughout using
READ_ONCE()andWRITE_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 asedge_irq_thread()is currently disabled when the eflags are changed.
drivers/gpio/gpiolib-cdev.c static irqreturn_t edge_irq_thread(int irq, void *p) · line 564
Quoted 5 lines
drivers/gpio/gpiolib-cdev.c +3
-2-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) {
?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Kent Gibson
reviewerQuoted 16 lines · Andy Shevchenko
Quoted 9 lines · Kent Gibson
The line.eflags field is shared so document this fact and highlight it throughout using
READ_ONCE()andWRITE_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 asedge_irq_thread()is currently disabled when the eflags are changed.drivers/gpio/gpiolib-cdev.c static irqreturn_t edge_irq_thread(int irq, void *p) · line 564
Quoted 5 lines
drivers/gpio/gpiolib-cdev.c +3
-2-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 )) {
Quoted 8 lines
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.
Quoted 4 lines · Kent Gibson
The line.eflags field is shared so document this fact and highlight it throughout using
READ_ONCE()andWRITE_ONCE()accessors.
Quoted 8 lines
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 asedge_irq_thread()is currently disabled when the eflags are changed.Signed-off-by Kent Gibson <[email protected]>---
Patch applied, thanks!
Bartosz