[PATCH v2 0/3] MEI VSC fixes and cleanups

[PATCH v2 0/3] MEI VSC fixes and cleanups

[Sakari Ailus]

Hi folks,

These patches address sleeping in atomic context. It wasn't obvious at
first this was taking place since the callback sleeps while the caller (a
different driver) called it in a threaded IRQ handler.

since v1:

- Rework patch 1. Moving wake_up() to the threaded handler is enough as
  sleeping is allowed in the threaded handler.

Sakari Ailus (3):
  mei: vsc: Call wake_up() in the threaded IRQ handler
  mei: vsc: Don't use sleeping condition in wait_event_timeout()
  mei: vsc: Assign pinfo fields in variable declaration

 drivers/misc/mei/vsc-tp.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ handler

[Sakari Ailus]

The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT. This leads
to sleeping in atomic context.

Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
where it can be safely called.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 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 6f4a4be6ccb5..2b69ada9349e 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
 
 	atomic_inc(&tp->assert_cnt);
 
-	wake_up(&tp->xfer_wait);
-
 	return IRQ_WAKE_THREAD;
 }
 
@@ -425,6 +423,8 @@ 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.39.2

[PATCH v2 2/3] mei: vsc: Don't use sleeping condition in wait_event_timeout()

[Sakari Ailus]

vsc_tp_wakeup_request() called wait_event_timeout() with
gpiod_get_value_cansleep() which may sleep, and does so as the
implementation is that of gpio-ljca.

Move the GPIO state check outside the call.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/mei/vsc-tp.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 2b69ada9349e..7b678005652b 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -25,7 +25,8 @@
 #define VSC_TP_ROM_BOOTUP_DELAY_MS		10
 #define VSC_TP_ROM_XFER_POLL_TIMEOUT_US		(500 * USEC_PER_MSEC)
 #define VSC_TP_ROM_XFER_POLL_DELAY_US		(20 * USEC_PER_MSEC)
-#define VSC_TP_WAIT_FW_ASSERTED_TIMEOUT		(2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_TIMEOUT		(2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_DELAY_US		(20 * USEC_PER_MSEC)
 #define VSC_TP_MAX_XFER_COUNT			5
 
 #define VSC_TP_PACKET_SYNC			0x31
@@ -101,13 +102,15 @@ static int vsc_tp_wakeup_request(struct vsc_tp *tp)
 	gpiod_set_value_cansleep(tp->wakeupfw, 0);
 
 	ret = wait_event_timeout(tp->xfer_wait,
-				 atomic_read(&tp->assert_cnt) &&
-				 gpiod_get_value_cansleep(tp->wakeuphost),
-				 VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
+				 atomic_read(&tp->assert_cnt),
+				 VSC_TP_WAIT_FW_POLL_TIMEOUT);
 	if (!ret)
 		return -ETIMEDOUT;
 
-	return 0;
+	return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
+				 VSC_TP_WAIT_FW_POLL_DELAY_US,
+				 VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
+				 tp->wakeuphost);
 }
 
 static void vsc_tp_wakeup_release(struct vsc_tp *tp)
-- 
2.39.2

[PATCH v2 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Sakari Ailus]

Assign all possible fields of pinfo in variable declaration, instead of
just zeroing it there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 7b678005652b..9b4584d67a1b 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -445,11 +445,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
 
 static int vsc_tp_probe(struct spi_device *spi)
 {
-	struct platform_device_info pinfo = { 0 };
+	struct vsc_tp *tp;
+	struct platform_device_info pinfo = {
+		.name = "intel_vsc",
+		.data = &tp,
+		.size_data = sizeof(tp),
+		.id = PLATFORM_DEVID_NONE,
+	};
 	struct device *dev = &spi->dev;
 	struct platform_device *pdev;
 	struct acpi_device *adev;
-	struct vsc_tp *tp;
 	int ret;
 
 	tp = devm_kzalloc(dev, sizeof(*tp), GFP_KERNEL);
@@ -501,13 +506,8 @@ static int vsc_tp_probe(struct spi_device *spi)
 		ret = -ENODEV;
 		goto err_destroy_lock;
 	}
