Re: [PATCH v3 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: check recovering in
  thc_ipts_send()
--

commit 4354955543093e96e544cc3b320f82723b8a7849
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 ac0074a85bfa4..bf75f8cc7994e 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> @@ -139,8 +141,79 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
>  				 &touch);
>  }
>  
> +/*
> + * One frame to the controller: code, length, then the payload padded to four
> + * bytes. The frame is built in @idev->tx_frame, so callers serialise with
> + * @idev->req_lock.
> + */
> +static int thc_ipts_send(struct thc_ipts *idev, u32 code, const void *data,
> +			 size_t len)
> +{
> +	size_t padded = round_up(len, 4);
> +	u8 *frame = idev->tx_frame;
> +
> +	if (READ_ONCE(idev->stopping))
> +		return -ESHUTDOWN;
> +	if (!READ_ONCE(idev->dma_ready))
> +		return -EAGAIN;
> +	if (THC_IPTS_TX_HDR_SIZE + padded > idev->max_tx)
> +		return -EMSGSIZE;
> +
> +	put_unaligned_le32(code, frame);
> +	put_unaligned_le32((u32)len, frame + 4);
> +	memset(frame + THC_IPTS_TX_HDR_SIZE, 0, padded);
> +	if (len && data)
> +		memcpy(frame + THC_IPTS_TX_HDR_SIZE, data, len);
> +
> +	return thc_dma_write(idev->thc, frame, THC_IPTS_TX_HDR_SIZE + padded);
> +}

[Severity: Medium]
Should thc_ipts_send() also check READ_ONCE(idev->recovering) before
calling thc_dma_write()?

When a transfer or receive error occurs, thc_ipts_thread() leaves the port
interrupts masked, sets idev->recovering, and schedules idev->recover_work:

CPU0                              CPU1 (kworker)
thc_ipts_thread()
  WRITE_ONCE(idev->recovering, true)
  schedule_work(&idev->recover_work)
  return IRQ_HANDLED (port stays masked)

thc_ipts_send()
  thc_dma_write()
    mutex_lock_interruptible(&dev->thc_bus_lock)
    dma_set_start_bit(...)
                                  thc_ipts_recover()
                                    thc_rxdma_reset()
                                      mutex_lock_interruptible(&dev->thc_bus_lock)
    wait_event_interruptible_timeout(dev->write_complete_wait, ...)
      <-- stalls 1s for masked IRQ while blocking thc_ipts_recover()

Because thc_ipts_recover() cannot unmask the port until it acquires
thc_bus_lock inside thc_rxdma_reset(), thc_dma_write() always times out
after 1 second and aborts the retry loop in thc_ipts_hid_parse() or
thc_ipts_get_feature().  Also, if thc_rxdma_reset() fails in
thc_ipts_recover(), the port stays masked with idev->recovering set while
idev->dma_ready remains true, so every subsequent thc_ipts_send() call will
start TxDMA on the masked port and stall for 1 second holding req_lock and
thc_bus_lock.

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