linux-kernel - RE: [PATCH 08/10] mei: vsc: Run event callback from a workqueue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CY5PR11MB63663E1CC062DC610D81954EED7BA@CY5PR11MB6366.namprd11.prod.outlook.com>
Date: Wed, 25 Jun 2025 10:07:23 +0000
From: "Usyskin, Alexander" <alexander.usyskin@...el.com>
To: Hans de Goede <hansg@...nel.org>, Sakari Ailus
	<sakari.ailus@...ux.intel.com>, Stanislaw Gruszka
	<stanislaw.gruszka@...ux.intel.com>
CC: Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 08/10] mei: vsc: Run event callback from a workqueue

> Subject: [PATCH 08/10] mei: vsc: Run event callback from a workqueue
> 
> The event_notify callback in some cases calls vsc_tp_xfer(), which checks
> tp->assert_cnt and waits for it through the tp->xfer_wait wait-queue.
> 
> And tp->assert_cnt is increased and the tp->xfer_wait queue is woken o
> from the interrupt handler.
> 
> So the interrupt handler which is running the event callback is waiting for
> itself to signal that it can continue.
> 
> This happens to work because the event callback runs from the threaded
> ISR handler and while that is running the hard ISR handler will still
> get called a second / third time for further interrupts and it is the hard
> ISR handler which does the atomic_inc() and wake_up() calls.
> 
> But having the threaded ISR handler wait for its own interrupt to trigger
> again is not how a threaded ISR handler is supposed to be used.
> 
> Move the running of the event callback from a threaded interrupt handler
> to a workqueue since a threaded ISR should not wait for events from its
> own interrupt.
> 
> This is a preparation patch for moving the atomic_inc() and wake_up() calls
> to the threaded ISR handler, which is necessary to fix a locking issue.
> 
Reviewed-by: Alexander Usyskin <alexander.usyskin@...el.com>

> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Hans de Goede <hansg@...nel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 76a6aa606a26..f5ea38f22419 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -18,6 +18,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
> 
>  #include "vsc-tp.h"
> 
> @@ -76,6 +77,7 @@ struct vsc_tp {
> 
>  	atomic_t assert_cnt;
>  	wait_queue_head_t xfer_wait;
> +	struct work_struct event_work;
> 
>  	vsc_tp_event_cb_t event_notify;
>  	void *event_notify_context;
> @@ -105,19 +107,19 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> 
>  	wake_up(&tp->xfer_wait);
> 
> -	return IRQ_WAKE_THREAD;
> +	schedule_work(&tp->event_work);
> +
> +	return IRQ_HANDLED;
>  }
> 
> -static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
> +static void vsc_tp_event_work(struct work_struct *work)
>  {
> -	struct vsc_tp *tp = data;
> +	struct vsc_tp *tp = container_of(work, struct vsc_tp, event_work);
> 
>  	guard(mutex)(&tp->event_notify_mutex);
> 
>  	if (tp->event_notify)
>  		tp->event_notify(tp->event_notify_context);
> -
> -	return IRQ_HANDLED;
>  }
> 
>  /* wakeup firmware and wait for response */
> @@ -495,7 +497,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>  	tp->spi = spi;
> 
>  	irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
> -	ret = request_threaded_irq(spi->irq, vsc_tp_isr, vsc_tp_thread_isr,
> +	ret = request_threaded_irq(spi->irq, vsc_tp_isr, NULL,
>  				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  				   dev_name(dev), tp);
>  	if (ret)
> @@ -503,6 +505,7 @@ static int vsc_tp_probe(struct spi_device *spi)
> 
>  	mutex_init(&tp->mutex);
>  	mutex_init(&tp->event_notify_mutex);
> +	INIT_WORK(&tp->event_work, vsc_tp_event_work);
> 
>  	/* only one child acpi device */
>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
> @@ -527,6 +530,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>  err_destroy_lock:
>  	free_irq(spi->irq, tp);
> 
> +	cancel_work_sync(&tp->event_work);
>  	mutex_destroy(&tp->event_notify_mutex);
>  	mutex_destroy(&tp->mutex);
> 
> @@ -542,6 +546,7 @@ static void vsc_tp_remove(struct spi_device *spi)
> 
>  	free_irq(spi->irq, tp);
> 
> +	cancel_work_sync(&tp->event_work);
>  	mutex_destroy(&tp->event_notify_mutex);
>  	mutex_destroy(&tp->mutex);
>  }
> --
> 2.49.0