Re: [PATCH v5 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 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: timeout race in
  thc_ipts_get_feature()
--

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

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

This commit registers a HID device for the IPTS port and implements report
descriptor parsing, input report forwarding, and feature/output report
requests over DMA.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index 76224f4444828..ea85c9a150899 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> +static void thc_ipts_feature(struct thc_ipts *idev, const u8 *data,
> +			     size_t len)
> +{
> +	if (!len)
> +		return;
> +
> +	/* Only a frame that arrives while a request is waiting is taken. */
> +	spin_lock_irq(&idev->feature_lock);
> +	if (!idev->feature_pending ||
> +	    (idev->feature_id && data[0] != idev->feature_id)) {
> +		spin_unlock_irq(&idev->feature_lock);
> +		return;
> +	}
> +
> +	len = min(len, idev->max_rx);
> +	memcpy(idev->feature, data, len);
> +	idev->feature_len = len;
> +	idev->feature_pending = false;
> +	WRITE_ONCE(idev->feature_done, true);
> +	spin_unlock_irq(&idev->feature_lock);
> +
> +	wake_up(&idev->wq);
> +}
[ ... ]
> +/* A feature report comes back as a frame of its own: the request waits. */
> +static int thc_ipts_get_feature(struct thc_ipts *idev, unsigned char reportnum,
> +				u8 *buf, size_t len)
> +{
> +	unsigned long timeout = msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS);
> +	int ret;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	/* The frame carries only the report ID, so a late answer cannot be
> +	 * told from the next one's: after a timeout the channel stays shut.
> +	 */
> +	if (READ_ONCE(idev->feature_off))
> +		return -EIO;
> +
> +	spin_lock_irq(&idev->feature_lock);
> +	WRITE_ONCE(idev->feature_done, false);
> +	idev->feature_id = reportnum;
> +	idev->feature_pending = true;
> +	spin_unlock_irq(&idev->feature_lock);
> +
> +	ret = thc_ipts_send(idev, THC_IPTS_TX_GET_FEATURE, &reportnum, 1);
> +	if (!ret &&
> +	    !wait_event_timeout(idev->wq, READ_ONCE(idev->feature_done),
> +				timeout))
> +		ret = -ETIMEDOUT;
> +
> +	/* Past this point no request is waiting for the frame. */
> +	spin_lock_irq(&idev->feature_lock);
> +	idev->feature_pending = false;
> +	spin_unlock_irq(&idev->feature_lock);
> +
> +	if (ret == -ETIMEDOUT) {
> +		/* An answer may still be on its way. */
> +		dev_err(idev->dev, "no feature report %u\n", reportnum);
> +		WRITE_ONCE(idev->feature_off, true);
> +		return ret;
> +	}

[Severity: Medium]
Can a feature report arriving right as wait_event_timeout() expires be
dropped while still latching idev->feature_off?

CPU0                                    CPU1 (irq thread)
thc_ipts_get_feature()
  wait_event_timeout(...) -> 0
  ret = -ETIMEDOUT;
                                        thc_ipts_feature()
                                          spin_lock_irq(&idev->feature_lock);
                                          /* feature_pending is still true */
                                          memcpy(idev->feature, data, len);
                                          idev->feature_pending = false;
                                          WRITE_ONCE(idev->feature_done, true);
                                          spin_unlock_irq(&idev->feature_lock);
  spin_lock_irq(&idev->feature_lock);
  idev->feature_pending = false;
  spin_unlock_irq(&idev->feature_lock);
  if (ret == -ETIMEDOUT) {
    WRITE_ONCE(idev->feature_off, true);
    return -ETIMEDOUT;                  <-- drops reply and shuts channel

Because idev->feature_done is not re-checked under idev->feature_lock when
clearing idev->feature_pending, a reply accepted in that window is
discarded, the request fails with -ETIMEDOUT, and all further feature
requests fail with -EIO until the port is restarted despite no late answer
remaining in flight.

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