[PATCH 00/10] mei: vsc: Various bug-fixes

[PATCH 00/10] mei: vsc: Various bug-fixes

[Hans de Goede]

Hi All,

When running a kernel with CONFIG_PROVE_RAW_LOCK_NESTING on any laptop
with an Intel Visual Sensing Controller chip, the vsc-tp code will
trigger a lockdep warning.

While investigating this I noticed a bunch of other issues / bugs in
the VSC code, resulting in the first 9 patches of this series, fixing:

- An unnecessary 11 second delay on shutdown / reboot
- Destroying the mutex while the threaded ISR which uses it might still
  be running
- Racy use of the event_notify callback
- Dead event_notify callback still being registered after remove()
- Thread ISR waiting for hard ISR to run a second/third time
- The lockdep issue starting all this
- And some cleanups while at it...

Patch 10 is a generic mei debug patch to help catch similar
use-after-free issues as the on I fixed recently [1].

Regards,

Hans

[1] https://lore.kernel.org/linux-media/20250621140052.67912-1-hansg@kernel.org/


Hans de Goede (10):
  mei: vsc: Drop unused vsc_tp_request_irq() and vsc_tp_free_irq()
  mei: vsc: Don't re-init VSC from mei_vsc_hw_reset() on stop
  mei: vsc: Don't call vsc_tp_reset() a second time on shutdown
  mei: vsc: Use vsc_tp_remove() as shutdown handler
  mei: vsc: Destroy mutex after freeing the IRQ
  mei: vsc: Event notifier fixes
  mei: vsc: Unset the event callback on remove and probe errors
  mei: vsc: Run event callback from a workqueue
  mei: vsc: Fix "BUG: Invalid wait context" lockdep error
  mei: bus: Check for still connected devices in
    mei_cl_bus_dev_release()

 drivers/misc/mei/bus.c          |  6 +++
 drivers/misc/mei/platform-vsc.c |  8 ++++
 drivers/misc/mei/vsc-tp.c       | 80 ++++++++++-----------------------
 drivers/misc/mei/vsc-tp.h       |  3 --
 4 files changed, 38 insertions(+), 59 deletions(-)

-- 
2.49.0

[PATCH 01/10] mei: vsc: Drop unused vsc_tp_request_irq() and vsc_tp_free_irq()

[Hans de Goede]

Drop the unused vsc_tp_request_irq() and vsc_tp_free_irq() functions.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/vsc-tp.c | 31 -------------------------------
 drivers/misc/mei/vsc-tp.h |  3 ---
 2 files changed, 34 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 267d0de5fade..99a55451e1fc 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -406,37 +406,6 @@ int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
 }
 EXPORT_SYMBOL_NS_GPL(vsc_tp_register_event_cb, "VSC_TP");
 
