[PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Marek Vasut]

Make cyclic_register() return error code, 0 in case of success,
-EALREADY in case the called attempts to re-register already
registered struct cyclic_info. The re-registration would lead
to corruption of gd->cyclic_list because the re-registration
would memset() one of its nodes, prevent that. Unregister only
initialized struct cyclic_info.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 common/cyclic.c  | 14 +++++++++++---
 include/cyclic.h |  9 ++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/common/cyclic.c b/common/cyclic.c
index 196797fd61e..53156a704cc 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -27,9 +27,13 @@ struct hlist_head *cyclic_get_list(void)
 	return (struct hlist_head *)&gd->cyclic_list;
 }
 
-void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
-		     uint64_t delay_us, const char *name)
+int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
+		    uint64_t delay_us, const char *name)
 {
+	/* Reassignment of function would corrupt cyclic list, exit */
+	if (cyclic->func)
+		return -EALREADY;
+
 	memset(cyclic, 0, sizeof(*cyclic));
 
 	/* Store values in struct */
@@ -38,11 +42,15 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
 	cyclic->delay_us = delay_us;
 	cyclic->start_time_us = timer_get_us();
 	hlist_add_head(&cyclic->list, cyclic_get_list());
+
+	return 0;
 }
 
 void cyclic_unregister(struct cyclic_info *cyclic)
 {
-	hlist_del(&cyclic->list);
+	/* Unregister only initialized struct cyclic_info */
+	if (cyclic->func)
+		hlist_del(&cyclic->list);
 }
 
 static void cyclic_run(void)
diff --git a/include/cyclic.h b/include/cyclic.h
index c6c463d68e9..c86ac407332 100644
--- a/include/cyclic.h
+++ b/include/cyclic.h
@@ -60,8 +60,10 @@ typedef void (*cyclic_func_t)(struct cyclic_info *c);
  * The function @func will be called with @cyclic as its
  * argument. @cyclic will usually be embedded in some device-specific
  * structure, which the callback can retrieve using container_of().
+ *
+ * @return 0 on success, -EALREADY on repeated registration, -ve otherwise
  */
-void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
+int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
 		     uint64_t delay_us, const char *name);
 
 /**
@@ -89,9 +91,10 @@ struct hlist_head *cyclic_get_list(void);
 
 #else
 
-static inline void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
-				   uint64_t delay_us, const char *name)
+static inline int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
+				  uint64_t delay_us, const char *name)
 {
+	return 0;
 }
 
 static inline void cyclic_unregister(struct cyclic_info *cyclic)
-- 
2.45.2

[PATCH 2/8] cyclic: Handle return code from cyclic_register() in demo and docs

[Marek Vasut]

Update the cyclic demo code and documentation to indicate there
is now a return code from cyclic_register().

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 cmd/cyclic.c           | 10 ++++++----
 doc/develop/cyclic.rst |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/cmd/cyclic.c b/cmd/cyclic.c
index 339dd4a7bce..e98c4a488d6 100644
--- a/cmd/cyclic.c
+++ b/cmd/cyclic.c
@@ -33,8 +33,10 @@ static void cyclic_demo(struct cyclic_info *c)
 static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc,
 			  char *const argv[])
 {
+	const char *demo_name = "cyclic_demo";
 	struct cyclic_demo_info *info;
 	uint time_ms;
+	int ret;
 
 	if (argc < 3)
 		return CMD_RET_USAGE;
@@ -49,12 +51,12 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc,
 	info->delay_us = simple_strtoul(argv[2], NULL, 0);
 
 	/* Register demo cyclic function */
-	cyclic_register(&info->cyclic, cyclic_demo, time_ms * 1000, "cyclic_demo");
+	ret = cyclic_register(&info->cyclic, cyclic_demo, time_ms * 1000, demo_name);
 
-	printf("Registered function \"%s\" to be executed all %dms\n",
-	       "cyclic_demo", time_ms);
+	printf("Registered function \"%s\" to be executed all %dms with return code %d\n",
+	       demo_name, time_ms, ret);
 
-	return 0;
+	return ret;
 }
 
 static int do_cyclic_list(struct cmd_tbl *cmdtp, int flag, int argc,
diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst
index 6f1da6f0d9b..604c0463f8d 100644
--- a/doc/develop/cyclic.rst
+++ b/doc/develop/cyclic.rst
@@ -38,9 +38,9 @@ To register a cyclic function, use something like this::
         /* Initialize donkey ... */
 
         /* Register demo cyclic function */
-        cyclic_register(&donkey->cyclic, cyclic_demo, 10 * 1000, "cyclic_demo");
+        ret = cyclic_register(&donkey->cyclic, cyclic_demo, 10 * 1000, "cyclic_demo");
         
-        return 0;
+        return ret;
     }
 
 This will register the function `cyclic_demo()` to be periodically