-	pinfo.fwnode = acpi_fwnode_handle(adev);
-
-	pinfo.name = "intel_vsc";
-	pinfo.data = &tp;
-	pinfo.size_data = sizeof(tp);
-	pinfo.id = PLATFORM_DEVID_NONE;
 
+	pinfo.fwnode = acpi_fwnode_handle(adev);
 	pdev = platform_device_register_full(&pinfo);
 	if (IS_ERR(pdev)) {
 		ret = PTR_ERR(pdev);
-- 
2.39.2

RE: [PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ handler

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.

Does this mean we can't use wake_up() in isr? 

BR,
Wentong

> This leads to sleeping in atomic context.
> 
> Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> where it can be safely called.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  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
> 6f4a4be6ccb5..2b69ada9349e 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> 
>  	atomic_inc(&tp->assert_cnt);
> 
> -	wake_up(&tp->xfer_wait);
> -
>  	return IRQ_WAKE_THREAD;
>  }
> 
> @@ -425,6 +423,8 @@ 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.39.2

RE: [PATCH v2 2/3] mei: vsc: Don't use sleeping condition in wait_event_timeout()

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> vsc_tp_wakeup_request() called wait_event_timeout() with
> gpiod_get_value_cansleep() which may sleep, and does so as the
> implementation is that of gpio-ljca.
> 
> Move the GPIO state check outside the call.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Tested-and-Reviewed-by: Wentong Wu <wentong.wu@intel.com>

> ---
>  drivers/misc/mei/vsc-tp.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 2b69ada9349e..7b678005652b 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -25,7 +25,8 @@
>  #define VSC_TP_ROM_BOOTUP_DELAY_MS		10
>  #define VSC_TP_ROM_XFER_POLL_TIMEOUT_US		(500 *
> USEC_PER_MSEC)
>  #define VSC_TP_ROM_XFER_POLL_DELAY_US		(20 *
> USEC_PER_MSEC)
> -#define VSC_TP_WAIT_FW_ASSERTED_TIMEOUT		(2 * HZ)
> +#define VSC_TP_WAIT_FW_POLL_TIMEOUT		(2 * HZ)
> +#define VSC_TP_WAIT_FW_POLL_DELAY_US		(20 *
> USEC_PER_MSEC)
>  #define VSC_TP_MAX_XFER_COUNT			5
> 
>  #define VSC_TP_PACKET_SYNC			0x31
> @@ -101,13 +102,15 @@ static int vsc_tp_wakeup_request(struct vsc_tp *tp)
>  	gpiod_set_value_cansleep(tp->wakeupfw, 0);
> 
>  	ret = wait_event_timeout(tp->xfer_wait,
> -				 atomic_read(&tp->assert_cnt) &&
> -				 gpiod_get_value_cansleep(tp->wakeuphost),
> -				 VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
> +				 atomic_read(&tp->assert_cnt),
> +				 VSC_TP_WAIT_FW_POLL_TIMEOUT);
>  	if (!ret)
>  		return -ETIMEDOUT;
> 
> -	return 0;
> +	return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
> +				 VSC_TP_WAIT_FW_POLL_DELAY_US,
> +				 VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
> +				 tp->wakeuphost);
>  }
> 
>  static void vsc_tp_wakeup_release(struct vsc_tp *tp)
> --
> 2.39.2

