Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] HID: intel-thc-hid: intel-ipts: missing INIT_WORK() for
idev->recover_work
- [High] HID: intel-thc-hid: premature DMA teardown in
thc_ipts_port_stop()
- [Medium] HID: intel-thc-hid: intel-ipts: uninitialized len in
thc_ipts_read_frames()
- [Medium] HID: intel-thc-hid: intel-ipts: RxDMA reset race and lost
TxDMA completion
- [Medium] HID: intel-thc-hid: intel-ipts: unconditional IRQ_HANDLED on
shared IRQ
--
commit ad15f281d350f2110628e7e198685de7d1f1724e
Author: Emfox Zhou [off-list ref]
HID: intel-thc-hid: intel-ipts: receive reports
This commit configures the RxDMA2 and TxDMA engines, requests a threaded
interrupt handler to receive and decode incoming frames, and adds a
workqueue handler to reset RxDMA on transfer errors.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
index b16dfdc73d751..8529b91e063f7 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -124,6 +135,162 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
[ ... ]
quoted hunk ↗ jump to hunk
+static int thc_ipts_read_frames(struct thc_ipts *idev)
+{
+ for (;;) {
+ int finished = 0;
+ size_t len;
+
+ if (thc_rxdma_read(idev->thc, THC_RXDMA2, idev->rx_frame,
+ &len, &finished)) {
+ dev_warn_once(idev->dev,
+ "cannot read a frame from the receive engine\n");
+ return -EIO;
+ }
+
+ thc_ipts_handle_frame(idev, len);
+ if (finished)
+ break;
+ }
+
+ return 0;
+}
[Severity: Medium]
Can thc_ipts_read_frames() pass an uninitialized len to
thc_ipts_handle_frame()?
thc_rxdma_read() -> thc_dma_read() only writes *read_len when
!is_dma_buf_empty(), returning 0 with *read_finished = 1 when the ring is
already empty:
drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:thc_dma_read() {
...
if (!is_dma_buf_empty(dev, read_config, &read_ptr, &write_ptr)) {
prd_table_index = write_ptr & THC_POINTER_MASK;
status = read_dma_buffer(dev, read_config, prd_table_index, read_buff);
if (status <= 0) {
dev_err_once(dev->dev, "read DMA buffer failed %d\n", status);
return -EIO;
}
*read_len = status;
...
}
If a second frame arrives after thc_interrupt_handler() clears the EOF
status bit on pass 0 of thc_ipts_thread() but before the loop in
thc_ipts_read_frames() finishes draining RxDMA2, pass 0 drains both frames
while the hardware latches EOF again. Pass 1 then sees BIT(THC_RXDMA2_INT)
with an empty ring, and thc_ipts_handle_frame() reads stack garbage from
len and re-processes stale data in idev->rx_frame whenever len is at least
THC_IPTS_RX_HDR_SIZE.
Initializing len = 0 before thc_rxdma_read() avoids this.
quoted hunk ↗ jump to hunk
+
+/*
+ * Run from a work item: thc_rxdma_reset() takes the bus lock, which
+ * thc_dma_write() holds while it waits for a completion this thread has to
+ * deliver, so resetting inline would stall until the write times out.
+ */
+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))
+ dev_err(idev->dev, "cannot reset the receive engine\n");
+}
[Severity: High]
Where is idev->recover_work initialized with INIT_WORK()?
Neither thc_ipts_probe() nor thc_ipts_port_start() calls
INIT_WORK(&idev->recover_work, thc_ipts_recover) in this commit (it is only
added in the next patch, commit "HID: intel-thc-hid: intel-ipts: expose the
port as a HID device"). At this commit, cancel_work_sync() in
thc_ipts_port_stop() hits WARN_ON(!work->func) in __flush_work(), and if
thc_ipts_thread() calls schedule_work(&idev->recover_work), __queue_work()
hits WARN_ON(!list_empty(&work->entry)) and leaves WORK_STRUCT_PENDING_BIT
set without queuing the work, causing cancel_work_sync() to spin forever in
work_grab_pending().
quoted hunk ↗ jump to hunk
+
+irqreturn_t thc_ipts_thread(int irq, void *data)
+{
+ struct thc_ipts *idev = data;
+ 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 receive engine\n");
+ schedule_work(&idev->recover_work);
+ break;
+ }
+
+ /*
+ * 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))
+ schedule_work(&idev->recover_work);
+ }
+
+ if (!READ_ONCE(idev->stopping))
+ thc_interrupt_enable(idev->thc, true);
[Severity: Medium]
Can thc_ipts_recover() race with thc_ipts_thread() on RxDMA2, and can
breaking out of the loop on error drop a pending TxDMA completion?
When thc_ipts_thread() schedules idev->recover_work, it still unmasks THC
interrupts before returning, and thc_rxdma_read() does not take
thc_bus_lock:
kworker irq/thc_ipts_thread
thc_ipts_recover()
thc_rxdma_reset()
thc_dma_unconfigure()
thc_dma_configure()
thc_ipts_read_frames()
thc_rxdma_read()
<-- corrupts PRD tables & ring ptrs
In addition, thc_interrupt_handler() returns early on THC_NONDMA_INT,
THC_TXN_ERR_INT (both transaction error and buffer overrun), and
THC_FATAL_ERR_INT before checking THC_M_PRT_WRITE_INT_STS_OFFSET. Breaking
out of the loop on BIT(THC_FATAL_ERR_INT) | BIT(THC_TXN_ERR_INT) (and
limiting THC_IPTS_IRQ_CAUSES to 4 when those four early returns can precede
the data path) leaves a concurrent TxDMA completion unacknowledged, so an
in-flight thc_dma_write() holds thc_bus_lock until its timeout expires
before thc_rxdma_reset() can run.
quoted hunk ↗ jump to hunk
+
+ return IRQ_HANDLED;
+}
[Severity: Medium]
Should thc_ipts_thread() and thc_ipts_hardirq() return IRQ_NONE when the
interrupt did not come from this port?
The IRQ is requested with IRQF_SHARED, but thc_ipts_hardirq() returns
IRQ_HANDLED when idev->stopping or !idev->dma_ready, and thc_ipts_thread()
returns IRQ_HANDLED even when pass 0 sees BIT(THC_UNKNOWN_INT). Claiming
every foreign interrupt on the shared PCH line prevents note_interrupt() in
the core IRQ spurious detector from ever counting unhandled interrupts if
another device on the line gets stuck asserting its level interrupt.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -220,6 +403,16 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
if (!idev->thc)
return;
+ /*
+ * Mask the line, then wait for a handler that may be holding a
+ * frame for the HID device which is about to go away.
+ */
+ WRITE_ONCE(idev->stopping, true);
+ thc_interrupt_enable(idev->thc, false);
+ synchronize_irq(idev->irq);
+ cancel_work_sync(&idev->recover_work);
+ thc_ipts_dma_stop(idev);
+
if (idev->running)
thc_ipts_touch_set(idev, false);
[Severity: High]
Does thc_ipts_port_stop() unmap and free the DMA buffers while the
controller and THC sequencer can still DMA into them?
thc_interrupt_enable(idev->thc, false) only masks host interrupt delivery
in THC_M_PRT_INT_EN_OFFSET; it does not stop the touch controller or
quiesce the THC hardware sequencer. Calling thc_ipts_dma_stop() before
thc_ipts_touch_set(idev, false), asserting THC_M_PRT_CONTROL_DEVRST, and
calling thc_ipts_quiesce(idev, true) unconfigures the PRD registers and
frees the scatter-gather buffers and coherent PRD tables while RxDMA2 can
still be active, risking a DMA write to freed pages or an IOMMU fault.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261114154611.1683519-1-emfox@debian.org?part=2