-- 
2.45.2

[PATCH 3/8] cyclic: Test return code from cyclic_register() in test

[Marek Vasut]

Update the cyclic test code to verify return code from cyclic_register() works.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 test/common/cyclic.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/test/common/cyclic.c b/test/common/cyclic.c
index 51a07c576b6..714dec60b7f 100644
--- a/test/common/cyclic.c
+++ b/test/common/cyclic.c
@@ -25,8 +25,11 @@ static void test_cb(struct cyclic_info *c)
 
 static int dm_test_cyclic_running(struct unit_test_state *uts)
 {
+	int ret;
+
 	cyclic_test.called = false;
-	cyclic_register(&cyclic_test.cyclic, test_cb, 10 * 1000, "cyclic_test");
+	ret = cyclic_register(&cyclic_test.cyclic, test_cb, 10 * 1000, "cyclic_test");
+	ut_asserteq(ret, 0);
 
 	/* Execute all registered cyclic functions */
 	schedule();
@@ -37,3 +40,19 @@ static int dm_test_cyclic_running(struct unit_test_state *uts)
 	return 0;
 }
 COMMON_TEST(dm_test_cyclic_running, 0);
+
+static int dm_test_cyclic_reregister(struct unit_test_state *uts)
+{
+	int ret;
+
+	cyclic_test.called = false;
+	ret = cyclic_register(&cyclic_test.cyclic, test_cb, 10 * 1000, "cyclic_test1");
+	ut_asserteq(ret, 0);
+	ret = cyclic_register(&cyclic_test.cyclic, test_cb, 10 * 1000, "cyclic_test2");
+	ut_asserteq(ret, -EALREADY);
+
+	cyclic_unregister(&cyclic_test.cyclic);
+
+	return 0;
+}
+COMMON_TEST(dm_test_cyclic_reregister, 0);
-- 
2.45.2

[PATCH 4/8] mips: octeon: nic23: Handle return value from cyclic_register()

[Marek Vasut]

Handle the error code returned by cyclic_register(). Report the error,
but do not return it from board_late_init() as that would make the
board not boot, which is undesired. Let the board boot somehow, so
the user can fix the problem, even if the cyclic callback is not
usable.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 board/Marvell/octeon_nic23/board.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/board/Marvell/octeon_nic23/board.c b/board/Marvell/octeon_nic23/board.c
index cf20c97684a..5d1bf15ed50 100644
--- a/board/Marvell/octeon_nic23/board.c
+++ b/board/Marvell/octeon_nic23/board.c
@@ -341,6 +341,7 @@ int board_late_init(void)
 	struct cyclic_info *cyclic;
 	struct gpio_desc gpio = {};
 	ofnode node;
+	int ret;
 
 	/* Turn on SFP+ transmitters */
 	ofnode_for_each_compatible_node(node, "ethernet,sfp-slot") {
@@ -358,13 +359,18 @@ int board_late_init(void)
 
 	/* Register cyclic function for PCIe FLR fixup */
 	cyclic = calloc(1, sizeof(*cyclic));
-	if (cyclic) {
-		cyclic_register(cyclic, octeon_board_restore_pf, 100,
-				"pcie_flr_fix");
-	} else {
-		printf("Registering of cyclic function failed\n");
+	if (!cyclic) {
+		printf("Registering of cyclic function failed, ENOMEM\n");
+		/* Do not exit with error code, let the board boot somehow. */
+		return 0;
 	}
 
+	ret = cyclic_register(cyclic, octeon_board_restore_pf, 100,
+			      "pcie_flr_fix");
+	/* Do not exit with error code, let the board boot somehow. */
+	if (ret)
+		printf("Registering of cyclic function failed, %d\n", ret);
+
 	return 0;
 }
 
-- 
2.45.2

[PATCH 5/8] led: Handle return value from cyclic_register()

[Marek Vasut]

Handle the error code returned by cyclic_register() and propagate it.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/led/led_sw_blink.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c
index ee1546d02d4..859dac47c30 100644
--- a/drivers/led/led_sw_blink.c
+++ b/drivers/led/led_sw_blink.c
@@ -50,6 +50,7 @@ int led_sw_set_period(struct udevice *dev, int period_ms)
 	struct led_sw_blink *sw_blink = uc_plat->sw_blink;
 	struct led_ops *ops = led_get_ops(dev);
 	int half_period_us;
+	int ret = 0;
 
 	half_period_us = period_ms * 1000 / 2;
 
@@ -71,8 +72,8 @@ int led_sw_set_period(struct udevice *dev, int period_ms)
 	}
 
 	if (sw_blink->state == LED_SW_BLINK_ST_DISABLED) {
-		cyclic_register(&sw_blink->cyclic, led_sw_blink,
-				half_period_us, sw_blink->cyclic_name);
+		ret = cyclic_register(&sw_blink->cyclic, led_sw_blink,
+				      half_period_us, sw_blink->cyclic_name);
 	} else {
 		sw_blink->cyclic.delay_us = half_period_us;
 		sw_blink->cyclic.start_time_us = timer_get_us();
@@ -81,7 +82,7 @@ int led_sw_set_period(struct udevice *dev, int period_ms)
 	sw_blink->state = LED_SW_BLINK_ST_NOT_READY;
 	ops->set_state(dev, LEDST_OFF);
 
-	return 0;
+	return ret;
 }
 
 bool led_sw_is_blinking(struct udevice *dev)