RE: [PATCH v2 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Assign all possible fields of pinfo in variable declaration, instead of just
> zeroing it there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Tested-and-Reviewed-by: Wentong Wu <wentong.wu@intel.com>

> ---
>  drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 7b678005652b..9b4584d67a1b 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -445,11 +445,16 @@ static int vsc_tp_match_any(struct acpi_device
> *adev, void *data)
> 
>  static int vsc_tp_probe(struct spi_device *spi)  {
> -	struct platform_device_info pinfo = { 0 };
> +	struct vsc_tp *tp;
> +	struct platform_device_info pinfo = {
> +		.name = "intel_vsc",
> +		.data = &tp,
> +		.size_data = sizeof(tp),
> +		.id = PLATFORM_DEVID_NONE,
> +	};
>  	struct device *dev = &spi->dev;
>  	struct platform_device *pdev;
>  	struct acpi_device *adev;
> -	struct vsc_tp *tp;
>  	int ret;
> 
>  	tp = devm_kzalloc(dev, sizeof(*tp), GFP_KERNEL); @@ -501,13 +506,8
> @@ static int vsc_tp_probe(struct spi_device *spi)
>  		ret = -ENODEV;
>  		goto err_destroy_lock;
>  	}
> -	pinfo.fwnode = acpi_fwnode_handle(adev);
> -
> -	pinfo.name = "intel_vsc";
> -	pinfo.data = &tp;
> -	pinfo.size_data = sizeof(tp);
> -	pinfo.id = PLATFORM_DEVID_NONE;
> 
> +	pinfo.fwnode = acpi_fwnode_handle(adev);
>  	pdev = platform_device_register_full(&pinfo);
>  	if (IS_ERR(pdev)) {
>  		ret = PTR_ERR(pdev);
> --
> 2.39.2

Re: [PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ handler

[Sakari Ailus]

Hi Wentong,

On Thu, Feb 22, 2024 at 03:26:18AM +0000, Wu, Wentong wrote:
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.
> 
> Does this mean we can't use wake_up() in isr? 

Good question. A lot of callers currently do.

In this case, handle_edge_irq() takes the raw spinlock and acquiring the
wake queue spinlock in wake_up() leads to sleeping IRQs disabled (see
below).

I don't think there's any harm in moving the wake_up() to the threaded
handler.

-------8<---------------------------
[   54.216989] =============================
[   54.218458] [ BUG: Invalid wait context ]
[   54.219913] 6.8.0-rc2+ #1972 Not tainted
[   54.221493] -----------------------------
[   54.223165] swapper/4/0 is trying to lock:
[   54.224756] ffff888112d04688 (&tp->xfer_wait){....}-{3:3}, at: __wake_up_common_lock+0x25/0x60
[   54.226426] other info that might help us debug this:
[   54.228189] context-{2:2}
[   54.229817] no locks held by swapper/4/0.
[   54.231453] stack backtrace:
[   54.233078] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 6.8.0-rc2+ #1972
[   54.234720] Hardware name: Dell Inc. XPS 9315/0WWXF6, BIOS 1.3.0 08/16/2022
[   54.236361] Call Trace:
[   54.237983]  <IRQ>
[   54.239594]  dump_stack_lvl+0x6a/0x9f
[   54.241147]  __lock_acquire+0x43e/0x11fb
[   54.242704]  ? mark_lock+0x96/0x34d
[   54.244212]  ? check_chain_key+0xe5/0x132
[   54.245759]  ? __wake_up_common_lock+0x25/0x60
[   54.247312]  lock_acquire+0x128/0x27c
[   54.248848]  ? __wake_up_common_lock+0x25/0x60
[   54.250384]  _raw_spin_lock_irqsave+0x3e/0x51
[   54.251922]  ? __wake_up_common_lock+0x25/0x60
[   54.253524]  __wake_up_common_lock+0x25/0x60
[   54.255049]  vsc_tp_isr+0x1e/0x28 [mei_vsc_hw]
[   54.256569]  __handle_irq_event_percpu+0xb4/0x1aa
[   54.258054]  handle_irq_event_percpu+0xf/0x32
[   54.259550]  handle_irq_event+0x34/0x53
[   54.260982]  handle_edge_irq+0xb0/0xcf
[   54.262440]  handle_irq_desc+0x39/0x43
[   54.263898]  intel_gpio_irq+0x105/0x15a
[   54.265348]  __handle_irq_event_percpu+0xb4/0x1aa
[   54.266792]  handle_irq_event_percpu+0xf/0x32
[   54.268244]  handle_irq_event+0x34/0x53
[   54.269634]  handle_fasteoi_irq+0xa5/0x131
[   54.271018]  __common_interrupt+0xdc/0xeb
[   54.272392]  common_interrupt+0x96/0xc1
[   54.273754]  </IRQ>
[   54.275109]  <TASK>
[   54.276391]  asm_common_interrupt+0x22/0x40
[   54.277733] RIP: 0010:cpuidle_enter_state+0x171/0x253
[   54.279071] Code: 8b 73 04 83 cf ff 49 89 c6 e8 62 fe df ff 31 ff e8 65 29 79 ff 45 84 ff 74 07 31 ff e8 0d c3 7f ff e8 fc 91 85 ff fb 45 85 e4 <0f> 88 ba 00 00 00 48 8b 04 24 49 63 cc 48 6b d1 68 49 29 c6 48 89
[   54.280496] RSP: 0018:ffffc900001a7e88 EFLAGS: 00000202
[   54.281935] RAX: 0000000000000004 RBX: ffffe8ffffa310c0 RCX: 000000000000001f
[   54.283382] RDX: 0000000c9f7cf88f RSI: ffffffff82103e63 RDI: ffffffff8209b592
[   54.284828] RBP: 0000000000000004 R08: 0000000000000000 R09: 0000000000000001
[   54.286274] R10: 0000000001151268 R11: 0000000000000020 R12: 0000000000000004
[   54.287699] R13: ffffffff8266fec0 R14: 0000000c9f7cf88f R15: 0000000000000000
[   54.289127]  ? cpuidle_enter_state+0x16d/0x253
[   54.290564]  cpuidle_enter+0x2a/0x38
[   54.291987]  do_idle+0x190/0x204
[   54.293394]  cpu_startup_entry+0x2a/0x2c
[   54.294797]  start_secondary+0x12d/0x12d
[   54.296198]  secondary_startup_64_no_verify+0x15e/0x16b
[   54.297595]  </TASK>
-------8<---------------------------

> 
> BR,
> Wentong
> 
> > This leads to sleeping in atomic context.
> > 
> > Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> > where it can be safely called.
> > 
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  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
> > 6f4a4be6ccb5..2b69ada9349e 100644
> > --- a/drivers/misc/mei/vsc-tp.c
> > +++ b/drivers/misc/mei/vsc-tp.c
> > @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> > 
> >  	atomic_inc(&tp->assert_cnt);
> > 
> > -	wake_up(&tp->xfer_wait);
> > -
> >  	return IRQ_WAKE_THREAD;
> >  }
> > 
> > @@ -425,6 +423,8 @@ 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);
> > 

-- 
Regards,

Sakari Ailus

RE: [PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ handler

[Wu, Wentong]

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT. This leads to
> sleeping in atomic context.
> 
> Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> where it can be safely called.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Tested-and-Reviewed-by: Wentong Wu <wentong.wu@intel.com>
> ---
>  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
> 6f4a4be6ccb5..2b69ada9349e 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> 
>  	atomic_inc(&tp->assert_cnt);
> 
> -	wake_up(&tp->xfer_wait);
> -
>  	return IRQ_WAKE_THREAD;
>  }
> 
> @@ -425,6 +423,8 @@ 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.39.2

Re: [PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ handler

[Sakari Ailus]

Hi Wentong,

On Wed, Feb 28, 2024 at 08:19:04AM +0000, Wu, Wentong wrote:
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT. This leads to
> > sleeping in atomic context.
> > 
> > Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> > where it can be safely called.
> > 
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Tested-and-Reviewed-by: Wentong Wu <wentong.wu@intel.com>

Thanks!

I dug a little bit deeper and it seems the lockdep warning this patch fixes
is something we can safely ignore, see
<URL:https://wiki.archlinux.org/title/Realtime_kernel_patchset#How_does_the_realtime_patch_work>.
My apologies for the noise.

The two other patches in the set are still unaffected by this.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/3] mei: vsc: Call wake_up() in the threaded IRQ handler

[Arnd Bergmann]

On Thu, Feb 22, 2024, at 12:46, Sakari Ailus wrote:
> Hi Wentong,
>
> On Thu, Feb 22, 2024 at 03:26:18AM +0000, Wu, Wentong wrote:
>> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
>> > 
>> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
>> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.
>> 
>> Does this mean we can't use wake_up() in isr? 
>
> Good question. A lot of callers currently do.

If driver has a traditional (non-threaded) handler, it should
always work fine: on non-PREEMPT_RT it can take the spinlock
and on PREEMPT_RT it automatically turns into a threaded
handler that can still call it.

> In this case, handle_edge_irq() takes the raw spinlock and acquiring the
> wake queue spinlock in wake_up() leads to sleeping IRQs disabled (see
> below).
>
> I don't think there's any harm in moving the wake_up() to the threaded
> handler.

It causes an extra bit of latency for the non-PREEMPT_RT case
because you now always have to go through two task switches.
This is probably fine if you don't care about latency. 

You can probably replace the open-coded completion (wait queue
head plus atomic) with a normal 'struct completion' and
call complete() in the isr instead. This one seems to only
take a raw spinlock already.

    Arnd

Re: [PATCH v2 2/3] mei: vsc: Don't use sleeping condition in wait_event_timeout()

[Andy Shevchenko]

On Mon, Feb 19, 2024 at 09:58:06PM +0200, Sakari Ailus wrote:
> vsc_tp_wakeup_request() called wait_event_timeout() with
> gpiod_get_value_cansleep() which may sleep, and does so as the
> implementation is that of gpio-ljca.
> 
> Move the GPIO state check outside the call.

...

> +#define VSC_TP_WAIT_FW_POLL_TIMEOUT		(2 * HZ)
> +#define VSC_TP_WAIT_FW_POLL_DELAY_US		(20 * USEC_PER_MSEC)

...

>  	ret = wait_event_timeout(tp->xfer_wait,
> -				 atomic_read(&tp->assert_cnt) &&
> -				 gpiod_get_value_cansleep(tp->wakeuphost),
> -				 VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
> +				 atomic_read(&tp->assert_cnt),
> +				 VSC_TP_WAIT_FW_POLL_TIMEOUT);

First of all, there is an API for such cases (wait_woken_up() IIRC).

>  	if (!ret)
>  		return -ETIMEDOUT;

...

> +	return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
> +				 VSC_TP_WAIT_FW_POLL_DELAY_US,
> +				 VSC_TP_WAIT_FW_POLL_TIMEOUT, false,

Second, this is a bug as you are using jiffies as microseconds. How had it been tested?

> +				 tp->wakeuphost);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Andy Shevchenko]

