Re: [PATCH v2 3/5] HID: intel-thc-hid: intel-ipts: register the HID device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: broken teardown order in
  thc_ipts_port_stop()
- [Medium] HID: intel-thc-hid: intel-ipts: unsynchronized state in
  thc_ipts_feature()
--

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

HID: intel-thc-hid: intel-ipts: register the HID device

This commit registers a HID low-level driver for the IPTS port to fetch
report descriptors and feature reports over DMA and pass input reports to
the HID core.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index 790c3eaae15f2..94731303af806 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c

[ ... ]

> +/* The report is collected in a private buffer; the caller copies it out. */
> +static void thc_ipts_feature(struct thc_ipts *idev, const u8 *data,
> +			     size_t len)
> +{
> +	if (!len || !READ_ONCE(idev->feature_pending))
> +		return;
> +
> +	/* An answer to an earlier request carries its own report ID. */
> +	if (READ_ONCE(idev->feature_id) &&
> +	    data[0] != READ_ONCE(idev->feature_id))
> +		return;
> +
> +	len = min(len, idev->max_rx);
> +	memcpy(idev->feature, data, len);
> +	idev->feature_len = len;
> +	WRITE_ONCE(idev->feature_pending, false);
> +	WRITE_ONCE(idev->feature_done, true);
> +	wake_up(&idev->wq);
> +}

[ ... ]

> +/*
> + * A feature report is not pushed by the controller: it is asked for and comes
> + * back as a frame of its own, so the request waits for the handler.
> + */
> +static int thc_ipts_get_feature(struct thc_ipts *idev, unsigned char reportnum,
> +				u8 *buf, size_t len)
> +{
> +	int ret = -ETIMEDOUT;
> +	unsigned long timeout = msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS);
> +	int try;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	WRITE_ONCE(idev->feature_done, false);
> +	WRITE_ONCE(idev->feature_id, reportnum);
> +	WRITE_ONCE(idev->feature_pending, true);
> +
> +	for (try = 0; try < THC_IPTS_REQ_TRIES; try++) {
> +		ret = thc_ipts_send(idev, THC_IPTS_TX_GET_FEATURE, &reportnum,
> +				    1);
> +		if (ret)
> +			break;
> +
> +		if (wait_event_timeout(idev->wq, READ_ONCE(idev->feature_done),
> +				       timeout)) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	WRITE_ONCE(idev->feature_pending, false);
> +
> +	if (ret) {
> +		dev_err(idev->dev, "no feature report %u after %d tries\n",
> +			reportnum, try);
> +		return ret;
> +	}
> +
> +	/* The payload is the report as user space writes it: ID first. */
> +	len = min(len, idev->feature_len);
> +	if (!len)
> +		return -EPROTO;
> +	memcpy(buf, idev->feature, len);
> +
> +	return len;
> +}

[Severity: Medium]
Can a late reply in thc_ipts_feature() overwrite idev->feature or complete
a later thc_ipts_get_feature() call with the wrong report?

thc_ipts_feature() checks feature_pending and feature_id without a lock
shared with thc_ipts_get_feature():

CPU0                                  CPU1 (thc_ipts_thread)
thc_ipts_get_feature(idev, id_A, ...)
  feature_id = id_A
  feature_pending = true
  ...
                                      thc_ipts_feature()
                                        if (!READ_ONCE(idev->feature_pending))
                                        if (data[0] != READ_ONCE(feature_id))
  /* times out or -EINTR on retry */
  feature_pending = false
  unlock(&idev->req_lock)

thc_ipts_get_feature(idev, id_B, ...)
  feature_done = false
  feature_id = id_B
  feature_pending = true
                                        memcpy(idev->feature, data, len)
                                        idev->feature_len = len
                                        feature_done = true  <-- wrong report
                                        wake_up(&idev->wq)

Also, when try 0 times out and try 1 sends a second THC_IPTS_TX_GET_FEATURE
for the same report ID, the delayed reply to try 0 wakes try 1 and leaves
the reply to try 1 in flight to satisfy the next thc_ipts_get_feature()
request for that report ID with stale data.

[ ... ]

> @@ -452,5 +732,9 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
>  			  THC_M_PRT_CONTROL_DEVRST);
>  	thc_ipts_quiesce(idev, true);
>  
> -	thc_ipts_dma_stop(idev);
> +	/* Let a send inside thc_dma_write finish before the DMA goes. */
> +	scoped_guard(mutex, &idev->req_lock)
> +		thc_ipts_dma_stop(idev);
> +
> +	thc_ipts_hid_remove(idev);
>  }

[Severity: Medium]
Does thc_ipts_port_stop() disable interrupts and reset the controller
before waiting for in-flight HID requests under idev->req_lock?

drivers/hid/intel-thc-hid/intel-ipts/ipts.c:thc_ipts_port_stop() {
    ...
	WRITE_ONCE(idev->stopping, true);
	thc_interrupt_enable(idev->thc, false);
	synchronize_irq(idev->irq);
	cancel_work_sync(&idev->recover_work);

	/* Stop the controller before the buffers it writes into go away. */
	if (idev->running)
		thc_ipts_touch_set(idev, false);

	idev->running = false;
	regmap_clear_bits(idev->thc->thc_regmap,
			  THC_M_PRT_CONTROL_OFFSET,
			  THC_M_PRT_CONTROL_DEVRST);
	thc_ipts_quiesce(idev, true);

	/* Let a send inside thc_dma_write finish before the DMA goes. */
	scoped_guard(mutex, &idev->req_lock)
		thc_ipts_dma_stop(idev);

	thc_ipts_hid_remove(idev);
}

Because thc_ipts_hid_remove() runs last and idev->req_lock is only acquired
around thc_ipts_dma_stop(), a concurrent thc_ipts_hid_raw_request() call can
still be running while the port is being stopped:

CPU0                                  CPU1
thc_ipts_hid_raw_request()
  lock(&idev->req_lock)
  thc_ipts_send()
    if (READ_ONCE(idev->stopping))
    if (!READ_ONCE(idev->dma_ready))
                                      thc_ipts_port_stop()
                                        idev->stopping = true
                                        thc_interrupt_enable(..., false)
                                        regmap_clear_bits(..., DEVRST)
                                        thc_ipts_quiesce(..., true)
                                        lock(&idev->req_lock)
    thc_dma_write()
      dma_set_start_bit(...)          <-- starts TxDMA in reset/quiesced
      wait_event_interruptible_timeout(dev->write_complete_wait, ..., 1 * HZ)
                                      <-- times out with interrupts masked

Even if thc_dma_write() was already waiting before thc_ipts_port_stop()
started, masking interrupts prevents THC_TXDMA_INT from waking
dev->write_complete_wait, forcing a 1-second timeout on dev->thc_bus_lock
and idev->req_lock.  Likewise, a caller waiting in thc_ipts_get_feature()
on idev->wq is not woken by thc_ipts_port_stop() and does not check
idev->stopping.

Removing the HID device first (or serializing the stopping state under
idev->req_lock and waking idev->wq before masking interrupts and resetting
the controller) would let active transfers complete cleanly.

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