-- 
2.45.2

[PATCH 6/8] mmc: Handle return value from cyclic_register()

[Marek Vasut]

Handle the error code returned by cyclic_register() and propagate it,
except in case the return code is EALREADY. The cyclic_register() in
mmc.c can be called multiple times with the same parameters, and that
is not an error, so depend on the cyclic_register() to skip such a
repeated registration and consider EALREADY as non-error here.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/mmc/mmc.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 4ea97974383..9c58d052261 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -3078,11 +3078,17 @@ int mmc_init(struct mmc *mmc)
 		return err;
 	}
 
-	if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) {
-		/* Register cyclic function for card detect polling */
-		cyclic_register(&mmc->cyclic, mmc_cyclic_cd_poll, 100 * 1000,
-				mmc->cfg->name);
-	}
+	/* Register cyclic function for card detect polling */
+	err = cyclic_register(&mmc->cyclic, mmc_cyclic_cd_poll, 100 * 1000,
+			      mmc->cfg->name);
+	/*
+	 * This cyclic_register() here might be called multiple times, this
+	 * is not a problem in this specific case, because the mmc subsystem
+	 * always registers the same function with the same polling delay,
+	 * so the EALREADY error can be ignored here.
+	 */
+	if (err && err == -EALREADY)
+		err = 0;
 
 	return err;
 }
@@ -3091,8 +3097,7 @@ int mmc_deinit(struct mmc *mmc)
 {
 	u32 caps_filtered;
 
-	if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL)))
-		cyclic_unregister(&mmc->cyclic);
+	cyclic_unregister(&mmc->cyclic);
 
 	if (!CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) &&
 	    !CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) &&
-- 
2.45.2

[PATCH 7/8] video: Handle return value from cyclic_register()

[Marek Vasut]

Handle the error code returned by cyclic_register() and propagate it.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/video/video-uclass.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index a5b3e898066..94b2c3c3b91 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -653,12 +653,12 @@ static int video_post_probe(struct udevice *dev)
 	    !uc_priv->cyc_active) {
 		uint ms = CONFIG_IF_ENABLED_INT(CYCLIC, VIDEO_SYNC_CYCLIC_MS);
 
-		cyclic_register(&uc_priv->cyc, video_idle, ms * 1000,
-				"video_init");
+		ret = cyclic_register(&uc_priv->cyc, video_idle, ms * 1000,
+				      "video_init");
 		uc_priv->cyc_active = true;
 	}
 
-	return 0;
+	return ret;
 };
 
 /* Post-relocation, allocate memory for the frame buffer */
-- 
2.45.2

[PATCH 8/8] wdt: Handle return value from cyclic_register()

[Marek Vasut]

Handle the error code returned by cyclic_register() and propagate it.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Angelo Dureghello <angelo@kernel-space.org>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Devarsh Thakkar <devarsht@ti.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Michael Polyntsov <michael.polyntsov@iopsys.eu>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Cc: Nikhil M Jain <n-jain1@ti.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Ronald Wahl <ronald.wahl@legrand.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de
---
 drivers/watchdog/wdt-uclass.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 10be334e9ed..5778c8bee40 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -129,9 +129,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
 				cyclic_unregister(&priv->cyclic);
 
 			/* Register the watchdog driver as a cyclic function */
-			cyclic_register(&priv->cyclic, wdt_cyclic,
-					priv->reset_period * 1000,
-					dev->name);
+			ret = cyclic_register(&priv->cyclic, wdt_cyclic,
+					      priv->reset_period * 1000,
+					      dev->name);
+			if (ret)
+				return ret;
 
 			snprintf(str, 16, "every %ldms", priv->reset_period);
 		}
-- 
2.45.2

Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Rasmus Villemoes]