-/**
- * vsc_tp_request_irq - request irq for vsc_tp device
- * @tp: vsc_tp device handle
- */
-int vsc_tp_request_irq(struct vsc_tp *tp)
-{
-	struct spi_device *spi = tp->spi;
-	struct device *dev = &spi->dev;
-	int ret;
-
-	irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
-	ret = request_threaded_irq(spi->irq, vsc_tp_isr, vsc_tp_thread_isr,
-				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				   dev_name(dev), tp);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-EXPORT_SYMBOL_NS_GPL(vsc_tp_request_irq, "VSC_TP");
-
-/**
- * vsc_tp_free_irq - free irq for vsc_tp device
- * @tp: vsc_tp device handle
- */
-void vsc_tp_free_irq(struct vsc_tp *tp)
-{
-	free_irq(tp->spi->irq, tp);
-}
-EXPORT_SYMBOL_NS_GPL(vsc_tp_free_irq, "VSC_TP");
-
 /**
  * vsc_tp_intr_synchronize - synchronize vsc_tp interrupt
  * @tp: vsc_tp device handle
diff --git a/drivers/misc/mei/vsc-tp.h b/drivers/misc/mei/vsc-tp.h
index 14ca195cbddc..f9513ddc3e40 100644
--- a/drivers/misc/mei/vsc-tp.h
+++ b/drivers/misc/mei/vsc-tp.h
@@ -37,9 +37,6 @@ int vsc_tp_xfer(struct vsc_tp *tp, u8 cmd, const void *obuf, size_t olen,
 int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
 			     void *context);
 
-int vsc_tp_request_irq(struct vsc_tp *tp);
-void vsc_tp_free_irq(struct vsc_tp *tp);
-
 void vsc_tp_intr_enable(struct vsc_tp *tp);
 void vsc_tp_intr_disable(struct vsc_tp *tp);
 void vsc_tp_intr_synchronize(struct vsc_tp *tp);
-- 
2.49.0

[PATCH 02/10] mei: vsc: Don't re-init VSC from mei_vsc_hw_reset() on stop

[Hans de Goede]

mei_vsc_hw_reset() gets called from mei_start() and mei_stop() in
the latter case we do not need to re-init the VSC by calling vsc_tp_init().

mei_stop() only happens on shutdown and driver unbind. On shutdown we
don't need to load + boot the firmware and if the driver later is
bound to the device again then mei_start() will do another reset.

The intr_enable flag is true when called from mei_start() and false on
mei_stop(). Skip vsc_tp_init() when intr_enable is false.

This avoids unnecessarily uploading the firmware, which takes 11 seconds.
This change reduces the poweroff/reboot time by 11 seconds.

Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/platform-vsc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
index 435760b1e86f..1ac85f0251c5 100644
--- a/drivers/misc/mei/platform-vsc.c
+++ b/drivers/misc/mei/platform-vsc.c
@@ -256,6 +256,9 @@ static int mei_vsc_hw_reset(struct mei_device *mei_dev, bool intr_enable)
 
 	vsc_tp_reset(hw->tp);
 
+	if (!intr_enable)
+		return 0;
+
 	return vsc_tp_init(hw->tp, mei_dev->dev);
 }
 
-- 
2.49.0

[PATCH 03/10] mei: vsc: Don't call vsc_tp_reset() a second time on shutdown

[Hans de Goede]

Now that mei_vsc_hw_reset() no longer re-inits the VSC when called from
mei_stop(), vsc_tp_shutdown() unregistering the platform-device, which
runs mei_stop() is sufficient to put the VSC in a clean state.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/vsc-tp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 99a55451e1fc..4a262e2117e4 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -547,8 +547,6 @@ static void vsc_tp_shutdown(struct spi_device *spi)
 
 	mutex_destroy(&tp->mutex);
 
-	vsc_tp_reset(tp);
-
 	free_irq(spi->irq, tp);
 }
 
-- 
2.49.0

[PATCH 04/10] mei: vsc: Use vsc_tp_remove() as shutdown handler

[Hans de Goede]

After removing the vsc_tp_reset() call from vsc_tp_shutdown() it is now
identical to vsc_tp_remove().

Use vsc_tp_remove() as shutdown handler and remove vsc_tp_shutdown().

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/vsc-tp.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 4a262e2117e4..f5438a600430 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -528,6 +528,7 @@ static int vsc_tp_probe(struct spi_device *spi)
 	return ret;
 }
 
+/* Note this is also used for shutdown */
 static void vsc_tp_remove(struct spi_device *spi)
 {
 	struct vsc_tp *tp = spi_get_drvdata(spi);
@@ -539,17 +540,6 @@ static void vsc_tp_remove(struct spi_device *spi)
 	free_irq(spi->irq, tp);
 }
 
-static void vsc_tp_shutdown(struct spi_device *spi)
-{
-	struct vsc_tp *tp = spi_get_drvdata(spi);
-
-	platform_device_unregister(tp->pdev);
-
-	mutex_destroy(&tp->mutex);
-
-	free_irq(spi->irq, tp);
-}
-
 static const struct acpi_device_id vsc_tp_acpi_ids[] = {
 	{ "INTC1009" }, /* Raptor Lake */
 	{ "INTC1058" }, /* Tiger Lake */
@@ -562,7 +552,7 @@ MODULE_DEVICE_TABLE(acpi, vsc_tp_acpi_ids);
 static struct spi_driver vsc_tp_driver = {
 	.probe = vsc_tp_probe,
 	.remove = vsc_tp_remove,
-	.shutdown = vsc_tp_shutdown,
+	.shutdown = vsc_tp_remove,
 	.driver = {
 		.name = "vsc-tp",
 		.acpi_match_table = vsc_tp_acpi_ids,
-- 
2.49.0

[PATCH 05/10] mei: vsc: Destroy mutex after freeing the IRQ

[Hans de Goede]

The event_notify callback which runs from vsc_tp_thread_isr may call
vsc_tp_xfer() which locks the mutex. So the ISR depends on the mutex.

Move the mutex_destroy() call to after free_irq() to ensure that the ISR
is not running while the mutex is destroyed.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/vsc-tp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index f5438a600430..0feebffabdb3 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -521,10 +521,10 @@ static int vsc_tp_probe(struct spi_device *spi)
 	return 0;
 
 err_destroy_lock:
-	mutex_destroy(&tp->mutex);
-
 	free_irq(spi->irq, tp);
 
+	mutex_destroy(&tp->mutex);
+
 	return ret;
 }
 
@@ -535,9 +535,9 @@ static void vsc_tp_remove(struct spi_device *spi)
 
 	platform_device_unregister(tp->pdev);
 
-	mutex_destroy(&tp->mutex);
-
 	free_irq(spi->irq, tp);
+
+	mutex_destroy(&tp->mutex);
 }
 
 static const struct acpi_device_id vsc_tp_acpi_ids[] = {
-- 
2.49.0

[PATCH 06/10] mei: vsc: Event notifier fixes

[Hans de Goede]

vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
to protect against this.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/vsc-tp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 0feebffabdb3..76a6aa606a26 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -79,9 +79,8 @@ struct vsc_tp {
 
 	vsc_tp_event_cb_t event_notify;
 	void *event_notify_context;
-
-	/* used to protect command download */
-	struct mutex mutex;
+	struct mutex event_notify_mutex;	/* protects event_notify + context */
+	struct mutex mutex;			/* protects command download */
 };
 
 /* GPIO resources */
@@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
 {
 	struct vsc_tp *tp = data;
 
+	guard(mutex)(&tp->event_notify_mutex);
+
 	if (tp->event_notify)
 		tp->event_notify(tp->event_notify_context);
 
@@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read, "VSC_TP");
 int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
 			    void *context)
 {
+	guard(mutex)(&tp->event_notify_mutex);
+
 	tp->event_notify = event_cb;
 	tp->event_notify_context = context;
 
@@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
 		return ret;
 
 	mutex_init(&tp->mutex);
+	mutex_init(&tp->event_notify_mutex);
 
 	/* only one child acpi device */
 	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
@@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
 err_destroy_lock:
 	free_irq(spi->irq, tp);
 
+	mutex_destroy(&tp->event_notify_mutex);
 	mutex_destroy(&tp->mutex);
 
 	return ret;
@@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
 
 	free_irq(spi->irq, tp);
 
+	mutex_destroy(&tp->event_notify_mutex);
 	mutex_destroy(&tp->mutex);
 }
 
-- 
2.49.0

[PATCH 07/10] mei: vsc: Unset the event callback on remove and probe errors

[Hans de Goede]

Make mei_vsc_remove() properly unset the callback to avoid a dead callback
sticking around after probe errors or unbinding of the platform driver.

Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/platform-vsc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
index 1ac85f0251c5..b2b5a20ae3fa 100644
--- a/drivers/misc/mei/platform-vsc.c
+++ b/drivers/misc/mei/platform-vsc.c
@@ -380,6 +380,8 @@ static int mei_vsc_probe(struct platform_device *pdev)
 err_cancel:
 	mei_cancel_work(mei_dev);
 
+	vsc_tp_register_event_cb(tp, NULL, NULL);
+
 	mei_disable_interrupts(mei_dev);
 
 	return ret;
@@ -388,11 +390,14 @@ static int mei_vsc_probe(struct platform_device *pdev)
 static void mei_vsc_remove(struct platform_device *pdev)
 {
 	struct mei_device *mei_dev = platform_get_drvdata(pdev);
+	struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
 
 	pm_runtime_disable(mei_dev->dev);
 
 	mei_stop(mei_dev);
 
+	vsc_tp_register_event_cb(hw->tp, NULL, NULL);
+
 	mei_disable_interrupts(mei_dev);
 
 	mei_deregister(mei_dev);
-- 
2.49.0

[PATCH 08/10] mei: vsc: Run event callback from a workqueue

[Hans de Goede]

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.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Hans de Goede <hansg@kernel.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

[PATCH 09/10] mei: vsc: Fix "BUG: Invalid wait context" lockdep error

[Hans de Goede]

Kernels build with CONFIG_PROVE_RAW_LOCK_NESTING report the following
tp-vsc lockdep error:

=============================
 [ BUG: Invalid wait context ]
 ...
 swapper/10/0 is trying to lock:
 ffff88819c271888 (&tp->xfer_wait){....}-{3:3},
  at: __wake_up (kernel/sched/wait.c:106 kernel/sched/wait.c:127)
 ...
 Call Trace:
 <IRQ>
 ...
 __raw_spin_lock_irqsave (./include/linux/spinlock_api_smp.h:111)
 __wake_up (kernel/sched/wait.c:106 kernel/sched/wait.c:127)
 vsc_tp_isr (drivers/misc/mei/vsc-tp.c:110) mei_vsc_hw
 __handle_irq_event_percpu (kernel/irq/handle.c:158)
 handle_irq_event (kernel/irq/handle.c:195 kernel/irq/handle.c:210)
 handle_edge_irq (kernel/irq/chip.c:833)
 ...
 </IRQ>

The root-cause of this is the IRQF_NO_THREAD flag used by the intel-pinctrl
code. Setting IRQF_NO_THREAD requires all interrupt handlers for GPIO ISRs
to use raw-spinlocks only since normal spinlocks can sleep in PREEMPT-RT
kernels and with IRQF_NO_THREAD the interrupt handlers will always run in
an atomic context [1].

vsc_tp_isr() calls wake_up(&tp->xfer_wait), which uses a regular spinlock,
breaking the raw-spinlocks only rule for Intel GPIO ISRs.

Make vsc_tp_isr() run as threaded ISR instead of as hard ISR to fix this.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Link: https://lore.kernel.org/linux-gpio/18ab52bd-9171-4667-a600-0f52ab7017ac@kernel.org/ [1]
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/vsc-tp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index f5ea38f22419..5ecf99883996 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -497,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, NULL,
+	ret = request_threaded_irq(spi->irq, NULL, vsc_tp_isr,
 				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				   dev_name(dev), tp);
 	if (ret)
-- 
2.49.0

[PATCH 10/10] mei: bus: Check for still connected devices in mei_cl_bus_dev_release()

[Hans de Goede]

mei_cl_bus_dev_release() also frees the mei-client (struct mei_cl)
belonging to the device being released.

If there are bugs like the just fixed bug in the ACE/CSI2 mei drivers,
the mei-client being freed might still be part of the mei_device's
file_list and iterating over this list after the freeing will then trigger
a use-afer-free bug.

Add a check to mei_cl_bus_dev_release() to make sure that the to-be-freed
mei-client is not on the mei_device's file_list.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/misc/mei/bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
index 67176caf5416..1958c043ac14 100644
--- a/drivers/misc/mei/bus.c
+++ b/drivers/misc/mei/bus.c
@@ -1301,10 +1301,16 @@ static void mei_dev_bus_put(struct mei_device *bus)
 static void mei_cl_bus_dev_release(struct device *dev)
 {
 	struct mei_cl_device *cldev = to_mei_cl_device(dev);
+	struct mei_device *mdev = cldev->cl->dev;
+	struct mei_cl *cl;
 
 	mei_cl_flush_queues(cldev->cl, NULL);
 	mei_me_cl_put(cldev->me_cl);
 	mei_dev_bus_put(cldev->bus);
+
+	list_for_each_entry(cl, &mdev->file_list, link)
+		WARN_ON(cl == cldev->cl);
+
 	kfree(cldev->cl);
 	kfree(cldev);
 }
-- 
2.49.0

RE: [PATCH 01/10] mei: vsc: Drop unused vsc_tp_request_irq() and vsc_tp_free_irq()

[Usyskin, Alexander]

> Subject: [PATCH 01/10] mei: vsc: Drop unused vsc_tp_request_irq() and
> vsc_tp_free_irq()
> 
> Drop the unused vsc_tp_request_irq() and vsc_tp_free_irq() functions.
> 

This is proposed in
https://lkml.org/lkml/2025/6/17/25 "[PATCH] mei: vsc: Remove unused irq functions"

But I haven't seen that Greg pulled patch in -next.

- - 
Thanks,
Sasha


> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 31 -------------------------------
>  drivers/misc/mei/vsc-tp.h |  3 ---
>  2 files changed, 34 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 267d0de5fade..99a55451e1fc 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -406,37 +406,6 @@ int vsc_tp_register_event_cb(struct vsc_tp *tp,
> vsc_tp_event_cb_t event_cb,
>  }
>  EXPORT_SYMBOL_NS_GPL(vsc_tp_register_event_cb, "VSC_TP");
> 
> -/**
> - * vsc_tp_request_irq - request irq for vsc_tp device
> - * @tp: vsc_tp device handle
> - */
> -int vsc_tp_request_irq(struct vsc_tp *tp)
> -{
> -	struct spi_device *spi = tp->spi;
> -	struct device *dev = &spi->dev;
> -	int ret;
> -
> -	irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
> -	ret = request_threaded_irq(spi->irq, vsc_tp_isr, vsc_tp_thread_isr,
> -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				   dev_name(dev), tp);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_NS_GPL(vsc_tp_request_irq, "VSC_TP");
> -
> -/**
> - * vsc_tp_free_irq - free irq for vsc_tp device
> - * @tp: vsc_tp device handle
> - */
> -void vsc_tp_free_irq(struct vsc_tp *tp)
> -{
> -	free_irq(tp->spi->irq, tp);
> -}
> -EXPORT_SYMBOL_NS_GPL(vsc_tp_free_irq, "VSC_TP");
> -
>  /**
>   * vsc_tp_intr_synchronize - synchronize vsc_tp interrupt
>   * @tp: vsc_tp device handle
> diff --git a/drivers/misc/mei/vsc-tp.h b/drivers/misc/mei/vsc-tp.h
> index 14ca195cbddc..f9513ddc3e40 100644
> --- a/drivers/misc/mei/vsc-tp.h
> +++ b/drivers/misc/mei/vsc-tp.h
> @@ -37,9 +37,6 @@ int vsc_tp_xfer(struct vsc_tp *tp, u8 cmd, const void
> *obuf, size_t olen,
>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
>  			     void *context);
> 
> -int vsc_tp_request_irq(struct vsc_tp *tp);
> -void vsc_tp_free_irq(struct vsc_tp *tp);
> -
>  void vsc_tp_intr_enable(struct vsc_tp *tp);
>  void vsc_tp_intr_disable(struct vsc_tp *tp);
>  void vsc_tp_intr_synchronize(struct vsc_tp *tp);
> --
> 2.49.0

RE: [PATCH 02/10] mei: vsc: Don't re-init VSC from mei_vsc_hw_reset() on stop

[Usyskin, Alexander]

> Subject: [PATCH 02/10] mei: vsc: Don't re-init VSC from mei_vsc_hw_reset()
> on stop
> 
> mei_vsc_hw_reset() gets called from mei_start() and mei_stop() in
> the latter case we do not need to re-init the VSC by calling vsc_tp_init().
> 
> mei_stop() only happens on shutdown and driver unbind. On shutdown we
> don't need to load + boot the firmware and if the driver later is
> bound to the device again then mei_start() will do another reset.
> 
> The intr_enable flag is true when called from mei_start() and false on
> mei_stop(). Skip vsc_tp_init() when intr_enable is false.
> 
> This avoids unnecessarily uploading the firmware, which takes 11 seconds.
> This change reduces the poweroff/reboot time by 11 seconds.
> 

Reviewed-by:  Alexander Usyskin <alexander.usyskin@intel.com>

> Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/platform-vsc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
> index 435760b1e86f..1ac85f0251c5 100644
> --- a/drivers/misc/mei/platform-vsc.c
> +++ b/drivers/misc/mei/platform-vsc.c
> @@ -256,6 +256,9 @@ static int mei_vsc_hw_reset(struct mei_device
> *mei_dev, bool intr_enable)
> 
>  	vsc_tp_reset(hw->tp);
> 
> +	if (!intr_enable)
> +		return 0;
> +
>  	return vsc_tp_init(hw->tp, mei_dev->dev);
>  }
> 
> --
> 2.49.0

Re: [PATCH 01/10] mei: vsc: Drop unused vsc_tp_request_irq() and vsc_tp_free_irq()

[Hans de Goede]

Hi,

On 24-Jun-25 8:06 AM, Usyskin, Alexander wrote:
>> Subject: [PATCH 01/10] mei: vsc: Drop unused vsc_tp_request_irq() and
>> vsc_tp_free_irq()
>>
>> Drop the unused vsc_tp_request_irq() and vsc_tp_free_irq() functions.
>>
> 
> This is proposed in
> https://lkml.org/lkml/2025/6/17/25 "[PATCH] mei: vsc: Remove unused irq functions"

Thank you for pointing that out.

Normally I would say lets go with David's version since that was
posted first.

But David's patch is missing the removal of the function prototypes
from vsc-tp.h, so in this case I think we should go with my version.

Regards,

Hans




>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/misc/mei/vsc-tp.c | 31 -------------------------------
>>  drivers/misc/mei/vsc-tp.h |  3 ---
>>  2 files changed, 34 deletions(-)
>>
>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
>> index 267d0de5fade..99a55451e1fc 100644
>> --- a/drivers/misc/mei/vsc-tp.c
>> +++ b/drivers/misc/mei/vsc-tp.c
>> @@ -406,37 +406,6 @@ int vsc_tp_register_event_cb(struct vsc_tp *tp,
>> vsc_tp_event_cb_t event_cb,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(vsc_tp_register_event_cb, "VSC_TP");
>>
>> -/**
>> - * vsc_tp_request_irq - request irq for vsc_tp device
>> - * @tp: vsc_tp device handle
>> - */
>> -int vsc_tp_request_irq(struct vsc_tp *tp)
>> -{
>> -	struct spi_device *spi = tp->spi;
>> -	struct device *dev = &spi->dev;
>> -	int ret;
>> -
>> -	irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
>> -	ret = request_threaded_irq(spi->irq, vsc_tp_isr, vsc_tp_thread_isr,
>> -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> -				   dev_name(dev), tp);
>> -	if (ret)
>> -		return ret;
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_NS_GPL(vsc_tp_request_irq, "VSC_TP");
>> -
>> -/**
>> - * vsc_tp_free_irq - free irq for vsc_tp device
>> - * @tp: vsc_tp device handle
>> - */
>> -void vsc_tp_free_irq(struct vsc_tp *tp)
>> -{
>> -	free_irq(tp->spi->irq, tp);
>> -}
>> -EXPORT_SYMBOL_NS_GPL(vsc_tp_free_irq, "VSC_TP");
>> -
>>  /**
>>   * vsc_tp_intr_synchronize - synchronize vsc_tp interrupt
>>   * @tp: vsc_tp device handle
>> diff --git a/drivers/misc/mei/vsc-tp.h b/drivers/misc/mei/vsc-tp.h
>> index 14ca195cbddc..f9513ddc3e40 100644
>> --- a/drivers/misc/mei/vsc-tp.h
>> +++ b/drivers/misc/mei/vsc-tp.h
>> @@ -37,9 +37,6 @@ int vsc_tp_xfer(struct vsc_tp *tp, u8 cmd, const void
>> *obuf, size_t olen,
>>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
>>  			     void *context);
>>
>> -int vsc_tp_request_irq(struct vsc_tp *tp);
>> -void vsc_tp_free_irq(struct vsc_tp *tp);
>> -
>>  void vsc_tp_intr_enable(struct vsc_tp *tp);
>>  void vsc_tp_intr_disable(struct vsc_tp *tp);
>>  void vsc_tp_intr_synchronize(struct vsc_tp *tp);
>> --
>> 2.49.0
> 

RE: [PATCH 03/10] mei: vsc: Don't call vsc_tp_reset() a second time on shutdown

[Usyskin, Alexander]

> Subject: [PATCH 03/10] mei: vsc: Don't call vsc_tp_reset() a second time on
> shutdown
> 
> Now that mei_vsc_hw_reset() no longer re-inits the VSC when called from
> mei_stop(), vsc_tp_shutdown() unregistering the platform-device, which
> runs mei_stop() is sufficient to put the VSC in a clean state.
> 

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 99a55451e1fc..4a262e2117e4 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -547,8 +547,6 @@ static void vsc_tp_shutdown(struct spi_device *spi)
> 
>  	mutex_destroy(&tp->mutex);
> 
> -	vsc_tp_reset(tp);
> -
>  	free_irq(spi->irq, tp);
>  }
> 
> --
> 2.49.0

RE: [PATCH 04/10] mei: vsc: Use vsc_tp_remove() as shutdown handler

[Usyskin, Alexander]

> Subject: [PATCH 04/10] mei: vsc: Use vsc_tp_remove() as shutdown handler
> 
> After removing the vsc_tp_reset() call from vsc_tp_shutdown() it is now
> identical to vsc_tp_remove().
> 
> Use vsc_tp_remove() as shutdown handler and remove vsc_tp_shutdown().
> 

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 4a262e2117e4..f5438a600430 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -528,6 +528,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>  	return ret;
>  }
> 
> +/* Note this is also used for shutdown */
>  static void vsc_tp_remove(struct spi_device *spi)
>  {
>  	struct vsc_tp *tp = spi_get_drvdata(spi);
> @@ -539,17 +540,6 @@ static void vsc_tp_remove(struct spi_device *spi)
>  	free_irq(spi->irq, tp);
>  }
> 
> -static void vsc_tp_shutdown(struct spi_device *spi)
> -{
> -	struct vsc_tp *tp = spi_get_drvdata(spi);
> -
> -	platform_device_unregister(tp->pdev);
> -
> -	mutex_destroy(&tp->mutex);
> -
> -	free_irq(spi->irq, tp);
> -}
> -
>  static const struct acpi_device_id vsc_tp_acpi_ids[] = {
>  	{ "INTC1009" }, /* Raptor Lake */
>  	{ "INTC1058" }, /* Tiger Lake */
> @@ -562,7 +552,7 @@ MODULE_DEVICE_TABLE(acpi, vsc_tp_acpi_ids);
>  static struct spi_driver vsc_tp_driver = {
>  	.probe = vsc_tp_probe,
>  	.remove = vsc_tp_remove,
> -	.shutdown = vsc_tp_shutdown,
> +	.shutdown = vsc_tp_remove,
>  	.driver = {
>  		.name = "vsc-tp",
>  		.acpi_match_table = vsc_tp_acpi_ids,
> --
> 2.49.0

RE: [PATCH 05/10] mei: vsc: Destroy mutex after freeing the IRQ

[Usyskin, Alexander]

> Subject: [PATCH 05/10] mei: vsc: Destroy mutex after freeing the IRQ
> 
> The event_notify callback which runs from vsc_tp_thread_isr may call
> vsc_tp_xfer() which locks the mutex. So the ISR depends on the mutex.
> 
> Move the mutex_destroy() call to after free_irq() to ensure that the ISR
> is not running while the mutex is destroyed.
> 

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index f5438a600430..0feebffabdb3 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -521,10 +521,10 @@ static int vsc_tp_probe(struct spi_device *spi)
>  	return 0;
> 
>  err_destroy_lock:
> -	mutex_destroy(&tp->mutex);
> -
>  	free_irq(spi->irq, tp);
> 
> +	mutex_destroy(&tp->mutex);
> +
>  	return ret;
>  }
> 
> @@ -535,9 +535,9 @@ static void vsc_tp_remove(struct spi_device *spi)
> 
>  	platform_device_unregister(tp->pdev);
> 
> -	mutex_destroy(&tp->mutex);
> -
>  	free_irq(spi->irq, tp);
> +
> +	mutex_destroy(&tp->mutex);
>  }
> 
>  static const struct acpi_device_id vsc_tp_acpi_ids[] = {
> --
> 2.49.0

RE: [PATCH 06/10] mei: vsc: Event notifier fixes

[Usyskin, Alexander]

> Subject: [PATCH 06/10] mei: vsc: Event notifier fixes
> 
> vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
> to protect against this.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 0feebffabdb3..76a6aa606a26 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -79,9 +79,8 @@ struct vsc_tp {
> 
>  	vsc_tp_event_cb_t event_notify;
>  	void *event_notify_context;
> -
> -	/* used to protect command download */
> -	struct mutex mutex;
> +	struct mutex event_notify_mutex;	/* protects event_notify +
> context */
> +	struct mutex mutex;			/* protects command
> download */
>  };
> 
>  /* GPIO resources */
> @@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> *data)
>  {
>  	struct vsc_tp *tp = data;
> 

The mutex overhead looks out of place here in the interrupt handler.
Maybe it can be replaced with something lighter?

BTW is it possible to have interrupt before call to vsc_tp_register_event_cb?

- - 
Thanks,
Sasha

> +	guard(mutex)(&tp->event_notify_mutex);
> +
>  	if (tp->event_notify)
>  		tp->event_notify(tp->event_notify_context);
> 
> @@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read,
> "VSC_TP");
>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
>  			    void *context)
>  {
> +	guard(mutex)(&tp->event_notify_mutex);
> +
>  	tp->event_notify = event_cb;
>  	tp->event_notify_context = context;
> 
> @@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>  		return ret;
> 
>  	mutex_init(&tp->mutex);
> +	mutex_init(&tp->event_notify_mutex);
> 
>  	/* only one child acpi device */
>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
> @@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>  err_destroy_lock:
>  	free_irq(spi->irq, tp);
> 
> +	mutex_destroy(&tp->event_notify_mutex);
>  	mutex_destroy(&tp->mutex);
> 
>  	return ret;
> @@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
> 
>  	free_irq(spi->irq, tp);
> 
> +	mutex_destroy(&tp->event_notify_mutex);
>  	mutex_destroy(&tp->mutex);
>  }
> 
> --
> 2.49.0

Re: [PATCH 06/10] mei: vsc: Event notifier fixes

[Hans de Goede]

Hi,

On 25-Jun-25 11:12 AM, Usyskin, Alexander wrote:
>> Subject: [PATCH 06/10] mei: vsc: Event notifier fixes
>>
>> vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
>> to protect against this.
>>
>> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/misc/mei/vsc-tp.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
>> index 0feebffabdb3..76a6aa606a26 100644
>> --- a/drivers/misc/mei/vsc-tp.c
>> +++ b/drivers/misc/mei/vsc-tp.c
>> @@ -79,9 +79,8 @@ struct vsc_tp {
>>
>>  	vsc_tp_event_cb_t event_notify;
>>  	void *event_notify_context;
>> -
>> -	/* used to protect command download */
>> -	struct mutex mutex;
>> +	struct mutex event_notify_mutex;	/* protects event_notify +
>> context */
>> +	struct mutex mutex;			/* protects command
>> download */
>>  };
>>
>>  /* GPIO resources */
>> @@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
>> *data)
>>  {
>>  	struct vsc_tp *tp = data;
>>
> 
> The mutex overhead looks out of place here in the interrupt handler.
> Maybe it can be replaced with something lighter?

Using mutexes in *threaded* isr handlers is quite normal, e.g.
both the SPI core (used as transport here) and the I2C cor will
take + release a mutex for each data transfer over the bus and
a threaded ISR handler may do more then 1 data transfer for a single
interrupt.

As to using something lighter I could not come up with any lighter
solution then this.

Also note that this is moved to a workqueue later in the series,
since the threaded ISR actually waits for the wake_up() call
done by the hard part of the ISR and an ISR waiting for
the interrupt to trigger a second/third/... time inside the ISR
handler is just plain wrong.

> BTW is it possible to have interrupt before call to vsc_tp_register_event_c

The interrupt gets triggered by a GPIO connected to the VSC,
so if the VSC is well behaved then the interrupt should not
trigger. But we cannot really count on that.

Regards,

Hans



>> +	guard(mutex)(&tp->event_notify_mutex);
>> +
>>  	if (tp->event_notify)
>>  		tp->event_notify(tp->event_notify_context);
>>
>> @@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read,
>> "VSC_TP");
>>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
>>  			    void *context)
>>  {
>> +	guard(mutex)(&tp->event_notify_mutex);
>> +
>>  	tp->event_notify = event_cb;
>>  	tp->event_notify_context = context;
>>
>> @@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>>  		return ret;
>>
>>  	mutex_init(&tp->mutex);
>> +	mutex_init(&tp->event_notify_mutex);
>>
>>  	/* only one child acpi device */
>>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
>> @@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>>  err_destroy_lock:
>>  	free_irq(spi->irq, tp);
>>
>> +	mutex_destroy(&tp->event_notify_mutex);
>>  	mutex_destroy(&tp->mutex);
>>
>>  	return ret;
>> @@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
>>
>>  	free_irq(spi->irq, tp);
>>
>> +	mutex_destroy(&tp->event_notify_mutex);
>>  	mutex_destroy(&tp->mutex);
>>  }
>>
>> --
>> 2.49.0
> 

Re: [PATCH 06/10] mei: vsc: Event notifier fixes

[Hans de Goede]

On 25-Jun-25 11:23 AM, Hans de Goede wrote:
> Hi,
> 
> On 25-Jun-25 11:12 AM, Usyskin, Alexander wrote:
>>> Subject: [PATCH 06/10] mei: vsc: Event notifier fixes
>>>
>>> vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
>>> to protect against this.
>>>
>>> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>> ---
>>>  drivers/misc/mei/vsc-tp.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
>>> index 0feebffabdb3..76a6aa606a26 100644
>>> --- a/drivers/misc/mei/vsc-tp.c
>>> +++ b/drivers/misc/mei/vsc-tp.c
>>> @@ -79,9 +79,8 @@ struct vsc_tp {
>>>
>>>  	vsc_tp_event_cb_t event_notify;
>>>  	void *event_notify_context;
>>> -
>>> -	/* used to protect command download */
>>> -	struct mutex mutex;
>>> +	struct mutex event_notify_mutex;	/* protects event_notify +
>>> context */
>>> +	struct mutex mutex;			/* protects command
>>> download */
>>>  };
>>>
>>>  /* GPIO resources */
>>> @@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
>>> *data)
>>>  {
>>>  	struct vsc_tp *tp = data;
>>>
>>
>> The mutex overhead looks out of place here in the interrupt handler.
>> Maybe it can be replaced with something lighter?
> 
> Using mutexes in *threaded* isr handlers is quite normal, e.g.
> both the SPI core (used as transport here) and the I2C cor will
> take + release a mutex for each data transfer over the bus and
> a threaded ISR handler may do more then 1 data transfer for a single
> interrupt.
> 
> As to using something lighter I could not come up with any lighter
> solution then this.

p.s.

I forgot to mention that this interrupt also does not trigger
that frequently that we really need to worry about overhead.

The VSC sits between the user-facing camera and the Intel
CPU/SoC's CSI2 receiver. So we only need to talk to it
(generating interrupts) on probe() and when starting/stopping
streaming video from the camera. Each start/stop we'll get
a bunch of interrupts but outside of that the interrupt never
triggers. So overhead is not really a big worry here.

Regards,

Hans





> Also note that this is moved to a workqueue later in the series,
> since the threaded ISR actually waits for the wake_up() call
> done by the hard part of the ISR and an ISR waiting for
> the interrupt to trigger a second/third/... time inside the ISR
> handler is just plain wrong.
> 
>> BTW is it possible to have interrupt before call to vsc_tp_register_event_c
> 
> The interrupt gets triggered by a GPIO connected to the VSC,
> so if the VSC is well behaved then the interrupt should not
> trigger. But we cannot really count on that.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>> +	guard(mutex)(&tp->event_notify_mutex);
>>> +
>>>  	if (tp->event_notify)
>>>  		tp->event_notify(tp->event_notify_context);
>>>
>>> @@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read,
>>> "VSC_TP");
>>>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
>>>  			    void *context)
>>>  {
>>> +	guard(mutex)(&tp->event_notify_mutex);
>>> +
>>>  	tp->event_notify = event_cb;
>>>  	tp->event_notify_context = context;
>>>
>>> @@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>>>  		return ret;
>>>
>>>  	mutex_init(&tp->mutex);
>>> +	mutex_init(&tp->event_notify_mutex);
>>>
>>>  	/* only one child acpi device */
>>>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
>>> @@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>>>  err_destroy_lock:
>>>  	free_irq(spi->irq, tp);
>>>
>>> +	mutex_destroy(&tp->event_notify_mutex);
>>>  	mutex_destroy(&tp->mutex);
>>>
>>>  	return ret;
>>> @@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
>>>
>>>  	free_irq(spi->irq, tp);
>>>
>>> +	mutex_destroy(&tp->event_notify_mutex);
>>>  	mutex_destroy(&tp->mutex);
>>>  }
>>>
>>> --
>>> 2.49.0
>>
> 

RE: [PATCH 06/10] mei: vsc: Event notifier fixes

[Usyskin, Alexander]

> Subject: Re: [PATCH 06/10] mei: vsc: Event notifier fixes
> 
> On 25-Jun-25 11:23 AM, Hans de Goede wrote:
> > Hi,
> >
> > On 25-Jun-25 11:12 AM, Usyskin, Alexander wrote:
> >>> Subject: [PATCH 06/10] mei: vsc: Event notifier fixes
> >>>
> >>> vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
> >>> to protect against this.
> >>>
> >>> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> >>> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >>> ---
> >>>  drivers/misc/mei/vsc-tp.c | 12 +++++++++---
> >>>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> >>> index 0feebffabdb3..76a6aa606a26 100644
> >>> --- a/drivers/misc/mei/vsc-tp.c
> >>> +++ b/drivers/misc/mei/vsc-tp.c
> >>> @@ -79,9 +79,8 @@ struct vsc_tp {
> >>>
> >>>  	vsc_tp_event_cb_t event_notify;
> >>>  	void *event_notify_context;
> >>> -
> >>> -	/* used to protect command download */
> >>> -	struct mutex mutex;
> >>> +	struct mutex event_notify_mutex;	/* protects event_notify +
> >>> context */
> >>> +	struct mutex mutex;			/* protects command
> >>> download */
> >>>  };
> >>>
> >>>  /* GPIO resources */
> >>> @@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> >>> *data)
> >>>  {
> >>>  	struct vsc_tp *tp = data;
> >>>
> >>
> >> The mutex overhead looks out of place here in the interrupt handler.
> >> Maybe it can be replaced with something lighter?
> >
> > Using mutexes in *threaded* isr handlers is quite normal, e.g.
> > both the SPI core (used as transport here) and the I2C cor will
> > take + release a mutex for each data transfer over the bus and
> > a threaded ISR handler may do more then 1 data transfer for a single
> > interrupt.
> >
> > As to using something lighter I could not come up with any lighter
> > solution then this.
> 
> p.s.
> 
> I forgot to mention that this interrupt also does not trigger
> that frequently that we really need to worry about overhead.
> 
> The VSC sits between the user-facing camera and the Intel
> CPU/SoC's CSI2 receiver. So we only need to talk to it
> (generating interrupts) on probe() and when starting/stopping
> streaming video from the camera. Each start/stop we'll get
> a bunch of interrupts but outside of that the interrupt never
> triggers. So overhead is not really a big worry here.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> > Also note that this is moved to a workqueue later in the series,
> > since the threaded ISR actually waits for the wake_up() call
> > done by the hard part of the ISR and an ISR waiting for
> > the interrupt to trigger a second/third/... time inside the ISR
> > handler is just plain wrong.
> >
> >> BTW is it possible to have interrupt before call to vsc_tp_register_event_c
> >
> > The interrupt gets triggered by a GPIO connected to the VSC,
> > so if the VSC is well behaved then the interrupt should not
> > trigger. But we cannot really count on that.
> >
> > Regards,
> >
> > Hans
> >

Look like better be safe then sorry here, let's add mutex.

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> >
> >
> >>> +	guard(mutex)(&tp->event_notify_mutex);
> >>> +
> >>>  	if (tp->event_notify)
> >>>  		tp->event_notify(tp->event_notify_context);
> >>>
> >>> @@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read,
> >>> "VSC_TP");
> >>>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t
> event_cb,
> >>>  			    void *context)
> >>>  {
> >>> +	guard(mutex)(&tp->event_notify_mutex);
> >>> +
> >>>  	tp->event_notify = event_cb;
> >>>  	tp->event_notify_context = context;
> >>>
> >>> @@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
> >>>  		return ret;
> >>>
> >>>  	mutex_init(&tp->mutex);
> >>> +	mutex_init(&tp->event_notify_mutex);
> >>>
> >>>  	/* only one child acpi device */
> >>>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
> >>> @@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
> >>>  err_destroy_lock:
> >>>  	free_irq(spi->irq, tp);
> >>>
> >>> +	mutex_destroy(&tp->event_notify_mutex);
> >>>  	mutex_destroy(&tp->mutex);
> >>>
> >>>  	return ret;
> >>> @@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
> >>>
> >>>  	free_irq(spi->irq, tp);
> >>>
> >>> +	mutex_destroy(&tp->event_notify_mutex);
> >>>  	mutex_destroy(&tp->mutex);
> >>>  }
> >>>
> >>> --
> >>> 2.49.0
> >>
> >

