2024-04-03 04:53:00

by Wu, Wentong

[permalink] [raw]
Subject: [PATCH v2 1/2] Revert "mei: vsc: Call wake_up() in the threaded IRQ handler"

From: Sakari Ailus <[email protected]>

This reverts commit 058a38acba15fd8e7b262ec6e17c4204cb15f984.

It's not necessary to avoid a spinlock, a sleeping lock on PREEMPT_RT, in
an interrupt handler as the interrupt handler itself would be called in a
process context if PREEMPT_RT is enabled. So revert the patch.

Cc: [email protected] # for 6.8
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/misc/mei/vsc-tp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index ecfb70cd057c..968a92a7425d 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -419,6 +419,8 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)

atomic_inc(&tp->assert_cnt);

+ wake_up(&tp->xfer_wait);
+
return IRQ_WAKE_THREAD;
}

@@ -426,8 +428,6 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
{
struct vsc_tp *tp = data;

- wake_up(&tp->xfer_wait);
-
if (tp->event_notify)
tp->event_notify(tp->event_notify_context);

--
2.34.1



2024-04-03 04:53:16

by Wu, Wentong

[permalink] [raw]
Subject: [PATCH v2 2/2] mei: vsc: Unregister interrupt handler for system suspend

From: Sakari Ailus <[email protected]>

Unregister the MEI VSC interrupt handler before system suspend and
re-register it at system resume time. This mirrors implementation of other
MEI devices.

This patch fixes the bug that causes continuous stream of MEI VSC errors
after system resume.

Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
Cc: [email protected] # for 6.8
Reported-by: Dominik Brodowski <[email protected]>
Signed-off-by: Wentong Wu <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/misc/mei/platform-vsc.c | 17 ++++++-
drivers/misc/mei/vsc-tp.c | 84 +++++++++++++++++++++++----------
drivers/misc/mei/vsc-tp.h | 3 ++
3 files changed, 78 insertions(+), 26 deletions(-)

diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
index 6c9f00bcb94b..b543e6b9f3cf 100644
--- a/drivers/misc/mei/platform-vsc.c
+++ b/drivers/misc/mei/platform-vsc.c
@@ -400,25 +400,40 @@ static void mei_vsc_remove(struct platform_device *pdev)
static int mei_vsc_suspend(struct device *dev)
{
struct mei_device *mei_dev = dev_get_drvdata(dev);
+ struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);

mei_stop(mei_dev);

+ mei_disable_interrupts(mei_dev);
+
+ vsc_tp_free_irq(hw->tp);
+
return 0;
}

static int mei_vsc_resume(struct device *dev)
{
struct mei_device *mei_dev = dev_get_drvdata(dev);
+ struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
int ret;

- ret = mei_restart(mei_dev);
+ ret = vsc_tp_request_irq(hw->tp);
if (ret)
return ret;

+ ret = mei_restart(mei_dev);
+ if (ret)
+ goto err_free;
+
/* start timer if stopped in suspend */
schedule_delayed_work(&mei_dev->timer_work, HZ);

return 0;
+
+err_free:
+ vsc_tp_free_irq(hw->tp);
+
+ return ret;
}

static DEFINE_SIMPLE_DEV_PM_OPS(mei_vsc_pm_ops, mei_vsc_suspend, mei_vsc_resume);
diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 968a92a7425d..e6a98dba8a73 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -94,6 +94,27 @@ static const struct acpi_gpio_mapping vsc_tp_acpi_gpios[] = {
{}
};

