Re: [PATCH v4 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: broken feature_seq check in
  thc_ipts_get_feature()
--

commit 933c7f8eb6c2821b64a9386be774666dc0883345
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 raw 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 b48199448dbec..80973944a8994 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)
> +		return;
> +
> +	/*
> +	 * A request that timed out is dropped by the sender, which may be
> +	 * asking for another report by now: check and copy in one step. The
> +	 * serial number tells an earlier answer from the one asked for.
> +	 */
> +	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;
> +	idev->feature_ack = idev->feature_seq;
> +	WRITE_ONCE(idev->feature_done, true);
> +	spin_unlock_irq(&idev->feature_lock);
> +
> +	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);
> +	unsigned int seq;
> +	int try;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	/*
> +	 * The same report may be asked for twice in a row and the answer to
> +	 * the first request can still be in flight: the ID alone cannot tell
> +	 * the two apart, so every request carries a serial number.
> +	 */
> +	spin_lock_irq(&idev->feature_lock);
> +	WRITE_ONCE(idev->feature_done, false);
> +	idev->feature_id = reportnum;
> +	idev->feature_pending = true;
> +	seq = ++idev->feature_seq;
> +	spin_unlock_irq(&idev->feature_lock);
> +
> +	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) &&
> +				       READ_ONCE(idev->feature_ack) == seq,
> +				       timeout)) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		ret = -ETIMEDOUT;
> +	}

[Severity: Medium]
Can idev->feature_seq distinguish a delayed reply to an earlier request from
the current request?

thc_ipts_get_feature() only transmits reportnum on the wire, and
thc_ipts_feature() unconditionally sets idev->feature_ack = idev->feature_seq
whenever a matching report ID arrives while idev->feature_pending is true.
Because idev->feature_seq is only updated under idev->req_lock at the start
of thc_ipts_get_feature(), a late reply from a timed-out request for the
same report ID will be stamped with the new request's sequence number and
consumed as the new answer.

In addition, idev->feature_done and idev->feature_pending are initialized
once before the retry loop rather than on each attempt.  If
wait_event_timeout() expires on try 0 and the delayed reply arrives just as
try 1 begins, thc_ipts_feature() sets feature_done = true and
feature_pending = false before try 1 calls thc_ipts_send().  Try 1 then
sends a duplicate THC_IPTS_TX_GET_FEATURE frame before checking
feature_done, leaving an extra reply in flight (or failing the request if
thc_ipts_send() returns an error even though the reply already arrived).

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