Re: [PATCH 00/10] mei: vsc: Various bug-fixes

[Sakari Ailus]

Hi Hans,

On Mon, Jun 23, 2025 at 10:50:42AM +0200, Hans de Goede wrote:
> Hi All,
> 
> When running a kernel with CONFIG_PROVE_RAW_LOCK_NESTING on any laptop
> with an Intel Visual Sensing Controller chip, the vsc-tp code will
> trigger a lockdep warning.
> 
> While investigating this I noticed a bunch of other issues / bugs in
> the VSC code, resulting in the first 9 patches of this series, fixing:
> 
> - An unnecessary 11 second delay on shutdown / reboot
> - Destroying the mutex while the threaded ISR which uses it might still
>   be running
> - Racy use of the event_notify callback
> - Dead event_notify callback still being registered after remove()
> - Thread ISR waiting for hard ISR to run a second/third time
> - The lockdep issue starting all this
> - And some cleanups while at it...
> 
> Patch 10 is a generic mei debug patch to help catch similar
> use-after-free issues as the on I fixed recently [1].

Thanks for the set.

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

RE: [PATCH 07/10] mei: vsc: Unset the event callback on remove and probe errors

[Usyskin, Alexander]

> Subject: [PATCH 07/10] mei: vsc: Unset the event callback on remove and
> probe errors
> 
> Make mei_vsc_remove() properly unset the callback to avoid a dead callback
> sticking around after probe errors or unbinding of the platform driver.
> 

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/platform-vsc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
> index 1ac85f0251c5..b2b5a20ae3fa 100644
> --- a/drivers/misc/mei/platform-vsc.c
> +++ b/drivers/misc/mei/platform-vsc.c
> @@ -380,6 +380,8 @@ static int mei_vsc_probe(struct platform_device
> *pdev)
>  err_cancel:
>  	mei_cancel_work(mei_dev);
> 
> +	vsc_tp_register_event_cb(tp, NULL, NULL);
> +
>  	mei_disable_interrupts(mei_dev);
> 
>  	return ret;
> @@ -388,11 +390,14 @@ static int mei_vsc_probe(struct platform_device
> *pdev)
>  static void mei_vsc_remove(struct platform_device *pdev)
>  {
>  	struct mei_device *mei_dev = platform_get_drvdata(pdev);
> +	struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
> 
>  	pm_runtime_disable(mei_dev->dev);
> 
>  	mei_stop(mei_dev);
> 
> +	vsc_tp_register_event_cb(hw->tp, NULL, NULL);
> +
>  	mei_disable_interrupts(mei_dev);
> 
>  	mei_deregister(mei_dev);
> --
> 2.49.0

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

[Usyskin, Alexander]

> 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@intel.com>

> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Hans de Goede <hansg@kernel.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

RE: [PATCH 09/10] mei: vsc: Fix "BUG: Invalid wait context" lockdep error

[Usyskin, Alexander]

> Subject: [PATCH 09/10] mei: vsc: Fix "BUG: Invalid wait context" lockdep error
> 
> Kernels build with CONFIG_PROVE_RAW_LOCK_NESTING report the following
> tp-vsc lockdep error:
> 
> =============================
>  [ BUG: Invalid wait context ]
>  ...
>  swapper/10/0 is trying to lock:
>  ffff88819c271888 (&tp->xfer_wait){....}-{3:3},
>   at: __wake_up (kernel/sched/wait.c:106 kernel/sched/wait.c:127)
>  ...
>  Call Trace:
>  <IRQ>
>  ...
>  __raw_spin_lock_irqsave (./include/linux/spinlock_api_smp.h:111)
>  __wake_up (kernel/sched/wait.c:106 kernel/sched/wait.c:127)
>  vsc_tp_isr (drivers/misc/mei/vsc-tp.c:110) mei_vsc_hw
>  __handle_irq_event_percpu (kernel/irq/handle.c:158)
>  handle_irq_event (kernel/irq/handle.c:195 kernel/irq/handle.c:210)
>  handle_edge_irq (kernel/irq/chip.c:833)
>  ...
>  </IRQ>
> 
> The root-cause of this is the IRQF_NO_THREAD flag used by the intel-pinctrl
> code. Setting IRQF_NO_THREAD requires all interrupt handlers for GPIO ISRs
> to use raw-spinlocks only since normal spinlocks can sleep in PREEMPT-RT
> kernels and with IRQF_NO_THREAD the interrupt handlers will always run in
> an atomic context [1].
> 
> vsc_tp_isr() calls wake_up(&tp->xfer_wait), which uses a regular spinlock,
> breaking the raw-spinlocks only rule for Intel GPIO ISRs.
> 
> Make vsc_tp_isr() run as threaded ISR instead of as hard ISR to fix this.
> 

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Link: https://lore.kernel.org/linux-gpio/18ab52bd-9171-4667-a600-
> 0f52ab7017ac@kernel.org/ [1]
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/vsc-tp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index f5ea38f22419..5ecf99883996 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -497,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, NULL,
> +	ret = request_threaded_irq(spi->irq, NULL, vsc_tp_isr,
>  				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  				   dev_name(dev), tp);
>  	if (ret)
> --
> 2.49.0

RE: [PATCH 10/10] mei: bus: Check for still connected devices in mei_cl_bus_dev_release()

[Usyskin, Alexander]

> Subject: [PATCH 10/10] mei: bus: Check for still connected devices in
> mei_cl_bus_dev_release()
> 
> mei_cl_bus_dev_release() also frees the mei-client (struct mei_cl)
> belonging to the device being released.
> 
> If there are bugs like the just fixed bug in the ACE/CSI2 mei drivers,
> the mei-client being freed might still be part of the mei_device's
> file_list and iterating over this list after the freeing will then trigger
> a use-afer-free bug.
> 
> Add a check to mei_cl_bus_dev_release() to make sure that the to-be-freed
> mei-client is not on the mei_device's file_list.
> 

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/misc/mei/bus.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
> index 67176caf5416..1958c043ac14 100644
> --- a/drivers/misc/mei/bus.c
> +++ b/drivers/misc/mei/bus.c
> @@ -1301,10 +1301,16 @@ static void mei_dev_bus_put(struct mei_device
> *bus)
>  static void mei_cl_bus_dev_release(struct device *dev)
>  {
>  	struct mei_cl_device *cldev = to_mei_cl_device(dev);
> +	struct mei_device *mdev = cldev->cl->dev;
> +	struct mei_cl *cl;
> 
>  	mei_cl_flush_queues(cldev->cl, NULL);
>  	mei_me_cl_put(cldev->me_cl);
>  	mei_dev_bus_put(cldev->bus);
> +
> +	list_for_each_entry(cl, &mdev->file_list, link)
> +		WARN_ON(cl == cldev->cl);
> +
>  	kfree(cldev->cl);
>  	kfree(cldev);
>  }
> --
> 2.49.0