[PATCH] platform: cznic: turris-omnia-mcu: stop button emulation producers

[PATCH] platform: cznic: turris-omnia-mcu: stop button emulation producers

[Myeonghun Pak]

The legacy MCU IRQ thread can requeue button-release emulation work
after its managed cleanup has canceled it. Since the IRQ is still active
at that point, the newly queued work can outlive device teardown and
access freed MCU state.

Use disable_delayed_work_sync() to prevent further queueing and drain
the worker before the IRQ action is released. Keep the parent IRQ active
while the worker completes. devm_add_action_or_reset() also drains the
work if action registration fails.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: dfa556e45ae9 ("platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 .../platform/cznic/turris-omnia-mcu-gpio.c    | 31 +++++++++----------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
index 4e430d6c3fc4..8f43bfa92373 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -11,7 +11,6 @@
 #include <linux/bug.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
-#include <linux/devm-helpers.h>
 #include <linux/errno.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
@@ -841,6 +840,18 @@ static void button_release_emul_fn(struct work_struct *work)
 	generic_handle_irq_safe(mcu->client->irq);
 }
 
+static void omnia_disable_button_release_work(void *data)
+{
+	struct delayed_work *work = data;
+
+	/*
+	 * The IRQ thread may requeue this work while the IRQ remains active.
+	 * Disable and drain it before releasing the IRQ action and the MCU
+	 * state used by the worker.
+	 */
+	disable_delayed_work_sync(work);
+}
+
 static void
 fill_int_from_sts(unsigned long *rising, unsigned long *falling,
 		  unsigned long rising_sts, unsigned long falling_sts,
@@ -1082,21 +1093,9 @@ int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu)
 		return dev_err_probe(dev, err, "Cannot request IRQ\n");
 
 	if (!new_api) {
-		/*
-		 * The button_release_emul_work has to be initialized before the
-		 * thread is requested, and on driver remove it needs to be
-		 * canceled before the thread is freed. Therefore we can't use
-		 * devm_delayed_work_autocancel() directly, because the order
-		 *   devm_delayed_work_autocancel();
-		 *   devm_request_threaded_irq();
-		 * would cause improper release order:
-		 *   free_irq();
-		 *   cancel_delayed_work_sync();
-		 * Instead we first initialize the work above, and only now
-		 * after IRQ is requested we add the work devm action.
-		 */
-		err = devm_add_action(dev, devm_delayed_work_drop,
-				      &mcu->button_release_emul_work);
+		err = devm_add_action_or_reset(dev,
+					       omnia_disable_button_release_work,
+					       &mcu->button_release_emul_work);
 		if (err)
 			return err;
 	}

Re: [PATCH] platform: cznic: turris-omnia-mcu: stop button emulation producers

[Marek Behún]

Acked-by: Marek Behún <kabel@kernel.org>

This makes sense.

But I wonder whether the devm_delayed_work_drop() from devm-helpers.h
should not be changed to use disable_delayed_work_sync() instead of
cancel_delayed_work_sync().

In this case the initialization function in devm-helpers should be called
devm_delayed_work_autodisable() instead of devm_delayed_work_autocancel(),
though.

On Thu, Oct 15, 2026 at 09:39:18PM -0400, Myeonghun Pak wrote:
> The legacy MCU IRQ thread can requeue button-release emulation work
> after its managed cleanup has canceled it. Since the IRQ is still active
> at that point, the newly queued work can outlive device teardown and
> access freed MCU state.
> 
> Use disable_delayed_work_sync() to prevent further queueing and drain
> the worker before the IRQ action is released. Keep the parent IRQ active
> while the worker completes. devm_add_action_or_reset() also drains the
> work if action registration fails.
> 
> This issue was identified as part of our ongoing research while
> reviewing kernel code.
> 
> Fixes: dfa556e45ae9 ("platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs")
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  .../platform/cznic/turris-omnia-mcu-gpio.c    | 31 +++++++++----------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
> index 4e430d6c3fc4..8f43bfa92373 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
> @@ -11,7 +11,6 @@
>  #include <linux/bug.h>
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
> -#include <linux/devm-helpers.h>
>  #include <linux/errno.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
> @@ -841,6 +840,18 @@ static void button_release_emul_fn(struct work_struct *work)
>  	generic_handle_irq_safe(mcu->client->irq);
>  }
>  
> +static void omnia_disable_button_release_work(void *data)
> +{
> +	struct delayed_work *work = data;
> +
> +	/*
> +	 * The IRQ thread may requeue this work while the IRQ remains active.
> +	 * Disable and drain it before releasing the IRQ action and the MCU
> +	 * state used by the worker.
> +	 */
> +	disable_delayed_work_sync(work);
> +}
> +
>  static void
>  fill_int_from_sts(unsigned long *rising, unsigned long *falling,
>  		  unsigned long rising_sts, unsigned long falling_sts,
> @@ -1082,21 +1093,9 @@ int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu)
>  		return dev_err_probe(dev, err, "Cannot request IRQ\n");
>  
>  	if (!new_api) {
> -		/*
> -		 * The button_release_emul_work has to be initialized before the
> -		 * thread is requested, and on driver remove it needs to be
> -		 * canceled before the thread is freed. Therefore we can't use
> -		 * devm_delayed_work_autocancel() directly, because the order
> -		 *   devm_delayed_work_autocancel();
> -		 *   devm_request_threaded_irq();
> -		 * would cause improper release order:
> -		 *   free_irq();
> -		 *   cancel_delayed_work_sync();
> -		 * Instead we first initialize the work above, and only now
> -		 * after IRQ is requested we add the work devm action.
> -		 */
> -		err = devm_add_action(dev, devm_delayed_work_drop,
> -				      &mcu->button_release_emul_work);
> +		err = devm_add_action_or_reset(dev,
> +					       omnia_disable_button_release_work,
> +					       &mcu->button_release_emul_work);
>  		if (err)
>  			return err;
>  	}