[PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[d3z-the-dev]

On the Surface Pro 10 (Meteor Lake) the touchscreen stops working after a
suspend/resume cycle and only recovers after a reboot. The driver logs
"GET_DEVICE_INFO: recv failed: -11" on resume.

The touch IC loses power during system suspend (s2idle) on this platform,
the same way it does across hibernation. quickspi_resume() only restores
the THC port, interrupts and DMA and sends a HIDSPI_ON command, assuming
the touch IC kept its power and state. When it has actually lost power the
HIDSPI_ON command is never acknowledged and the descriptor read fails,
leaving the touchscreen dead until the module is reloaded.

quickspi_restore() already handles this for hibernation by running
reset_tic() and reconfiguring the THC SPI/LTR settings. Make
quickspi_resume() do the same: quiesce interrupts, re-select the THC port,
reconfigure the SPI input/output addresses and read/write parameters, run
reset_tic() to re-enumerate the device and restore the LTR configuration.

Tested on a Surface Pro 10 across multiple s2idle suspend/resume cycles.

Link: https://github.com/linux-surface/linux-surface/issues/1799

Signed-off-by: d3z-the-dev <d3z.the.dev@gmail.com>
---
 .../intel-quickspi/pci-quickspi.c             | 38 +++++++++++++++++--
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index f669235f1883..d59278a3e5c1 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
 	if (!qsdev)
 		return -ENODEV;
 
+	ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
+	if (ret)
+		return ret;
+
 	ret = thc_port_select(qsdev->thc_hw, THC_PORT_TYPE_SPI);
 	if (ret)
 		return ret;
 
+	thc_spi_input_output_address_config(qsdev->thc_hw,
+					    qsdev->input_report_hdr_addr,
+					    qsdev->input_report_bdy_addr,
+					    qsdev->output_report_addr);
+
+	ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
+				  qsdev->spi_read_io_mode,
+				  qsdev->spi_read_opcode,
+				  qsdev->spi_packet_size);
+	if (ret)
+		return ret;
+
+	ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
+				   qsdev->spi_write_io_mode,
+				   qsdev->spi_write_opcode,
+				   qsdev->spi_packet_size,
+				   qsdev->performance_limit);
+	if (ret)
+		return ret;
+
 	thc_interrupt_config(qsdev->thc_hw);
 
 	thc_interrupt_enable(qsdev->thc_hw, true);
 
-	ret = thc_dma_configure(qsdev->thc_hw);
+	/* The TIC may lose power across system suspend, reset it to recover */
+	ret = reset_tic(qsdev);
 	if (ret)
 		return ret;
 
-	ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
+	ret = thc_dma_configure(qsdev->thc_hw);
 	if (ret)
 		return ret;
 
-	if (!device_may_wakeup(qsdev->dev))
-		return quickspi_set_power(qsdev, HIDSPI_ON);
+	thc_ltr_config(qsdev->thc_hw,
+		       qsdev->active_ltr_val,
+		       qsdev->low_power_ltr_val);
+
+	thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
+
+	qsdev->state = QUICKSPI_ENABLED;
 
 	return 0;
 }
-- 
2.54.0

Re: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional hardware reset during system resume destroys pending wake events and breaks Wake-on-Touch.

Pre-existing issues:
- [High] Using wait_event_interruptible_timeout in a system resume callback can prematurely abort device resume.
- [High] Concurrent execution of `reset_tic()` from the PM resume callback and the IRQ thread without proper locking.
--

commit e9088cb0484bc3fbc7b5dd49968293dc14d55d36
Author: d3z-the-dev <d3z.the.dev@gmail.com>

HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

This commit modifies quickspi_resume() to perform a full reset of the touch
IC during system resume. It mirrors the hibernation restore path to fix an
issue where the touch IC loses power during s2idle suspend on certain platforms.

Link: https://github.com/linux-surface/linux-surface/issues/1799

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index f669235f18839..d59278a3e5c10 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
>  	thc_interrupt_config(qsdev->thc_hw);
>  
>  	thc_interrupt_enable(qsdev->thc_hw, true);

[Severity: High]
This is a pre-existing issue, but does enabling interrupts right before calling
reset_tic() introduce a race condition?

