| 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
| ||
|
Message-ID: <3c62fec1-9df0-fe5b-484a-a1da533a25a1@intel.com>
Date: Thu, 1 Sep 2022 09:00:54 -0700
From: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@...el.com>
To: Tomas Winkler <tomas.winkler@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>
CC: <intel-gfx@...ts.freedesktop.org>,
Alexander Usyskin <alexander.usyskin@...el.com>,
<linux-kernel@...r.kernel.org>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Vitaly Lubart <vitaly.lubart@...el.com>
Subject: Re: [Intel-gfx] [PATCH v7 06/15] mei: gsc: use polling instead of
interrupts
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> A work-around for a HW issue in XEHPSDV that manifests itself when SW reads
> a gsc register when gsc is sending an interrupt. The work-around is
> to disable interrupts and to use polling instead.
>
> Cc: James Ausmus <james.ausmus@...el.com>
> Signed-off-by: Vitaly Lubart <vitaly.lubart@...el.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@...el.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> ---
> drivers/misc/mei/gsc-me.c | 48 ++++++++++++++++++++++++++------
> drivers/misc/mei/hw-me.c | 58 ++++++++++++++++++++++++++++++++++++---
> drivers/misc/mei/hw-me.h | 12 ++++++++
> 3 files changed, 105 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> index c8145e9b62b6..2caba3a9ac35 100644
> --- a/drivers/misc/mei/gsc-me.c
> +++ b/drivers/misc/mei/gsc-me.c
> @@ -13,6 +13,7 @@
> #include <linux/ktime.h>
> #include <linux/delay.h>
> #include <linux/pm_runtime.h>
> +#include <linux/kthread.h>
>
> #include "mei_dev.h"
> #include "hw-me.h"
> @@ -66,13 +67,28 @@ static int mei_gsc_probe(struct auxiliary_device *aux_dev,
>
> dev_set_drvdata(device, dev);
>
> - ret = devm_request_threaded_irq(device, hw->irq,
> - mei_me_irq_quick_handler,
> - mei_me_irq_thread_handler,
> - IRQF_ONESHOT, KBUILD_MODNAME, dev);
> - if (ret) {
> - dev_err(device, "irq register failed %d\n", ret);
> - goto err;
> + /* use polling */
> + if (mei_me_hw_use_polling(hw)) {
> + mei_disable_interrupts(dev);
> + mei_clear_interrupts(dev);
> + init_waitqueue_head(&hw->wait_active);
> + hw->is_active = true; /* start in active mode for initialization */
> + hw->polling_thread = kthread_run(mei_me_polling_thread, dev,
> + "kmegscirqd/%s", dev_name(device));
> + if (IS_ERR(hw->polling_thread)) {
> + ret = PTR_ERR(hw->polling_thread);
> + dev_err(device, "unable to create kernel thread: %d\n", ret);
> + goto err;
> + }
> + } else {
> + ret = devm_request_threaded_irq(device, hw->irq,
> + mei_me_irq_quick_handler,
> + mei_me_irq_thread_handler,
> + IRQF_ONESHOT, KBUILD_MODNAME, dev);
> + if (ret) {
> + dev_err(device, "irq register failed %d\n", ret);
> + goto err;
> + }
> }
>
> pm_runtime_get_noresume(device);
> @@ -98,7 +114,8 @@ static int mei_gsc_probe(struct auxiliary_device *aux_dev,
>
> register_err:
> mei_stop(dev);
> - devm_free_irq(device, hw->irq, dev);
> + if (!mei_me_hw_use_polling(hw))
> + devm_free_irq(device, hw->irq, dev);
>
> err:
> dev_err(device, "probe failed: %d\n", ret);
> @@ -119,12 +136,17 @@ static void mei_gsc_remove(struct auxiliary_device *aux_dev)
>
> mei_stop(dev);
>
> + hw = to_me_hw(dev);
> + if (mei_me_hw_use_polling(hw))
> + kthread_stop(hw->polling_thread);
> +
> mei_deregister(dev);
>
> pm_runtime_disable(&aux_dev->dev);
>
> mei_disable_interrupts(dev);
> - devm_free_irq(&aux_dev->dev, hw->irq, dev);
> + if (!mei_me_hw_use_polling(hw))
> + devm_free_irq(&aux_dev->dev, hw->irq, dev);
> }
>
> static int __maybe_unused mei_gsc_pm_suspend(struct device *device)
> @@ -185,6 +207,9 @@ static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device *device)
> if (mei_write_is_idle(dev)) {
> hw = to_me_hw(dev);
> hw->pg_state = MEI_PG_ON;
> +
> + if (mei_me_hw_use_polling(hw))
> + hw->is_active = false;
> ret = 0;
> } else {
> ret = -EAGAIN;
> @@ -209,6 +234,11 @@ static int __maybe_unused mei_gsc_pm_runtime_resume(struct device *device)
> hw = to_me_hw(dev);
> hw->pg_state = MEI_PG_OFF;
>
> + if (mei_me_hw_use_polling(hw)) {
> + hw->is_active = true;
> + wake_up(&hw->wait_active);
> + }
> +
> mutex_unlock(&dev->device_lock);
>
> irq_ret = mei_me_irq_thread_handler(1, dev);
> diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
> index befa491e3344..46559517a902 100644
> --- a/drivers/misc/mei/hw-me.c
> +++ b/drivers/misc/mei/hw-me.c
> @@ -10,6 +10,7 @@
> #include <linux/interrupt.h>
> #include <linux/pm_runtime.h>
> #include <linux/sizes.h>
> +#include <linux/delay.h>
>
> #include "mei_dev.h"
> #include "hbm.h"
> @@ -327,9 +328,12 @@ static void mei_me_intr_clear(struct mei_device *dev)
> */
> static void mei_me_intr_enable(struct mei_device *dev)
> {
> - u32 hcsr = mei_hcsr_read(dev);
> + u32 hcsr;
> +
> + if (mei_me_hw_use_polling(to_me_hw(dev)))
> + return;
>
> - hcsr |= H_CSR_IE_MASK;
> + hcsr = mei_hcsr_read(dev) | H_CSR_IE_MASK;
> mei_hcsr_set(dev, hcsr);
> }
>
> @@ -354,6 +358,9 @@ static void mei_me_synchronize_irq(struct mei_device *dev)
> {
> struct mei_me_hw *hw = to_me_hw(dev);
>
> + if (mei_me_hw_use_polling(hw))
> + return;
> +
> synchronize_irq(hw->irq);
> }
>
> @@ -380,7 +387,10 @@ static void mei_me_host_set_ready(struct mei_device *dev)
> {
> u32 hcsr = mei_hcsr_read(dev);
>
> - hcsr |= H_CSR_IE_MASK | H_IG | H_RDY;
> + if (!mei_me_hw_use_polling(to_me_hw(dev)))
> + hcsr |= H_CSR_IE_MASK;
> +
> + hcsr |= H_IG | H_RDY;
> mei_hcsr_set(dev, hcsr);
> }
>
> @@ -1176,7 +1186,7 @@ static int mei_me_hw_reset(struct mei_device *dev, bool intr_enable)
>
> hcsr |= H_RST | H_IG | H_CSR_IS_MASK;
>
> - if (!intr_enable)
> + if (!intr_enable || mei_me_hw_use_polling(to_me_hw(dev)))
> hcsr &= ~H_CSR_IE_MASK;
>
> dev->recvd_hw_ready = false;
> @@ -1331,6 +1341,46 @@ irqreturn_t mei_me_irq_thread_handler(int irq, void *dev_id)
> }
> EXPORT_SYMBOL_GPL(mei_me_irq_thread_handler);
>
> +#define MEI_POLLING_TIMEOUT_ACTIVE 100
> +#define MEI_POLLING_TIMEOUT_IDLE 500
> +
> +int mei_me_polling_thread(void *_dev)
> +{
> + struct mei_device *dev = _dev;
> + irqreturn_t irq_ret;
> + long polling_timeout = MEI_POLLING_TIMEOUT_ACTIVE;
> +
> + dev_dbg(dev->dev, "kernel thread is running\n");
> + while (!kthread_should_stop()) {
> + struct mei_me_hw *hw = to_me_hw(dev);
> + u32 hcsr;
> +
> + wait_event_timeout(hw->wait_active,
> + hw->is_active || kthread_should_stop(),
> + msecs_to_jiffies(MEI_POLLING_TIMEOUT_IDLE));
> +
> + if (kthread_should_stop())
> + break;
> +
> + hcsr = mei_hcsr_read(dev);
> + if (me_intr_src(hcsr)) {
> + polling_timeout = MEI_POLLING_TIMEOUT_ACTIVE;
> + irq_ret = mei_me_irq_thread_handler(1, dev);
> + if (irq_ret != IRQ_HANDLED)
> + dev_err(dev->dev, "irq_ret %d\n", irq_ret);
> + } else {
> + polling_timeout = clamp_val(polling_timeout + MEI_POLLING_TIMEOUT_ACTIVE,
> + MEI_POLLING_TIMEOUT_ACTIVE,
> + MEI_POLLING_TIMEOUT_IDLE);
> + }
> +
> + schedule_timeout_interruptible(msecs_to_jiffies(polling_timeout));
> + }
IMO this loop could have used a couple of comments to make it easier to
understand what's going on with the various waits and timeouts. Not a
blocker.
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@...el.com>
Daniele
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mei_me_polling_thread);
> +
> static const struct mei_hw_ops mei_me_hw_ops = {
>
> .trc_status = mei_me_trc_status,
> diff --git a/drivers/misc/mei/hw-me.h b/drivers/misc/mei/hw-me.h
> index a071c645e905..ca09274ac299 100644
> --- a/drivers/misc/mei/hw-me.h
> +++ b/drivers/misc/mei/hw-me.h
> @@ -51,6 +51,8 @@ struct mei_cfg {
> * @d0i3_supported: di03 support
> * @hbuf_depth: depth of hardware host/write buffer in slots
> * @read_fws: read FW status register handler
> + * @wait_active: the polling thread activity wait queue
> + * @is_active: the device is active
> */
> struct mei_me_hw {
> const struct mei_cfg *cfg;
> @@ -60,10 +62,19 @@ struct mei_me_hw {
> bool d0i3_supported;
> u8 hbuf_depth;
> int (*read_fws)(const struct mei_device *dev, int where, u32 *val);
> + /* polling */
> + struct task_struct *polling_thread;
> + wait_queue_head_t wait_active;
> + bool is_active;
> };
>
> #define to_me_hw(dev) (struct mei_me_hw *)((dev)->hw)
>
> +static inline bool mei_me_hw_use_polling(const struct mei_me_hw *hw)
> +{
> + return hw->irq < 0;
> +}
> +
> /**
> * enum mei_cfg_idx - indices to platform specific configurations.
> *
> @@ -127,5 +138,6 @@ int mei_me_pg_exit_sync(struct mei_device *dev);
>
> irqreturn_t mei_me_irq_quick_handler(int irq, void *dev_id);
> irqreturn_t mei_me_irq_thread_handler(int irq, void *dev_id);
> +int mei_me_polling_thread(void *_dev);
>
> #endif /* _MEI_INTERFACE_H_ */
Powered by blists - more mailing lists