+static irqreturn_t vsc_tp_isr(int irq, void *data)
+{
+ struct vsc_tp *tp = data;
+
+ atomic_inc(&tp->assert_cnt);
+
+ wake_up(&tp->xfer_wait);
+
+ return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
+{
+ struct vsc_tp *tp = data;
+
+ if (tp->event_notify)
+ tp->event_notify(tp->event_notify_context);
+
+ return IRQ_HANDLED;
+}
+
/* wakeup firmware and wait for response */
static int vsc_tp_wakeup_request(struct vsc_tp *tp)
{
@@ -383,6 +404,37 @@ 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
@@ -413,27 +465,6 @@ void vsc_tp_intr_disable(struct vsc_tp *tp)
}
EXPORT_SYMBOL_NS_GPL(vsc_tp_intr_disable, VSC_TP);

-static irqreturn_t vsc_tp_isr(int irq, void *data)
-{
- struct vsc_tp *tp = data;
-
- atomic_inc(&tp->assert_cnt);
-
- wake_up(&tp->xfer_wait);
-
- return IRQ_WAKE_THREAD;
-}
-
-static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
-{
- struct vsc_tp *tp = data;
-
- if (tp->event_notify)
- tp->event_notify(tp->event_notify_context);
-
- return IRQ_HANDLED;
-}
-
static int vsc_tp_match_any(struct acpi_device *adev, void *data)
{
struct acpi_device **__adev = data;
@@ -490,10 +521,9 @@ static int vsc_tp_probe(struct spi_device *spi)
tp->spi = spi;

irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
- ret = devm_request_threaded_irq(dev, spi->irq, vsc_tp_isr,
- vsc_tp_thread_isr,
- IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
- dev_name(dev), tp);
+ 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;

@@ -522,6 +552,8 @@ static int vsc_tp_probe(struct spi_device *spi)
err_destroy_lock:
mutex_destroy(&tp->mutex);

+ free_irq(spi->irq, tp);
+
return ret;
}

@@ -532,6 +564,8 @@ static void vsc_tp_remove(struct spi_device *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[] = {
diff --git a/drivers/misc/mei/vsc-tp.h b/drivers/misc/mei/vsc-tp.h
index f9513ddc3e40..14ca195cbddc 100644
--- a/drivers/misc/mei/vsc-tp.h
+++ b/drivers/misc/mei/vsc-tp.h
@@ -37,6 +37,9 @@ 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.34.1


2024-04-03 12:01:11

by Tomas Winkler

[permalink] [raw]
Subject: RE: [PATCH v2 1/2] Revert "mei: vsc: Call wake_up() in the threaded IRQ handler"

> handler"
>
> From: Sakari Ailus <[email protected]>
>
> This reverts commit 058a38acba15fd8e7b262ec6e17c4204cb15f984.
>
> It's not necessary to avoid a spinlock, a sleeping lock on PREEMPT_RT, in an
> interrupt handler as the interrupt handler itself would be called in a process
> context if PREEMPT_RT is enabled. So revert the patch.
>
> Cc: [email protected] # for 6.8
> Signed-off-by: Sakari Ailus <[email protected]>
Acked-by: Tomas Winkler <[email protected]>
> ---
> drivers/misc/mei/vsc-tp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> ecfb70cd057c..968a92a7425d 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -419,6 +419,8 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
>
> atomic_inc(&tp->assert_cnt);
>
> + wake_up(&tp->xfer_wait);
> +
> return IRQ_WAKE_THREAD;
> }
>
> @@ -426,8 +428,6 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> *data) {
> struct vsc_tp *tp = data;
>
> - wake_up(&tp->xfer_wait);
> -
> if (tp->event_notify)
> tp->event_notify(tp->event_notify_context);
>
> --
> 2.34.1


2024-04-03 12:01:27

by Tomas Winkler

[permalink] [raw]
Subject: RE: [PATCH v2 2/2] mei: vsc: Unregister interrupt handler for system suspend


>
> From: Sakari Ailus <[email protected]>
>
> Unregister the MEI VSC interrupt handler before system suspend and re-
> register it at system resume time. This mirrors implementation of other MEI
> devices.
>
> This patch fixes the bug that causes continuous stream of MEI VSC errors
> after system resume.
>
> Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
> Cc: [email protected] # for 6.8
> Reported-by: Dominik Brodowski <[email protected]>
> Signed-off-by: Wentong Wu <[email protected]>
> Signed-off-by: Sakari Ailus <[email protected]>
Acked-by: Tomas Winkler <[email protected]>

> ---
> drivers/misc/mei/platform-vsc.c | 17 ++++++-
> drivers/misc/mei/vsc-tp.c | 84 +++++++++++++++++++++++----------
> drivers/misc/mei/vsc-tp.h | 3 ++
> 3 files changed, 78 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-
> vsc.c index 6c9f00bcb94b..b543e6b9f3cf 100644
> --- a/drivers/misc/mei/platform-vsc.c
> +++ b/drivers/misc/mei/platform-vsc.c
> @@ -400,25 +400,40 @@ static void mei_vsc_remove(struct platform_device
> *pdev) static int mei_vsc_suspend(struct device *dev) {
> struct mei_device *mei_dev = dev_get_drvdata(dev);
> + struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
>
> mei_stop(mei_dev);
>
> + mei_disable_interrupts(mei_dev);
> +
> + vsc_tp_free_irq(hw->tp);
> +
> return 0;
> }
>
> static int mei_vsc_resume(struct device *dev) {
> struct mei_device *mei_dev = dev_get_drvdata(dev);
> + struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
> int ret;
>
> - ret = mei_restart(mei_dev);
> + ret = vsc_tp_request_irq(hw->tp);
> if (ret)
> return ret;
>
> + ret = mei_restart(mei_dev);
> + if (ret)
> + goto err_free;
> +
> /* start timer if stopped in suspend */
> schedule_delayed_work(&mei_dev->timer_work, HZ);
>
> return 0;
> +
> +err_free:
> + vsc_tp_free_irq(hw->tp);
> +
> + return ret;
> }
>
> static DEFINE_SIMPLE_DEV_PM_OPS(mei_vsc_pm_ops, mei_vsc_suspend,
> mei_vsc_resume); diff --git a/drivers/misc/mei/vsc-tp.c
> b/drivers/misc/mei/vsc-tp.c index 968a92a7425d..e6a98dba8a73 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -94,6 +94,27 @@ static const struct acpi_gpio_mapping
> vsc_tp_acpi_gpios[] = {
> {}
> };
>
> +static irqreturn_t vsc_tp_isr(int irq, void *data) {
> + struct vsc_tp *tp = data;
> +
> + atomic_inc(&tp->assert_cnt);
> +
> + wake_up(&tp->xfer_wait);
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t vsc_tp_thread_isr(int irq, void *data) {
> + struct vsc_tp *tp = data;
> +
> + if (tp->event_notify)
> + tp->event_notify(tp->event_notify_context);
> +
> + return IRQ_HANDLED;
> +}
> +
> /* wakeup firmware and wait for response */ static int
> vsc_tp_wakeup_request(struct vsc_tp *tp) { @@ -383,6 +404,37 @@ 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
> @@ -413,27 +465,6 @@ void vsc_tp_intr_disable(struct vsc_tp *tp) }
> EXPORT_SYMBOL_NS_GPL(vsc_tp_intr_disable, VSC_TP);
>
> -static irqreturn_t vsc_tp_isr(int irq, void *data) -{
> - struct vsc_tp *tp = data;
> -
> - atomic_inc(&tp->assert_cnt);
> -
> - wake_up(&tp->xfer_wait);
> -
> - return IRQ_WAKE_THREAD;
> -}
> -
> -static irqreturn_t vsc_tp_thread_isr(int irq, void *data) -{
> - struct vsc_tp *tp = data;
> -
> - if (tp->event_notify)
> - tp->event_notify(tp->event_notify_context);
> -
> - return IRQ_HANDLED;
> -}
> -
> static int vsc_tp_match_any(struct acpi_device *adev, void *data) {
> struct acpi_device **__adev = data;
> @@ -490,10 +521,9 @@ static int vsc_tp_probe(struct spi_device *spi)
> tp->spi = spi;
>
> irq_set_status_flags(spi->irq, IRQ_DISABLE_UNLAZY);
> - ret = devm_request_threaded_irq(dev, spi->irq, vsc_tp_isr,
> - vsc_tp_thread_isr,
> - IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> - dev_name(dev), tp);
> + 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;
>
> @@ -522,6 +552,8 @@ static int vsc_tp_probe(struct spi_device *spi)
> err_destroy_lock:
> mutex_destroy(&tp->mutex);
>
> + free_irq(spi->irq, tp);
> +
> return ret;
> }
>
> @@ -532,6 +564,8 @@ static void vsc_tp_remove(struct spi_device *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[] = { diff --git
> a/drivers/misc/mei/vsc-tp.h b/drivers/misc/mei/vsc-tp.h index
> f9513ddc3e40..14ca195cbddc 100644
> --- a/drivers/misc/mei/vsc-tp.h
> +++ b/drivers/misc/mei/vsc-tp.h
> @@ -37,6 +37,9 @@ 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.34.1