If the touch controller generates an error interrupt during the reset sequence,
the OS schedules the IRQ thread, which calls try_recover() and executes
reset_tic() again. Since both the PM thread and the IRQ thread execute
reset_tic() concurrently without a mutex:

PM thread
quickspi_resume()
   reset_tic()

IRQ thread
quickspi_irq_thread_handler()
   try_recover()
      reset_tic()

Can this corrupt the device state and cause both threads to wait on the same
reset_ack_wq?

>  
> -	ret = thc_dma_configure(qsdev->thc_hw);
> +	/* The TIC may lose power across system suspend, reset it to recover */
> +	ret = reset_tic(qsdev);

[Severity: High]
This isn't a bug introduced by this patch, but since reset_tic() is now
used in the primary system resume path, is it safe for it to use
wait_event_interruptible_timeout() internally?

System resume callbacks are often executed by a user-space process which
might have a pending signal from the task freeze phase. If
wait_event_interruptible_timeout() aborts immediately and returns
-ERESTARTSYS, won't reset_tic() fail with -ETIMEDOUT and completely abort
the device resume?

>  	if (ret)
>  		return ret;
>  
> -	ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
> +	ret = thc_dma_configure(qsdev->thc_hw);
>  	if (ret)
>  		return ret;
>  
> -	if (!device_may_wakeup(qsdev->dev))
> -		return quickspi_set_power(qsdev, HIDSPI_ON);

[Severity: High]
Does unconditionally calling reset_tic() here break Wake-on-Touch for
platforms that don't lose power?

Previously, this function had a check to avoid disturbing a wake-enabled
device:

-       if (!device_may_wakeup(qsdev->dev))
-               return quickspi_set_power(qsdev, HIDSPI_ON);

Will a hard reset wipe the internal buffers of the touch controller before
the OS can fetch the wake-triggering touch event, resulting in permanently
lost events?

> +	thc_ltr_config(qsdev->thc_hw,
> +		       qsdev->active_ltr_val,
> +		       qsdev->low_power_ltr_val);
> +
> +	thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
> +
> +	qsdev->state = QUICKSPI_ENABLED;
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529222210.1532876-1-d3z.the.dev@gmail.com?part=1

RE: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[Xu, Even]

