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

["Marek Behún" <kabel@kernel.org>]

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;
>  	}