On Mon, Feb 19, 2024 at 09:58:07PM +0200, Sakari Ailus wrote:
> Assign all possible fields of pinfo in variable declaration, instead of
> just zeroing it there.

...

Side note: Can we actually make acpi_dev_has_children() public and use here
instead of open coding?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] mei: vsc: Don't use sleeping condition in wait_event_timeout()

[Arnd Bergmann]

On Tue, Mar 5, 2024, at 18:11, Andy Shevchenko wrote:
> On Mon, Feb 19, 2024 at 09:58:06PM +0200, Sakari Ailus wrote:
>> vsc_tp_wakeup_request() called wait_event_timeout() with
>> gpiod_get_value_cansleep() which may sleep, and does so as the
>> implementation is that of gpio-ljca.
>> 
>> Move the GPIO state check outside the call.
>
> ...
>
>> +#define VSC_TP_WAIT_FW_POLL_TIMEOUT		(2 * HZ)
>> +#define VSC_TP_WAIT_FW_POLL_DELAY_US		(20 * USEC_PER_MSEC)
>
> ...
>
>>  	ret = wait_event_timeout(tp->xfer_wait,
>> -				 atomic_read(&tp->assert_cnt) &&
>> -				 gpiod_get_value_cansleep(tp->wakeuphost),
>> -				 VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
>> +				 atomic_read(&tp->assert_cnt),
>> +				 VSC_TP_WAIT_FW_POLL_TIMEOUT);
>
> First of all, there is an API for such cases (wait_woken_up() IIRC).

I think wait_for_completion_timeout() would be the obvious
replacement if the wait_event is no longer waiting for the
gpio but only for the irq handler to complete.

     Arnd