[-- Attachment #1: Type: text/plain, Size: 4832 bytes --]

Hi, 

Sorry, I cannot get your exact name from this patch email.

From your issue description, I suppose Surface Pro 10 uses "Suspend-to-RAM" instead of "Standby" or "Suspend-to-Idle" for suspend/resume.
As this documents mentioned, driver needs to take care the suspend type then handle resume flow carefully.

Actually, I had a patch for this "Suspend-to-RAM" support several months ago, please find attached file for the patch. This patch is still under our internal final validation.

It will be very appreciated if you can help test attached patch on your Surface device.
If it works, then everything is fine, I will submit this patch with your name after our final validation.
If it doesn't work for your case, then we need to future debug on your surface device to refine your patch to let it more standard.

Thanks!

Best Regards,
Even Xu

> -----Original Message-----
> From: d3z-the-dev <d3z.the.dev@gmail.com>
> Sent: Saturday, May 30, 2026 6:22 AM
> To: Xu, Even <even.xu@intel.com>; Sun, Xinpeng <xinpeng.sun@intel.com>; Jiri
> Kosina <jikos@kernel.org>; Benjamin Tissoires <bentiss@kernel.org>
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Abhishek Tamboli
> <abhishektamboli9@gmail.com>; Sakari Ailus <sakari.ailus@linux.intel.com>;
> d3z-the-dev <d3z.the.dev@gmail.com>
> Subject: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system
> resume
> 
> On the Surface Pro 10 (Meteor Lake) the touchscreen stops working after a
> suspend/resume cycle and only recovers after a reboot. The driver logs
> "GET_DEVICE_INFO: recv failed: -11" on resume.
> 
> The touch IC loses power during system suspend (s2idle) on this platform, the
> same way it does across hibernation. quickspi_resume() only restores the THC
> port, interrupts and DMA and sends a HIDSPI_ON command, assuming the touch
> IC kept its power and state. When it has actually lost power the HIDSPI_ON
> command is never acknowledged and the descriptor read fails, leaving the
> touchscreen dead until the module is reloaded.
> 
> quickspi_restore() already handles this for hibernation by running
> reset_tic() and reconfiguring the THC SPI/LTR settings. Make
> quickspi_resume() do the same: quiesce interrupts, re-select the THC port,
> reconfigure the SPI input/output addresses and read/write parameters, run
> reset_tic() to re-enumerate the device and restore the LTR configuration.
> 
> Tested on a Surface Pro 10 across multiple s2idle suspend/resume cycles.
> 
> Link: https://github.com/linux-surface/linux-surface/issues/1799
> 
> Signed-off-by: d3z-the-dev <d3z.the.dev@gmail.com>
> ---
>  .../intel-quickspi/pci-quickspi.c             | 38 +++++++++++++++++--
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index f669235f1883..d59278a3e5c1 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
>  	if (!qsdev)
>  		return -ENODEV;
> 
> +	ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
> +	if (ret)
> +		return ret;
> +
>  	ret = thc_port_select(qsdev->thc_hw, THC_PORT_TYPE_SPI);
>  	if (ret)
>  		return ret;
> 
> +	thc_spi_input_output_address_config(qsdev->thc_hw,
> +					    qsdev->input_report_hdr_addr,
> +					    qsdev->input_report_bdy_addr,
> +					    qsdev->output_report_addr);
> +
> +	ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
> +				  qsdev->spi_read_io_mode,
> +				  qsdev->spi_read_opcode,
> +				  qsdev->spi_packet_size);
> +	if (ret)
> +		return ret;
> +
> +	ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
> +				   qsdev->spi_write_io_mode,
> +				   qsdev->spi_write_opcode,
> +				   qsdev->spi_packet_size,
> +				   qsdev->performance_limit);
> +	if (ret)
> +		return ret;
> +
>  	thc_interrupt_config(qsdev->thc_hw);
> 
>  	thc_interrupt_enable(qsdev->thc_hw, true);
> 
> -	ret = thc_dma_configure(qsdev->thc_hw);
> +	/* The TIC may lose power across system suspend, reset it to recover */
> +	ret = reset_tic(qsdev);
>  	if (ret)
>  		return ret;
> 
> -	ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
> +	ret = thc_dma_configure(qsdev->thc_hw);
>  	if (ret)
>  		return ret;
> 
> -	if (!device_may_wakeup(qsdev->dev))
> -		return quickspi_set_power(qsdev, HIDSPI_ON);
> +	thc_ltr_config(qsdev->thc_hw,
> +		       qsdev->active_ltr_val,
> +		       qsdev->low_power_ltr_val);
> +
> +	thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
> +
> +	qsdev->state = QUICKSPI_ENABLED;
> 
>  	return 0;
>  }
> --
> 2.54.0


[-- Attachment #2: 0001-Hid-Intel-thc-hid-Intel-quickspi-Fix-non-functional-.patch --]
[-- Type: application/octet-stream, Size: 3337 bytes --]

From 939ec2c491f789b5b740a58936ff0e04cb71d2a8 Mon Sep 17 00:00:00 2001
From: Even Xu <even.xu@intel.com>
Date: Mon, 19 Jan 2026 16:16:12 +0800
Subject: [PATCH] Hid: Intel-thc-hid: Intel-quickspi: Fix non-functional touch
 after S3 resume

THC hardware registers lose their content during S3 suspend. Add hardware
reconfiguration on resume from S3 to restore touch functionality.

Signed-off-by: Even Xu <even.xu@intel.com>
---
 .../intel-quickspi/pci-quickspi.c             | 32 +++++++++++++++++++
 .../intel-quickspi/quickspi-dev.h             |  4 +++
 2 files changed, 36 insertions(+)

diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index 20e8dbf1d313..0d67a0bb2ee7 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -10,6 +10,7 @@
 #include <linux/irqreturn.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
+#include <linux/suspend.h>
 
 #include <linux/gpio/consumer.h>
 
@@ -765,6 +766,8 @@ static int quickspi_suspend(struct device *device)
 
 	thc_dma_unconfigure(qsdev->thc_hw);
 
+	qsdev->last_suspend_state = pm_suspend_target_state;
+
 	return 0;
 }
 
@@ -782,6 +785,35 @@ static int quickspi_resume(struct device *device)
 	if (ret)
 		return ret;
 
