[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