drivers: gpio: fix mis-use of slist API in callback processing

The iterator over registered callbacks failed to account for the
possibility that the callback would remove itself from the list.  If
this occurred any remaining callbacks would no longer be reachable from
the node.  Switch to the slist iterator that is safe for self-removal.

Note that the slist API remains unsafe for removal of subsequent nodes.
Even with the corrected code removal of the next callback registration
(cached in tmp) will result in it being called anyway, with the
remaining unremoved registrations not being called.  If the next
callback were removed and re-registered on a different device, the
callbacks would be invoked for the wrong device.

Resolve this by a documentation change describing the conditions under
which a change to callback registration from within a callback are
permitted.  Add a similar note regarding the effect of adding a
callback.  The current event invocation behavior for callbacks added
within an event is explicitly left unspecified, though in the current
slist implementation newly added callbacks will not be invoked until the
next event.

Closes #10186

Signed-off-by: Peter A. Bigot <[email protected]>

Author: pabigot

drivers/gpio/gpio_utils.h

@@ -44,9 +44,9 @@ static inline void _gpio_fire_callbacks(sys_slist_t *list,
 					struct device *port,
 					u32_t pins)
 {
-	struct gpio_callback *cb;
+	struct gpio_callback *cb, *tmp;
 
-	SYS_SLIST_FOR_EACH_CONTAINER(list, cb, node) {
+	SYS_SLIST_FOR_EACH_CONTAINER_SAFE(list, cb, tmp, node) {
 		if (cb->pin_mask & pins) {
 			__ASSERT(cb->handler, "No callback handler!");
 			cb->handler(port, cb, pins);

include/gpio.h

@@ -252,6 +252,10 @@ static inline void gpio_init_callback(struct gpio_callback *callback,
  * @param callback A valid Application's callback structure pointer.
  * @return 0 if successful, negative errno code on failure.
  *
+ * @note Callbacks may be added to the device from within a callback
+ * handler invocation, but whether they are invoked for the current
+ * GPIO event is not specified.
+ *
  * Note: enables to add as many callback as needed on the same port.
  */
 static inline int gpio_add_callback(struct device *port,
@@ -271,6 +275,13 @@ static inline int gpio_add_callback(struct device *port,
  * @param callback A valid application's callback structure pointer.
  * @return 0 if successful, negative errno code on failure.
  *
+ * @warning It is explicitly permitted, within a callback handler, to
+ * remove the registration for the callback that is running, i.e. @p
+ * callback.  Attempts to remove other registrations on the same
+ * device may result in undefined behavior, including failure to
+ * invoke callbacks that remain registered and unintended invocation
+ * of removed callbacks.
+ *
  * Note: enables to remove as many callbacks as added through
  *       gpio_add_callback().
  */

tests/drivers/gpio/gpio_basic_api/src/main.c

@@ -53,6 +53,7 @@ void test_main(void)
 			 ztest_unit_test(test_gpio_callback_edge_low),
 			 ztest_unit_test(test_gpio_callback_level_high),
 			 ztest_unit_test(test_gpio_callback_add_remove),
+			 ztest_unit_test(test_gpio_callback_self_remove),
 			 ztest_unit_test(test_gpio_callback_enable_disable),
 			 ztest_unit_test(test_gpio_callback_level_low));
 	ztest_run_test_suite(gpio_basic_test);

tests/drivers/gpio/gpio_basic_api/src/test_callback_manage.c

@@ -30,7 +30,18 @@ static void callback_2(struct device *dev,
 	TC_PRINT("%s triggered: %d\n", __func__, ++cb_cnt[1]);
 }
 
-static void init_callback(struct device *dev)
+static void callback_remove_self(struct device *dev,
+		       struct gpio_callback *gpio_cb, u32_t pins)
+{
+	struct drv_data *dd = CONTAINER_OF(gpio_cb, struct drv_data, gpio_cb);
+
+	TC_PRINT("%s triggered: %d\n", __func__, ++cb_cnt[1]);
+	dd->aux = gpio_remove_callback(dev, gpio_cb);
+}
+
+static void init_callback(struct device *dev,
+			  gpio_callback_handler_t handler_1,
+			  gpio_callback_handler_t handler_2)
 {
 	gpio_pin_disable_callback(dev, PIN_IN);
 	gpio_pin_disable_callback(dev, PIN_OUT);
@@ -44,10 +55,10 @@ static void init_callback(struct device *dev)
 			   GPIO_DIR_IN | GPIO_INT | GPIO_INT_EDGE | \
 			   GPIO_INT_ACTIVE_HIGH | GPIO_INT_DEBOUNCE);
 
-	gpio_init_callback(&cb_data[0].gpio_cb, callback_1, BIT(PIN_IN));
+	gpio_init_callback(&cb_data[0].gpio_cb, handler_1, BIT(PIN_IN));
 	gpio_add_callback(dev, &cb_data[0].gpio_cb);
 
-	gpio_init_callback(&cb_data[1].gpio_cb, callback_2, BIT(PIN_IN));
+	gpio_init_callback(&cb_data[1].gpio_cb, handler_2, BIT(PIN_IN));
 	gpio_add_callback(dev, &cb_data[1].gpio_cb);
 }
 
@@ -73,7 +84,7 @@ static int test_callback_add_remove(void)
 	struct device *dev = device_get_binding(DEV_NAME);
 
 	/* SetUp: initialize environment */
-	init_callback(dev);
+	init_callback(dev, callback_1, callback_2);
 
 	/* 3. enable callback, trigger PIN_IN interrupt by operate PIN_OUT */
 	trigger_callback(dev, 1);
@@ -109,12 +120,58 @@ static int test_callback_add_remove(void)
 	return TC_FAIL;
 }
 
+static int test_callback_self_remove(void)
+{
+	int res = TC_FAIL;
+	struct device *dev = device_get_binding(DEV_NAME);
+
+	/* SetUp: initialize environment */
+	init_callback(dev, callback_1, callback_remove_self);
+
+	gpio_pin_write(dev, PIN_OUT, 0);
+	k_sleep(100);
+
+	cb_data[0].aux = INT_MAX;
+	cb_data[1].aux = INT_MAX;
+
+	/* 3. enable callback, trigger PIN_IN interrupt by operate PIN_OUT */
+	trigger_callback(dev, 1);
+
+	/*= checkpoint: check both callbacks are triggered =*/
+	if (cb_cnt[0] != 1 || cb_cnt[1] != 1) {
+		TC_ERROR("not trigger callback correctly\n");
+		goto err_exit;
+	}
+
+	/*= checkpoint: check remove executed is invoked =*/
+	if (cb_data[0].aux != INT_MAX || cb_data[1].aux != 0) {
+		TC_ERROR("not remove callback correctly\n");
+		goto err_exit;
+	}
+
+	/* 4 enable callback, trigger PIN_IN interrupt by operate PIN_OUT */
+	trigger_callback(dev, 1);
+
+	/*= checkpoint: check only remaining callback is triggered =*/
+	if (cb_cnt[0] != 1 || cb_cnt[1] != 0) {
+		TC_ERROR("not trigger remaining callback correctly\n");
+		goto err_exit;
+	}
+
+	res = TC_PASS;
+
+err_exit:
+	gpio_remove_callback(dev, &cb_data[0].gpio_cb);
+	gpio_remove_callback(dev, &cb_data[1].gpio_cb);
+	return res;
+}
+
 static int test_callback_enable_disable(void)
 {
 	struct device *dev = device_get_binding(DEV_NAME);
 
 	/* SetUp: initialize environment */
-	init_callback(dev);
+	init_callback(dev, callback_1, callback_2);
 
 	/* 3. enable callback, trigger PIN_IN interrupt by operate PIN_OUT */
 	trigger_callback(dev, 1);
@@ -155,6 +212,12 @@ void test_gpio_callback_add_remove(void)
 		test_callback_add_remove() == TC_PASS, NULL);
 }
 
+void test_gpio_callback_self_remove(void)
+{
+	zassert_true(
+		test_callback_self_remove() == TC_PASS, NULL);
+}
+
 void test_gpio_callback_enable_disable(void)
 {
 	zassert_true(

tests/drivers/gpio/gpio_basic_api/src/test_gpio.h

@@ -61,6 +61,7 @@ struct drv_data {
 	struct gpio_callback gpio_cb;
 	int mode;
 	int index;
+	int aux;
 };
 
 void test_gpio_pin_read_write(void);
@@ -69,6 +70,7 @@ void test_gpio_callback_edge_low(void);
 void test_gpio_callback_level_high(void);
 void test_gpio_callback_level_low(void);
 void test_gpio_callback_add_remove(void);
+void test_gpio_callback_self_remove(void);
 void test_gpio_callback_enable_disable(void);
 
 #endif /* __TEST_GPIO_H__ */