+	/* If it's resume from S3, need to re-configure SPI port */
+	if (qsdev->last_suspend_state == PM_SUSPEND_MEM) {
+		thc_spi_input_output_address_config(qsdev->thc_hw,
+						    qsdev->input_report_hdr_addr,
+						    qsdev->input_report_bdy_addr,
+						    qsdev->output_report_addr);
+
+		ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
+					  qsdev->spi_read_io_mode,
+					  qsdev->spi_read_opcode,
+					  qsdev->spi_packet_size);
+		if (ret)
+			return ret;
+
+		ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
+					   qsdev->spi_write_io_mode,
+					   qsdev->spi_write_opcode,
+					   qsdev->spi_packet_size,
+					   qsdev->performance_limit);
+		if (ret)
+			return ret;
+
+		thc_ltr_config(qsdev->thc_hw,
+			       qsdev->active_ltr_val,
+			       qsdev->low_power_ltr_val);
+
+		thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
+	}
+
 	thc_interrupt_config(qsdev->thc_hw);
 
 	thc_interrupt_enable(qsdev->thc_hw, true);
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
index ef433fe1b45d..3beeec62dd09 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
@@ -7,6 +7,7 @@
 #include <linux/bits.h>
 #include <linux/hid-over-spi.h>
 #include <linux/sizes.h>
+#include <linux/suspend.h>
 #include <linux/wait.h>
 
 #include "quickspi-protocol.h"
@@ -124,6 +125,7 @@ struct acpi_device;
  * @get_feature_cmpl: indicate get feature received or not
  * @set_feature_cmpl_wq: workqueue for waiting set feature to device
  * @set_feature_cmpl: indicate set feature send complete or not
