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

[sashiko-bot@kernel.org]

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