On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> Make cyclic_register() return error code, 0 in case of success,
> -EALREADY in case the called attempts to re-register already
> registered struct cyclic_info. The re-registration would lead
> to corruption of gd->cyclic_list because the re-registration
> would memset() one of its nodes, prevent that. Unregister only
> initialized struct cyclic_info.

I had considered something like this, but I don't like it, because it
relies on the cyclic structure (or more likely whatever structure it is
embedded in) being initially zero-initialized. And if the caller doesn't
know whether the cyclic_info is already registered or not, he can't do a
memset() of it.

So my preference would be that we instead simply iterate the current
list to see if the struct cyclic_info is already registered that
way. Also, I think I'd prefer if double cyclic_register() is allowed and
always succeeds; this could be used to change the period of an already
registered instance, for example. Also, that avoids making the
interfaces fallible.

And cyclic_unregister() could similarly just check
whether the passed pointer is already on the list, and be a no-op in
case it's not. Those extra list traversals are not expensive (we're
traversing them thousands of times per second anyway in cyclic_run), and
I doubt one would ever has more than about 10 items on the list.

IOW, I'd suggest adding an internal

bool cyclic_is_registered(struct cyclic_info *info)
{
  struct cyclic_info *c;
  hlist_for_each(...) if (c == info) return true;
  return false;
}

add

  if (!cyclic_is_registered(c))
    return;

to cyclic_unregister(), and have cyclic_register() unconditionally start
by a

  cyclic_unregister(c);

and then proceed to initialize it as it does currently. No other
changes, apart from documentation, would be needed.
    


>  common/cyclic.c  | 14 +++++++++++---
>  include/cyclic.h |  9 ++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/common/cyclic.c b/common/cyclic.c
> index 196797fd61e..53156a704cc 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -27,9 +27,13 @@ struct hlist_head *cyclic_get_list(void)
>  	return (struct hlist_head *)&gd->cyclic_list;
>  }
>  
> -void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> -		     uint64_t delay_us, const char *name)
> +int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> +		    uint64_t delay_us, const char *name)
>  {
> +	/* Reassignment of function would corrupt cyclic list, exit */
> +	if (cyclic->func)
> +		return -EALREADY;
> +
>  	memset(cyclic, 0, sizeof(*cyclic));
>  
>  	/* Store values in struct */
> @@ -38,11 +42,15 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>  	cyclic->delay_us = delay_us;
>  	cyclic->start_time_us = timer_get_us();
>  	hlist_add_head(&cyclic->list, cyclic_get_list());
> +
> +	return 0;
>  }
>  
>  void cyclic_unregister(struct cyclic_info *cyclic)
>  {
> -	hlist_del(&cyclic->list);
> +	/* Unregister only initialized struct cyclic_info */
> +	if (cyclic->func)
> +		hlist_del(&cyclic->list);
>  }

So this already shows how error prone this approach is. You are not
clearing cyclic->func, so if the caller subsequently tries to register
that struct again, he would get -EALREADY, while a subsequent unregister
could would lead to exactly the list corruption you want to avoid.

And unless the caller immediately himself clears ->func, other code in
the client cannot rely on ->func being NULL or not as a proxy for
whether the struct is already registered (and the caller shouldn't do
either of those things, as the struct cyclic_info should be considered
opaque).

Rasmus

Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Stefan Roese]

Hi Marek,
Hi Rasmus,