+ * @last_suspend_state: save last system suspend state
  */
 struct quickspi_device {
 	struct device *dev;
@@ -171,6 +173,8 @@ struct quickspi_device {
 
 	wait_queue_head_t set_report_cmpl_wq;
 	bool set_report_cmpl;
+
+	suspend_state_t last_suspend_state;
 };
 
 #endif /* _QUICKSPI_DEV_H_ */
-- 
2.43.0

RE: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[Xu, Even]

Sorry, missed the doc link: https://docs.kernel.org/admin-guide/pm/sleep-states.html

> -----Original Message-----
> From: Xu, Even
> Sent: Monday, June 1, 2026 11:25 AM
> To: 'd3z-the-dev' <d3z.the.dev@gmail.com>; Sun, Xinpeng
> <Xinpeng.Sun@intel.com>; Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires
> <bentiss@kernel.org>
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Abhishek Tamboli
> <abhishektamboli9@gmail.com>; Sakari Ailus <sakari.ailus@linux.intel.com>
> Subject: RE: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system
> resume
> 
> Hi,
> 
> Sorry, I cannot get your exact name from this patch email.
> 
> From your issue description, I suppose Surface Pro 10 uses "Suspend-to-RAM"
> instead of "Standby" or "Suspend-to-Idle" for suspend/resume.
> As this documents mentioned, driver needs to take care the suspend type then
> handle resume flow carefully.
> 
> Actually, I had a patch for this "Suspend-to-RAM" support several months ago,
> please find attached file for the patch. This patch is still under our internal final
> validation.
> 
> It will be very appreciated if you can help test attached patch on your Surface
> device.
> If it works, then everything is fine, I will submit this patch with your name after
> our final validation.
> If it doesn't work for your case, then we need to future debug on your surface
> device to refine your patch to let it more standard.
> 
> Thanks!
> 
> Best Regards,
> Even Xu
> 
> > -----Original Message-----
> > From: d3z-the-dev <d3z.the.dev@gmail.com>
> > Sent: Saturday, May 30, 2026 6:22 AM
> > To: Xu, Even <even.xu@intel.com>; Sun, Xinpeng
> > <xinpeng.sun@intel.com>; Jiri Kosina <jikos@kernel.org>; Benjamin
> > Tissoires <bentiss@kernel.org>
> > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Abhishek Tamboli <abhishektamboli9@gmail.com>; Sakari Ailus
> > <sakari.ailus@linux.intel.com>; d3z-the-dev <d3z.the.dev@gmail.com>
> > Subject: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on
> > system resume
> >
> > On the Surface Pro 10 (Meteor Lake) the touchscreen stops working
> > after a suspend/resume cycle and only recovers after a reboot. The
> > driver logs
> > "GET_DEVICE_INFO: recv failed: -11" on resume.
> >
> > The touch IC loses power during system suspend (s2idle) on this
> > platform, the same way it does across hibernation. quickspi_resume()
> > only restores the THC port, interrupts and DMA and sends a HIDSPI_ON
> > command, assuming the touch IC kept its power and state. When it has
> > actually lost power the HIDSPI_ON command is never acknowledged and
> > the descriptor read fails, leaving the touchscreen dead until the module is
> reloaded.
> >
> > quickspi_restore() already handles this for hibernation by running
> > reset_tic() and reconfiguring the THC SPI/LTR settings. Make
> > quickspi_resume() do the same: quiesce interrupts, re-select the THC
> > port, reconfigure the SPI input/output addresses and read/write
> > parameters, run
> > reset_tic() to re-enumerate the device and restore the LTR configuration.
> >
> > Tested on a Surface Pro 10 across multiple s2idle suspend/resume cycles.
> >
> > Link: https://github.com/linux-surface/linux-surface/issues/1799
> >
> > Signed-off-by: d3z-the-dev <d3z.the.dev@gmail.com>
> > ---
> >  .../intel-quickspi/pci-quickspi.c             | 38 +++++++++++++++++--
> >  1 file changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> > b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> > index f669235f1883..d59278a3e5c1 100644
> > --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> > +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> > @@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
> >  	if (!qsdev)
> >  		return -ENODEV;
> >
> > +	ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = thc_port_select(qsdev->thc_hw, THC_PORT_TYPE_SPI);
> >  	if (ret)
> >  		return ret;
> >
> > +	thc_spi_input_output_address_config(qsdev->thc_hw,
> > +					    qsdev->input_report_hdr_addr,
> > +					    qsdev->input_report_bdy_addr,
> > +					    qsdev->output_report_addr);
> > +
> > +	ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
> > +				  qsdev->spi_read_io_mode,
> > +				  qsdev->spi_read_opcode,
> > +				  qsdev->spi_packet_size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
> > +				   qsdev->spi_write_io_mode,
> > +				   qsdev->spi_write_opcode,
> > +				   qsdev->spi_packet_size,
> > +				   qsdev->performance_limit);
> > +	if (ret)
> > +		return ret;
> > +
> >  	thc_interrupt_config(qsdev->thc_hw);
> >
> >  	thc_interrupt_enable(qsdev->thc_hw, true);
> >
> > -	ret = thc_dma_configure(qsdev->thc_hw);
> > +	/* The TIC may lose power across system suspend, reset it to recover */
> > +	ret = reset_tic(qsdev);
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
> > +	ret = thc_dma_configure(qsdev->thc_hw);
> >  	if (ret)
> >  		return ret;
> >
> > -	if (!device_may_wakeup(qsdev->dev))
> > -		return quickspi_set_power(qsdev, HIDSPI_ON);
> > +	thc_ltr_config(qsdev->thc_hw,
> > +		       qsdev->active_ltr_val,
> > +		       qsdev->low_power_ltr_val);
> > +
> > +	thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
> > +
> > +	qsdev->state = QUICKSPI_ENABLED;
> >
> >  	return 0;
> >  }
> > --
> > 2.54.0

Re: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[d3z]

From: Danny D. <d3z.the.dev@gmail.com>

Hi Even,

Thanks for the quick reply and for sharing your patch.

First, the missing piece from your mail: my name is Danny D.
(d3z.the.dev@gmail.com) - please use it for any
Signed-off-by/Reported-by/Tested-by.

One note on the starting assumption, though: in its current configuration
this Surface Pro 10 doesn't go to S3 at all. /sys/power/mem_sleep only
offers s2idle:

    $ cat /sys/power/mem_sleep
    [s2idle]

There's no "deep" entry to select here, so every suspend goes through
s2idle and never reaches PM_SUSPEND_MEM. The touch IC still loses power
across that s2idle suspend - the same as over hibernation - which is what
gives the "recv failed: -11" on resume.

That's the catch with the attached patch: it gates the THC reconfigure on

    if (last_suspend_state == PM_SUSPEND_MEM)

i.e. S3 only. On this machine that branch never runs, so it wouldn't bring
the touchscreen back here. Happy to apply it and confirm on the device, but
from the code path alone it can't help while we're s2idle-only.

