Re: [PATCH v2] 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] 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