On 20.01.25 10:17, Rasmus Villemoes wrote:
> On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:
> 
>> Make cyclic_register() return error code, 0 in case of success,
>> -EALREADY in case the called attempts to re-register already
>> registered struct cyclic_info. The re-registration would lead
>> to corruption of gd->cyclic_list because the re-registration
>> would memset() one of its nodes, prevent that. Unregister only
>> initialized struct cyclic_info.
> 
> I had considered something like this, but I don't like it, because it
> relies on the cyclic structure (or more likely whatever structure it is
> embedded in) being initially zero-initialized. And if the caller doesn't
> know whether the cyclic_info is already registered or not, he can't do a
> memset() of it.
> 
> So my preference would be that we instead simply iterate the current
> list to see if the struct cyclic_info is already registered that
> way. Also, I think I'd prefer if double cyclic_register() is allowed and
> always succeeds; this could be used to change the period of an already
> registered instance, for example. Also, that avoids making the
> interfaces fallible.
> 
> And cyclic_unregister() could similarly just check
> whether the passed pointer is already on the list, and be a no-op in
> case it's not. Those extra list traversals are not expensive (we're
> traversing them thousands of times per second anyway in cyclic_run), and
> I doubt one would ever has more than about 10 items on the list.
> 
> IOW, I'd suggest adding an internal
> 
> bool cyclic_is_registered(struct cyclic_info *info)
> {
>    struct cyclic_info *c;
>    hlist_for_each(...) if (c == info) return true;
>    return false;
> }
> 
> add
> 
>    if (!cyclic_is_registered(c))
>      return;
> 
> to cyclic_unregister(), and have cyclic_register() unconditionally start
> by a
> 
>    cyclic_unregister(c);
> 
> and then proceed to initialize it as it does currently. No other
> changes, apart from documentation, would be needed.
>      
> 
> 
>>   common/cyclic.c  | 14 +++++++++++---
>>   include/cyclic.h |  9 ++++++---
>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/cyclic.c b/common/cyclic.c
>> index 196797fd61e..53156a704cc 100644
>> --- a/common/cyclic.c
>> +++ b/common/cyclic.c
>> @@ -27,9 +27,13 @@ struct hlist_head *cyclic_get_list(void)
>>   	return (struct hlist_head *)&gd->cyclic_list;
>>   }
>>   
>> -void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>> -		     uint64_t delay_us, const char *name)
>> +int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>> +		    uint64_t delay_us, const char *name)
>>   {
>> +	/* Reassignment of function would corrupt cyclic list, exit */
>> +	if (cyclic->func)
>> +		return -EALREADY;
>> +
>>   	memset(cyclic, 0, sizeof(*cyclic));
>>   
>>   	/* Store values in struct */
>> @@ -38,11 +42,15 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>>   	cyclic->delay_us = delay_us;
>>   	cyclic->start_time_us = timer_get_us();
>>   	hlist_add_head(&cyclic->list, cyclic_get_list());
>> +
>> +	return 0;
>>   }
>>   
>>   void cyclic_unregister(struct cyclic_info *cyclic)
>>   {
>> -	hlist_del(&cyclic->list);
>> +	/* Unregister only initialized struct cyclic_info */
>> +	if (cyclic->func)
>> +		hlist_del(&cyclic->list);
>>   }
> 
> So this already shows how error prone this approach is. You are not
> clearing cyclic->func, so if the caller subsequently tries to register
> that struct again, he would get -EALREADY, while a subsequent unregister
> could would lead to exactly the list corruption you want to avoid.
> 
> And unless the caller immediately himself clears ->func, other code in
> the client cannot rely on ->func being NULL or not as a proxy for
> whether the struct is already registered (and the caller shouldn't do
> either of those things, as the struct cyclic_info should be considered
> opaque).

I like the approach suggest by Rasmus. Marek, do you see any flaws using
this version? If not, please send an updated version.

Thanks,
Stefan

Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Marek Vasut]

On 1/20/25 10:17 AM, Rasmus Villemoes wrote:
> On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:
> 
>> Make cyclic_register() return error code, 0 in case of success,
>> -EALREADY in case the called attempts to re-register already
>> registered struct cyclic_info. The re-registration would lead
>> to corruption of gd->cyclic_list because the re-registration
>> would memset() one of its nodes, prevent that. Unregister only
>> initialized struct cyclic_info.
> 
> I had considered something like this, but I don't like it, because it
> relies on the cyclic structure (or more likely whatever structure it is
> embedded in) being initially zero-initialized.

True

> And if the caller doesn't
> know whether the cyclic_info is already registered or not, he can't do a
> memset() of it.

This is what can happen right now, which is dangerous and what this 
series attempts to address.

> So my preference would be that we instead simply iterate the current
> list to see if the struct cyclic_info is already registered that
> way.

That I can do, but it will be a bit slower.

> Also, I think I'd prefer if double cyclic_register() is allowed and
> always succeeds; this could be used to change the period of an already
> registered instance, for example.

This would be terribly overloaded interface, no, let's not do that.

Better introduce a dedicated function for that kind of period adjustment.

> Also, that avoids making the
> interfaces fallible.
> 
> And cyclic_unregister() could similarly just check
> whether the passed pointer is already on the list, and be a no-op in
> case it's not. Those extra list traversals are not expensive (we're
> traversing them thousands of times per second anyway in cyclic_run), and
> I doubt one would ever has more than about 10 items on the list.
> 
> IOW, I'd suggest adding an internal
> 
> bool cyclic_is_registered(struct cyclic_info *info)
> {
>    struct cyclic_info *c;
>    hlist_for_each(...) if (c == info) return true;

I don't think this works, because that struct cyclic_info contains 
.next_call member, which is updated over time, so this exact match would 
not work as-is. I have something like this now:

diff --git a/common/cyclic.c b/common/cyclic.c
index 807a3d73f67..d721a21a575 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -27,11 +27,29 @@ struct hlist_head *cyclic_get_list(void)
  	return (struct hlist_head *)&gd->cyclic_list;
  }