What I did instead was key off whether the device actually kept power
rather than the suspend type: run the full reset only when the device isn't
a wake source (device_may_wakeup() == false), mirroring quickspi_restore().
A wake-enabled device stays on the light path so wake-on-touch is preserved;
one that lost power goes through reset_tic() re-enumeration. That fires on
both s2idle and S3, which is what makes it work on the SP10.

I've tested this on the Surface Pro 10 across many s2idle suspend/resume
cycles - touch and pen both keep working - and just posted a v2 that adds
the device_may_wakeup() gate.

Could we converge on the wakeup-based condition? I'm glad to test any
revision of your patch on the hardware, and equally glad for you to carry
the fix with my Signed-off-by/Tested-by once it handles the s2idle case.

Thanks,
Danny

[PATCH v2] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[d3z]

From: Danny D. <d3z.the.dev@gmail.com>

On the Surface Pro 10 (Meteor Lake) the touchscreen stops working after a
suspend/resume cycle and only recovers after a reboot. The driver logs
"GET_DEVICE_INFO: recv failed: -11" on resume.

This platform suspends through s2idle: /sys/power/mem_sleep exposes
"[s2idle]" as the only state, there is no "deep"/S3 entry at all. The
touch IC nonetheless loses power across that s2idle suspend, the same
way it does across hibernation. quickspi_resume() only re-selects the
THC port, restores interrupts and DMA and sends a HIDSPI_ON command,
assuming the touch IC kept its power and state. When it has actually
lost power the HIDSPI_ON command is never acknowledged and the
descriptor read fails, leaving the touchscreen dead until the module is
reloaded.

quickspi_restore() already handles this for hibernation by
reconfiguring the THC SPI/LTR settings and running reset_tic() to
re-enumerate the device. Make quickspi_resume() do the same when the
device is not a wake source. A wake-enabled device keeps its power and
state across suspend, so it stays on the light restore path: resetting
it would discard a pending wake touch event and break wake-on-touch.

The non-wake path mirrors the existing quickspi_restore() sequence,
including enabling interrupts before reset_tic(), so it introduces no
new ordering relative to code already in the driver.

This change has been validated on a Surface Pro 10 running the
linux-surface kernel across multiple s2idle suspend/resume cycles; it
has not been tested on a mainline build.

Closes: https://github.com/linux-surface/linux-surface/issues/1799
Signed-off-by: Danny D. <d3z.the.dev@gmail.com>
---
v1 -> v2:
 - Only run the full reset when the device is not a wake source
   (device_may_wakeup()), so wake-on-touch is no longer disturbed.
 - Reword the changelog around s2idle: the SP10 has no "deep"/S3 state, the
   touch IC loses power across s2idle.

 .../hid/intel-thc-hid/intel-quickspi/pci-quickspi.c | 60 +++++++++++++++++--
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -784,20 +784,72 @@ static int quickspi_resume(struct device
 	if (ret)
 		return ret;
 
+	/*
+	 * A wake-enabled device keeps its power and state across suspend, so
+	 * only restore the THC context. Resetting it here would discard a
+	 * pending wake touch event and break wake-on-touch.
+	 */
+	if (device_may_wakeup(qsdev->dev)) {
+		thc_interrupt_config(qsdev->thc_hw);
+
+		thc_interrupt_enable(qsdev->thc_hw, true);
+
+		ret = thc_dma_configure(qsdev->thc_hw);
+		if (ret)
+			return ret;
+
+		return thc_interrupt_quiesce(qsdev->thc_hw, false);
+	}
+
+	/*
+	 * Otherwise the touch IC may have lost power across suspend. On
+	 * platforms that suspend through s2idle (for example the Surface Pro
+	 * 10, whose firmware exposes s2idle as the only mem_sleep state) the
+	 * IC loses power the same way it does across hibernation. A plain
+	 * HIDSPI_ON is then not acknowledged and the descriptor read fails, so
+	 * re-enumerate the device through the full reset flow already used by
+	 * quickspi_restore().
+	 */
+	thc_spi_input_output_address_config(qsdev->thc_hw,
+					    qsdev->input_report_hdr_addr,
+					    qsdev->input_report_bdy_addr,
+					    qsdev->output_report_addr);
+
+	ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
+				  qsdev->spi_read_io_mode,
+				  qsdev->spi_read_opcode,
+				  qsdev->spi_packet_size);
+	if (ret)
+		return ret;
+
+	ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
+				   qsdev->spi_write_io_mode,
+				   qsdev->spi_write_opcode,
+				   qsdev->spi_packet_size,
+				   qsdev->performance_limit);
+	if (ret)
+		return ret;
+
 	thc_interrupt_config(qsdev->thc_hw);
 
 	thc_interrupt_enable(qsdev->thc_hw, true);
 
