Re: [PATCH v3 4/5] HID: intel-thc-hid: intel-ipts: handle system sleep

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: race in thc_ipts_port_stop()
  re-enables IRQ
--

commit c33cb8e49648df90b4589c66ad8cc8769f40b6b6
Author: Emfox Zhou <emfox@debian.org>

HID: intel-thc-hid: intel-ipts: handle system sleep

This commit adds system sleep and resume callbacks to the intel-ipts PCI
driver by stopping and restarting the port across suspend and resume.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> index 8f8db21df8250..81a46149be2e9 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> @@ -142,6 +142,41 @@ static void thc_ipts_remove(struct pci_dev *pci)
>  	pci_clear_master(pci);
>  }
>  
> +/*
> + * The controller forgets the streaming mode and hidraw cannot ask user space
> + * to repeat itself, so the port is taken down and rebuilt: the HID device
> + * comes back and the daemon restarts with it.
> + */
> +static int thc_ipts_suspend(struct device *dev)
> +{
> +	struct thc_ipts *idev = dev_get_drvdata(dev);
> +
> +	if (!idev)
> +		return -ENODEV;
> +
> +	thc_ipts_port_stop(idev);

[Severity: Medium]
Can thc_ipts_port_stop() race with thc_ipts_thread() or thc_ipts_recover()
and leave THC interrupts enabled across suspend and resume?

CPU0                                     CPU1
thc_ipts_suspend()
  thc_ipts_port_stop()
                                         thc_ipts_thread()
                                           if (!READ_ONCE(idev->stopping))
    WRITE_ONCE(idev->stopping, true);
    thc_interrupt_enable(idev->thc, false);
                                             thc_interrupt_enable(idev->thc, true);
    synchronize_irq(idev->irq);
    cancel_work_sync(&idev->recover_work);

thc_ipts_port_stop() masks interrupts before synchronize_irq() and
cancel_work_sync(), and does not mask them again after those calls finish.
If the IRQ remains registered across suspend and resume and the controller
raises an interrupt during thc_ipts_touch_set() or during
thc_ipts_resume() -> thc_ipts_port_start() before idev->dma_ready is set,
thc_ipts_hardirq() sees idev->stopping or !idev->dma_ready and returns
IRQ_NONE without clearing the hardware status bits:

drivers/hid/intel-thc-hid/intel-ipts/ipts.c:thc_ipts_hardirq() {
	struct thc_ipts *idev = data;

	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
		return IRQ_NONE;
    ...
}

Calling thc_interrupt_enable(idev->thc, false) again after
synchronize_irq() and cancel_work_sync() in thc_ipts_port_stop() would
ensure interrupts stay masked once both contexts have exited.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114183624.2379573-1-emfox@debian.org?part=4