+static int cyclic_already_registered(struct cyclic_info *cyclic)
+{
+	struct cyclic_info *cycliclst;
+	struct hlist_node *tmp;
+
+	/* Reassignment of function would corrupt cyclic list, exit */
+	hlist_for_each_entry_safe(cycliclst, tmp, cyclic_get_list(), list) {
+		if (cycliclst->func == cyclic->func &&
+		    cycliclst->name == cyclic->name && // or strcmp() ?
+		    cycliclst->delay_us == cyclic->delay_us &&
+		    cycliclst->start_time_us == cyclic->start_time_us)
+			return -EALREADY;	/* Match found */
+	}
+
+	/* Match not found */
+	return 0;
+}
+
  int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
  		    uint64_t delay_us, const char *name)
  {
  	/* Reassignment of function would corrupt cyclic list, exit */
-	if (cyclic->func)
+	if (!cyclic_already_registered(cyclic))
  		return -EALREADY;

  	memset(cyclic, 0, sizeof(*cyclic));
@@ -49,7 +67,7 @@ int cyclic_register(struct cyclic_info *cyclic, 
cyclic_func_t func,
  void cyclic_unregister(struct cyclic_info *cyclic)
  {
  	/* Unregister only initialized struct cyclic_info */
-	if (cyclic->func)
+	if (cyclic_already_registered(cyclic))
  		hlist_del(&cyclic->list);
  }

[...]

>>   void cyclic_unregister(struct cyclic_info *cyclic)
>>   {
>> -	hlist_del(&cyclic->list);
>> +	/* Unregister only initialized struct cyclic_info */
>> +	if (cyclic->func)
>> +		hlist_del(&cyclic->list);
>>   }
> 
> So this already shows how error prone this approach is. You are not
> clearing cyclic->func, so if the caller subsequently tries to register
> that struct again, he would get -EALREADY, while a subsequent unregister
> could would lead to exactly the list corruption you want to avoid.

I would expect the caller should clear the structure before attempting 
to register it again. Shall we actually memset() the structure in 
cyclic_unregister() too ?

> And unless the caller immediately himself clears ->func, other code in
> the client cannot rely on ->func being NULL or not as a proxy for
> whether the struct is already registered (and the caller shouldn't do
> either of those things, as the struct cyclic_info should be considered
> opaque).

Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Marek Vasut]

On 1/22/25 10:25 AM, Stefan Roese wrote:
> Hi Marek,
> Hi Rasmus,
> 
> On 20.01.25 10:17, Rasmus Villemoes wrote:
>> On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:
>>
>>> Make cyclic_register() return error code, 0 in case of success,
>>> -EALREADY in case the called attempts to re-register already
>>> registered struct cyclic_info. The re-registration would lead
>>> to corruption of gd->cyclic_list because the re-registration
>>> would memset() one of its nodes, prevent that. Unregister only
>>> initialized struct cyclic_info.
>>
>> I had considered something like this, but I don't like it, because it
>> relies on the cyclic structure (or more likely whatever structure it is
>> embedded in) being initially zero-initialized. And if the caller doesn't
>> know whether the cyclic_info is already registered or not, he can't do a
>> memset() of it.
>>
>> So my preference would be that we instead simply iterate the current
>> list to see if the struct cyclic_info is already registered that
>> way. Also, I think I'd prefer if double cyclic_register() is allowed and
>> always succeeds; this could be used to change the period of an already
>> registered instance, for example. Also, that avoids making the
>> interfaces fallible.
>>
>> And cyclic_unregister() could similarly just check
>> whether the passed pointer is already on the list, and be a no-op in
>> case it's not. Those extra list traversals are not expensive (we're
>> traversing them thousands of times per second anyway in cyclic_run), and
>> I doubt one would ever has more than about 10 items on the list.
>>
>> IOW, I'd suggest adding an internal
>>
>> bool cyclic_is_registered(struct cyclic_info *info)
>> {
>>    struct cyclic_info *c;
>>    hlist_for_each(...) if (c == info) return true;
>>    return false;
>> }
>>
>> add
>>
>>    if (!cyclic_is_registered(c))
>>      return;
>>
>> to cyclic_unregister(), and have cyclic_register() unconditionally start
>> by a
>>
>>    cyclic_unregister(c);
>>
>> and then proceed to initialize it as it does currently. No other
>> changes, apart from documentation, would be needed.
>>
>>
>>>   common/cyclic.c  | 14 +++++++++++---
>>>   include/cyclic.h |  9 ++++++---
>>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/common/cyclic.c b/common/cyclic.c
>>> index 196797fd61e..53156a704cc 100644
>>> --- a/common/cyclic.c
>>> +++ b/common/cyclic.c
>>> @@ -27,9 +27,13 @@ struct hlist_head *cyclic_get_list(void)
>>>       return (struct hlist_head *)&gd->cyclic_list;
>>>   }
>>> -void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>>> -             uint64_t delay_us, const char *name)
>>> +int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>>> +            uint64_t delay_us, const char *name)
>>>   {
>>> +    /* Reassignment of function would corrupt cyclic list, exit */
>>> +    if (cyclic->func)
>>> +        return -EALREADY;
>>> +
>>>       memset(cyclic, 0, sizeof(*cyclic));
>>>       /* Store values in struct */
>>> @@ -38,11 +42,15 @@ void cyclic_register(struct cyclic_info *cyclic, 
>>> cyclic_func_t func,
>>>       cyclic->delay_us = delay_us;
>>>       cyclic->start_time_us = timer_get_us();
>>>       hlist_add_head(&cyclic->list, cyclic_get_list());
>>> +
>>> +    return 0;
>>>   }
>>>   void cyclic_unregister(struct cyclic_info *cyclic)
>>>   {
>>> -    hlist_del(&cyclic->list);
>>> +    /* Unregister only initialized struct cyclic_info */
>>> +    if (cyclic->func)
>>> +        hlist_del(&cyclic->list);
>>>   }
>>
>> So this already shows how error prone this approach is. You are not
>> clearing cyclic->func, so if the caller subsequently tries to register
>> that struct again, he would get -EALREADY, while a subsequent unregister
>> could would lead to exactly the list corruption you want to avoid.
>>
>> And unless the caller immediately himself clears ->func, other code in
>> the client cannot rely on ->func being NULL or not as a proxy for
>> whether the struct is already registered (and the caller shouldn't do
>> either of those things, as the struct cyclic_info should be considered
>> opaque).
> 
> I like the approach suggest by Rasmus. Marek, do you see any flaws using
> this version? If not, please send an updated version.
I do see a couple of issues, please see my reply.

Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Rasmus Villemoes]