-	ret = thc_dma_configure(qsdev->thc_hw);
+	/* The touch IC may have lost power, reset it to recover */
+	ret = reset_tic(qsdev);
 	if (ret)
 		return ret;
 
-	ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
+	ret = thc_dma_configure(qsdev->thc_hw);
 	if (ret)
 		return ret;
 
-	if (!device_may_wakeup(qsdev->dev))
-		return quickspi_set_power(qsdev, HIDSPI_ON);
+	thc_ltr_config(qsdev->thc_hw,
+		       qsdev->active_ltr_val,
+		       qsdev->low_power_ltr_val);
+
+	thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
+
+	qsdev->state = QUICKSPI_ENABLED;
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v2] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing cleanup on error paths in `quickspi_resume()` leaves hardware interrupts enabled.

Pre-existing issues:
- [High] Potential Use-After-Free in the IRQ handler during device removal.
- [High] Concurrent execution of hardware initialization (`reset_tic`) between PM callbacks and the IRQ thread.
--

commit 4380dc624b155997446c58c62fab2b9cced12a58
Author: Danny D <d3z.the.dev@gmail.com>

HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume

This commit updates quickspi_resume() to handle s2idle suspend by restoring
the THC context and resetting the touch IC when the device is not a wake
source. It reconfigures the THC SPI/LTR settings and runs reset_tic() to
re-enumerate the device.

Closes: https://github.com/linux-surface/linux-surface/issues/1799

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index f669235f18839..4ae2e1718b30c 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -784,20 +784,72 @@ static int quickspi_resume(struct device *device)
[ ... ]
> +	thc_interrupt_config(qsdev->thc_hw);
>  
> +	thc_interrupt_enable(qsdev->thc_hw, true);
>  
> -	ret = thc_dma_configure(qsdev->thc_hw);
> +	/* The touch IC may have lost power, reset it to recover */
> +	ret = reset_tic(qsdev);
> +	if (ret)
> +		return ret;

[Severity: High]
If reset_tic() or the subsequent thc_dma_configure() fails, does this
function return without disabling the hardware interrupts?

It appears thc_interrupt_enable() is called before reset_tic(), but
the error paths directly return without undoing the interrupt enable
or quiescing the hardware. Could this leave the device in an active
state capable of generating unhandled interrupts?

[Severity: High]
This is a pre-existing issue, but the patch newly introduces this
pattern into the resume path. Is it safe to call reset_tic() here
while hardware interrupts are actively enabled?

If a fatal error interrupt occurs during the reset window, the
threaded IRQ handler could catch it and invoke try_recover():

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:try_recover() {
    ...
    ret = reset_tic(qsdev);
    ...
}

Since there doesn't appear to be mutual exclusion protecting the
device state recovery path, could both the PM callback and the IRQ
thread execute acpi_tic_reset() concurrently and corrupt the hardware
state?

> +
> +	ret = thc_dma_configure(qsdev->thc_hw);
> +	if (ret)
> +		return ret;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the device
removal process have a use-after-free window in the IRQ handler?

In quickspi_remove(), the DMA buffers are freed before hardware
interrupts are disabled:

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:quickspi_remove() {
    ...
    quickspi_dma_deinit(qsdev);

    pm_runtime_get_noresume(qsdev->dev);

    quickspi_dev_deinit(qsdev);
    ...
}

Because the IRQ is managed by devm_request_threaded_irq(), it isn't
freed until after quickspi_remove() returns. If an interrupt fires
during this window, could the threaded handler access freed memory?

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:read_dma_buffer() {
    ...
    prd_tbl = &read_config->prd_tbls[prd_table_index];
    mes_len = calc_message_len(prd_tbl, &nent);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601211828.112626-1-d3z.the.dev@gmail.com?part=1