Re: [PATCH v2 5/5] HID: intel-thc-hid: intel-ipts: document the IPTS mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: shared IRQ wakes thc_ipts_thread() during
  recovery
--

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

HID: intel-thc-hid: intel-ipts: document the IPTS mode

This series adds the intel-ipts transport driver for Intel Touch Host
Controller ports operating in IPTS mode and documents the protocol details
the driver relies on.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> new file mode 100644
> index 0000000000000..94731303af806
> --- /dev/null
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c

[ ... ]

> +irqreturn_t thc_ipts_hardirq(int irq, void *data)
> +{
> +	struct thc_ipts *idev = data;
> +
> +	/*
> +	 * The pin is shared and this may be another function's interrupt; the
> +	 * cause can only be read in the thread, so claim it. Mask first, or a
> +	 * level triggered line stays asserted on the way out.
> +	 */
> +	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready)) {
> +		thc_interrupt_enable(idev->thc, false);
> +		return IRQ_HANDLED;
> +	}
> +
> +	thc_interrupt_enable(idev->thc, false);
> +
> +	return IRQ_WAKE_THREAD;
> +}

[ ... ]

> +void thc_ipts_recover(struct work_struct *work)
> +{
> +	struct thc_ipts *idev = container_of(work, struct thc_ipts,
> +					     recover_work);
> +
> +	if (thc_rxdma_reset(idev->thc)) {
> +		/* Nothing left: keep the port masked until it restarts. */
> +		dev_err(idev->dev, "cannot reset the receive engine\n");
> +		return;
> +	}
> +
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +}
> +
> +irqreturn_t thc_ipts_thread(int irq, void *data)
> +{
> +	struct thc_ipts *idev = data;
> +	bool recovering = false;
> +	int pass;
> +
> +	/*
> +	 * The port stays masked until the end of this handler, so a cause that
> +	 * arrives in between leaves only its status bit: keep reading until
> +	 * the core reports none.
> +	 */
> +	for (pass = 0; pass < THC_IPTS_IRQ_CAUSES; pass++) {
> +		int mask = thc_interrupt_handler(idev->thc);
> +
> +		/* The core reports this one when no cause is pending. */
> +		if (mask == BIT(THC_UNKNOWN_INT))
> +			break;
> +
> +		if (mask & (BIT(THC_FATAL_ERR_INT) | BIT(THC_TXN_ERR_INT))) {
> +			dev_err(idev->dev,
> +				"transfer error, resetting the engine\n");
> +			recovering = true;
> +			/*
> +			 * Keep reading: the core stops at the first cause, so
> +			 * a Tx completion queued behind the error would wait
> +			 * out its timeout under the bus lock.
> +			 */
> +			continue;
> +		}
> +
> +		if (recovering)
> +			continue;
> +
> +		/*
> +		 * A non-DMA cause needs no action: the core acknowledges it,
> +		 * and this port moves report data through the DMA engines.
> +		 */
> +		if (mask & BIT(THC_RXDMA2_INT) && thc_ipts_read_frames(idev))
> +			recovering = true;
> +	}
> +
> +	/*
> +	 * The work item resets the engine and unmasks the port again; reading
> +	 * frames while it rewrites the ring would corrupt the descriptors.
> +	 */
> +	if (recovering) {
> +		schedule_work(&idev->recover_work);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Can a foreign interrupt on the shared INTx line wake thc_ipts_thread()
while recover_work is running or after thc_rxdma_reset() has failed?

irq/thc_ipts                      kworker
thc_ipts_thread()
  recovering = true
  schedule_work(&idev->recover_work)
                                  thc_ipts_recover()
                                    thc_rxdma_reset(idev->thc)
<shared INTx from another device>
thc_ipts_hardirq()
  return IRQ_WAKE_THREAD
thc_ipts_thread()
  thc_interrupt_handler(idev->thc)
  thc_ipts_read_frames(idev)      <-- races with thc_rxdma_reset()
  thc_interrupt_enable(..., true) <-- unmasks port mid-reset

thc_ipts_probe() requests the legacy INTx vector with IRQF_SHARED, and
thc_ipts_hardirq() wakes thc_ipts_thread() whenever idev->stopping is
false and idev->dma_ready is true.  Because recovering is a local variable
in thc_ipts_thread(), any interrupt from another PCI function sharing the
line while recover_work is pending or executing wakes thc_ipts_thread()
again with recovering set to false.

That runs thc_interrupt_handler() and thc_ipts_read_frames() concurrently
with thc_rxdma_reset() rewriting the RXDMA2 descriptors, and then calls
thc_interrupt_enable(idev->thc, true), unmasking the port mid-reset or
after a failed reset that intended to keep the port masked.

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