On Sat, Jan 25 2025, Marek Vasut <marek.vasut@mailbox.org> wrote:

> On 1/20/25 10:17 AM, Rasmus Villemoes wrote:
>> On Sat, Jan 18 2025, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:
>> 
>>> Make cyclic_register() return error code, 0 in case of success,
>>> -EALREADY in case the called attempts to re-register already
>>> registered struct cyclic_info. The re-registration would lead
>>> to corruption of gd->cyclic_list because the re-registration
>>> would memset() one of its nodes, prevent that. Unregister only
>>> initialized struct cyclic_info.
>> I had considered something like this, but I don't like it, because
>> it
>> relies on the cyclic structure (or more likely whatever structure it is
>> embedded in) being initially zero-initialized.
>
> True
>
>> And if the caller doesn't
>> know whether the cyclic_info is already registered or not, he can't do a
>> memset() of it.
>
> This is what can happen right now, which is dangerous and what this
> series attempts to address.
>
>> So my preference would be that we instead simply iterate the current
>> list to see if the struct cyclic_info is already registered that
>> way.
>
> That I can do, but it will be a bit slower.
>

Please. I tried to preempt those kinds of objections by pointing out
that we already traverse the list thousands of times per second, and I
highly doubt we'll ever have more than, say, 10 elements registered, so
we're adding at most that many extra traversals, of a ridiculously short
linked list.

>> Also, I think I'd prefer if double cyclic_register() is allowed and
>> always succeeds; this could be used to change the period of an already
>> registered instance, for example.
>
> This would be terribly overloaded interface, no, let's not do that.

I disagree.

> Better introduce a dedicated function for that kind of period adjustment.
>
>> Also, that avoids making the
>> interfaces fallible.

And I believe that _this_ is an important property to get/preserve. 

>> And cyclic_unregister() could similarly just check
>> whether the passed pointer is already on the list, and be a no-op in
>> case it's not. Those extra list traversals are not expensive (we're
>> traversing them thousands of times per second anyway in cyclic_run), and
>> I doubt one would ever has more than about 10 items on the list.
>> IOW, I'd suggest adding an internal
>> bool cyclic_is_registered(struct cyclic_info *info)
>> {
>>    struct cyclic_info *c;
>>    hlist_for_each(...) if (c == info) return true;
>
> I don't think this works, because that struct cyclic_info contains
> .next_call member, which is updated over time, so this exact match
> would not work as-is.

Huh? Of course it will. I'm not looking at that next_call member at all
(that's not a pointer, that's just some future time stamp), I'm
merely comparing the passed-in pointer to the elements already on the
list.

Yeah, my pseudo-code should probably have used hlist_for_each_entry()
rather than hlist_for_each(), but that should be obvious from the type I
used for the iterator variable.


I have something like this now:
>
> diff --git a/common/cyclic.c b/common/cyclic.c
> index 807a3d73f67..d721a21a575 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -27,11 +27,29 @@ struct hlist_head *cyclic_get_list(void)
>  	return (struct hlist_head *)&gd->cyclic_list;
>  }
>
> +static int cyclic_already_registered(struct cyclic_info *cyclic)
> +{
> +	struct cyclic_info *cycliclst;
> +	struct hlist_node *tmp;
> +
> +	/* Reassignment of function would corrupt cyclic list, exit */
> +	hlist_for_each_entry_safe(cycliclst, tmp, cyclic_get_list(), list) {
> +		if (cycliclst->func == cyclic->func &&
> +		    cycliclst->name == cyclic->name && // or strcmp() ?
> +		    cycliclst->delay_us == cyclic->delay_us &&
> +		    cycliclst->start_time_us == cyclic->start_time_us)
> +			return -EALREADY;	/* Match found */
> +	}
> +

Please no. Just compare the pointers; if they're the same, it's
literally the same struct cyclic_info; if not, it's not. Why would you
dereference the maybe-to-be-registered struct cyclic_info when you don't
expect that to be in any kind of properly initialized state?

> +	/* Match not found */
> +	return 0;
> +}
> +
>  int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>  		    uint64_t delay_us, const char *name)
>  {
>  	/* Reassignment of function would corrupt cyclic list, exit */
> -	if (cyclic->func)
> +	if (!cyclic_already_registered(cyclic))
>  		return -EALREADY;
>
>  	memset(cyclic, 0, sizeof(*cyclic));
> @@ -49,7 +67,7 @@ int cyclic_register(struct cyclic_info *cyclic,
> cyclic_func_t func,
>  void cyclic_unregister(struct cyclic_info *cyclic)
>  {
>  	/* Unregister only initialized struct cyclic_info */
> -	if (cyclic->func)
> +	if (cyclic_already_registered(cyclic))
>  		hlist_del(&cyclic->list);
>  }
>
> [...]
>
>>>   void cyclic_unregister(struct cyclic_info *cyclic)
>>>   {
>>> -	hlist_del(&cyclic->list);
>>> +	/* Unregister only initialized struct cyclic_info */
>>> +	if (cyclic->func)
>>> +		hlist_del(&cyclic->list);
>>>   }
>> So this already shows how error prone this approach is. You are not
>> clearing cyclic->func, so if the caller subsequently tries to register
>> that struct again, he would get -EALREADY, while a subsequent unregister
>> could would lead to exactly the list corruption you want to avoid.
>
> I would expect the caller should clear the structure before attempting
> to register it again. Shall we actually memset() the structure in
> cyclic_unregister() too ?
>

This is what I tried to point out here:

>> And unless the caller immediately himself clears ->func, other code in
>> the client cannot rely on ->func being NULL or not as a proxy for
>> whether the struct is already registered (and the caller shouldn't do
>> either of those things, as the struct cyclic_info should be considered
>> opaque).

So, let's look at the mmc example. In mmc_deinit(), we do
unregister. OK. Later, we might do mmc_init(). Now mmc_init() tries to
figure out if its cyclic_info instance is already registered or
not. There, mmc_init cannot possibly just zero-init the cyclic_info
before the register, because it might be registered, and it can't not do
the zeroing, because then the new registration might spuriously fail.

So for that to work, the code that calls unregister and thus knows that
it has been unregistered would have to immediately do the zero'ing. At
which point it would much better if cyclic_unregister() did that
zero'ing, instead of all callers having to remember that memset().

Rasmus

Re: [PATCH 1/8] cyclic: Prevent corruption of cyclic list on reassignment

[Marek Vasut]

On 1/26/25 10:49 PM, Rasmus Villemoes wrote:

[...]

>>> Also, I think I'd prefer if double cyclic_register() is allowed and
>>> always succeeds; this could be used to change the period of an already
>>> registered instance, for example.
>>
>> This would be terribly overloaded interface, no, let's not do that.
> 
> I disagree.

Can you please elaborate on that ?

>> Better introduce a dedicated function for that kind of period adjustment.
>>
>>> Also, that avoids making the
>>> interfaces fallible.
> 
> And I believe that _this_ is an important property to get/preserve.

What exactly does this mean in this case ?

[...]

I'll see what I can come up with in V2 for the rest.