[PATCH v3 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports

[PATCH v3 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports

[Emfox Zhou]

The THC ports on Tiger Lake, Lakefield, Alder Lake and Raptor Lake run in
IPTS mode: plain SPI with a private framing instead of HIDSPI, which is
all intel-thc-hid handles today. This adds a backend for them next to
intel-quickspi and intel-quicki2c, on top of the same core, and is based
on v7.4-rc2, the first tag with the hid_safe_input_report() it uses.

Register, DMA and interrupt work goes through the exported intel-thc API,
except in four places where the core has no call for it: the reset,
port-supported and quiesce-state bits of THC_M_PRT_CONTROL are accessed
directly. Nothing outside the new directory changes except Kconfig and
Makefile.

The hardware facts - port IDs, configuration block layout, frame codes
and header sizes - come from the ithc driver linux-surface ships, at
https://github.com/quo/ithc-linux 6d53c9c (dual licensed GPL-2.0 /
BSD-2-Clause); no code is taken from it. The layout, the frame format and
the ordering between quiesce, interrupt configuration and the DMA engines
are the parts most likely to need scrutiny; each is argued in the commit
message that relies on it. All of it was confirmed on a Surface Pro 8 with
a 045e:0c37 controller.

The first four patches each leave a state that builds and loads: port
bring-up, DMA and interrupts, HID registration, system sleep. The fifth
documents the controller behaviour the driver relies on, none of which is
written down anywhere else.

On that machine the touchscreen and the pen work through hidraw and iptsd
after patch 3, and three suspend/resume cycles (pm_test=devices) bring
them back after patch 4, with the daemon restarting on its own.

The interrupt is the legacy pin: these ports put requester ID 01:05.0 in
their MSI messages, which is neither their own function nor expressible
as a DMA alias, so with interrupt remapping on the MSI is rejected and the
port stays silent. The pin is shared on the PCH, hence IRQF_SHARED; MSI
is the fallback where no pin is routed.

Ports are matched by PCI ID. These four generations are not in the HIDSPI
ID table, so nothing else claims them, and a port that firmware drives as
HIDSPI has no driver today: this driver gives up with -EIO when the $TIC
tag never appears.

Report handling stops at HID core on purpose: the controller is the one
declaring itself a HID device, so the descriptor comes from it, Windows
exposes the same port the same way, and turning the heatmap into contacts
is user space work that already exists.

Some concerns:

  - Runtime PM is not implemented, and only the Tiger Lake ports were
    tested; Lakefield, Alder Lake and Raptor Lake share the code path but
    have not been run. LTR configuration comes with runtime PM.

  - The SPI clock is one of ten gears the core can program, 7.8 to
    50 MHz, and the core rounds the request down. A controller that asks
    for less than the slowest gear is raised to 7.8 MHz, warned about
    once and carried on with.

  - System sleep takes the HID device down and brings it back, so the
    daemon restarts and asks for the report stream again: the controller
    does not keep that state, and replaying one set feature would not
    cover the several the daemon sends. It also means the touchscreen
    cannot wake the machine.

Link to v2:
https://lore.kernel.org/r/20261114170740.2068179-1-emfox@debian.org

Link to v1:
https://lore.kernel.org/r/20261114154611.1683519-1-emfox@debian.org


Changes in v3, from the automated review of v2:

  - The running flag is set where the touch engine is enabled, in the
    patch that enables it. Patch 1 read the flag on teardown but only
    patch 2 assigned it, so a bisect landed on a kernel that reset the
    controller with the touch engine still powered.

  - A recovery in flight is device state now, not a local variable in
    the threaded handler. An interrupt from another function on the
    shared line woke a second handler that read frames while the work
    item was rewriting the ring, and unmasked the port mid-reset.

  - The shared line is requested with IRQF_COND_ONESHOT instead of
    IRQF_ONESHOT. A shared line only accepts handlers that agree on
    oneshot, and the other functions on this pin do not use it, so the
    request failed with -EBUSY as soon as one of them had probed first.

  - An interrupt with no cause of ours is reported as not handled: from
    the thread when the status is empty, and from the hardirq while the
    port is stopping or has no DMA. Both only after the port has been
    masked, so the spurious detector cannot see a storm from here.

  - The drain loop stopped one pass short: thc_interrupt_handler()
    returns early on four causes and the data path is a fifth call, and a
    cause that arrives while frames are read adds one more. The bound is
    now a pass per cause plus the one that reports none.

  - The feature reply is taken under a lock shared with the sender. A
    request that timed out is dropped, and the sender may already be
    asking for another report: checking the flags and copying the answer
    has to be one step, or the new request completes with the old one.

  - Teardown takes req_lock before masking the line and resetting the
    controller. A request already inside thc_dma_write still needs the
    interrupt that completes it, and with the line masked first it waited
    out its timeout holding the bus lock.

Changes in v2, from the automated review of v1:

  - The recovery work item is initialised where it is introduced.
    Patch 2 used it and only patch 3 initialised it, which breaks the
    bisect.

  - A frame read that finds the ring empty no longer passes an
    uninitialised length to the frame decoder.

  - The port stays masked while the recovery item resets the engine,
    and the handler keeps draining the status registers after an error,
    so a queued transmit completion is acknowledged instead of timing
    out under the bus lock.

  - Teardown stops and quiesces the controller before the DMA memory is
    released, and takes req_lock around the release: a send already
    inside thc_dma_write finishes, later ones fail on ->stopping.

  - SET_REPORT returns the byte count, as hidraw expects; zero was
    returned before.

  - A late or empty report descriptor no longer overwrites the buffer
    being parsed, and an empty one is retried instead of registered.

  - Feature reports: empty answers are rejected, and the report ID in
    an answer is checked against the request. Verified on hardware,
    where GET_FEATURE for reports 5, 6, 9 and 17 echoes its own ID.


Emfox Zhou (5):
  HID: intel-thc-hid: intel-ipts: bring up the port
  HID: intel-thc-hid: intel-ipts: receive reports
  HID: intel-thc-hid: intel-ipts: register the HID device
  HID: intel-thc-hid: intel-ipts: handle system sleep
  HID: intel-thc-hid: intel-ipts: document the IPTS mode

 Documentation/hid/intel-thc-hid.rst           |  95 ++-
 drivers/hid/intel-thc-hid/Kconfig             |  14 +
 drivers/hid/intel-thc-hid/Makefile            |   4 +
 drivers/hid/intel-thc-hid/intel-ipts/ipts.c   | 776 ++++++++++++++++++
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   | 196 +++++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   | 203 +++++
 6 files changed, 1287 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/ipts.c
 create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/ipts.h
 create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb

[PATCH v3 1/5] HID: intel-thc-hid: intel-ipts: bring up the port

[Emfox Zhou]

Ports on Tiger Lake, Lakefield, Alder Lake and Raptor Lake can run in
IPTS mode: instead of HIDSPI the port drives the touch controller over
plain SPI, and the controller answers with a 64 byte configuration
block at address zero that carries the vendor and product IDs, the
buffer sizes and the SPI timing it expects.

Add a port driver that does that much: select the SPI port, release the
controller from reset, read the configuration block over the existing
PIO path, apply the timing the controller asked for and enable the
touch engine. What the controller reports is printed, and removal puts
it back into reset.

Three things are not obvious from the register names:

  - The bulk data window is at address 0x1000, not at zero. It is set
    here because the DMA engines write there; left unset they write into
    the configuration block and the controller drops the frames without
    reporting an error.

  - Quiescing gates the whole data path, not only the interrupt. The
    port stays quiesced while the configuration block is read and the
    timing is applied, and is only released once that is done.

  - The edge detect bit belongs to the interrupt setup, but is set
    here: the first configuration read is unreliable without it.

Meteor Lake and newer IDs are deliberately absent: those ports are
already claimed by the HIDSPI driver.

Signed-off-by: Emfox Zhou <emfox@debian.org>
---
 drivers/hid/intel-thc-hid/Kconfig             |  14 ++
 drivers/hid/intel-thc-hid/Makefile            |   4 +
 drivers/hid/intel-thc-hid/intel-ipts/ipts.c   | 231 ++++++++++++++++++
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   | 132 ++++++++++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   | 125 ++++++++++
 5 files changed, 506 insertions(+)
 create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/ipts.c
 create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/ipts.h
 create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c

diff --git a/drivers/hid/intel-thc-hid/Kconfig b/drivers/hid/intel-thc-hid/Kconfig
index 9d74e53..3df645f 100644
--- a/drivers/hid/intel-thc-hid/Kconfig
+++ b/drivers/hid/intel-thc-hid/Kconfig
@@ -40,4 +40,18 @@ config INTEL_QUICKI2C
 
 	  Say Y/M here if you want to support Intel QuickI2C. If unsure, say N.
 
+config INTEL_IPTS
+	tristate "Intel Touch Host Controller IPTS mode"
+	depends on INTEL_THC_HID
+	help
+	  Support for THC ports in IPTS mode. Instead of HIDSPI these ports
+	  drive the touch controller over plain SPI and carry its reports in a
+	  private framing. This driver brings such a port up, reads the
+	  controller's configuration block and exposes the reports as a HID
+	  device, so that a decoder in user space can turn them into input
+	  events.
+
+	  Say Y/M here if you have an Intel touch controller that runs in IPTS
+	  mode. If unsure, say N.
+
 endmenu
diff --git a/drivers/hid/intel-thc-hid/Makefile b/drivers/hid/intel-thc-hid/Makefile
index f118225..0490095 100644
--- a/drivers/hid/intel-thc-hid/Makefile
+++ b/drivers/hid/intel-thc-hid/Makefile
@@ -20,4 +20,8 @@ intel-quicki2c-objs += intel-quicki2c/pci-quicki2c.o
 intel-quicki2c-objs += intel-quicki2c/quicki2c-hid.o
 intel-quicki2c-objs += intel-quicki2c/quicki2c-protocol.o
 
+obj-$(CONFIG_INTEL_IPTS) += intel-ipts.o
+intel-ipts-objs += intel-ipts/pci-ipts.o
+intel-ipts-objs += intel-ipts/ipts.o
+
 ccflags-y += -I $(src)/intel-thc
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 0000000..2644796
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Transport for THC ports in IPTS mode: the controller is driven over plain
+ * SPI and carries its HID reports in a private framing.
+ */
+
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include "intel-thc-hw.h"
+#include "ipts.h"
+
+/* Bulk transfers have no address phase, so the SPI opcode is zero. */
+#define THC_IPTS_SPI_OPCODE		0
+
+/* Slowest clock the core can program, used until the controller is read. */
+#define THC_IPTS_BOOT_SPI_HZ		THC_IPTS_SPI_MIN_HZ
+
+/* Time the controller needs after reset before it answers on the bus. */
+#define THC_IPTS_BOOT_MS		100
+
+static int thc_ipts_read_cfg(struct thc_ipts *idev)
+{
+	int ret, try;
+
+	for (try = 0; try < THC_IPTS_CFG_TRIES; try++) {
+		u32 got = 0;
+
+		memset(&idev->cfg, 0, sizeof(idev->cfg));
+		ret = thc_tic_pio_read(idev->thc, 0, THC_IPTS_CFG_SIZE, &got,
+				       (u32 *)&idev->cfg);
+		if (!ret && got == THC_IPTS_CFG_SIZE &&
+		    le32_to_cpu(idev->cfg.device_id) == THC_IPTS_TIC_ID)
+			return 0;
+
+		if (try + 1 < THC_IPTS_CFG_TRIES)
+			msleep(THC_IPTS_CFG_RETRY_MS);
+	}
+
+	dev_err(idev->dev,
+		"controller did not answer, last id 0x%08x, error %d\n",
+		le32_to_cpu(idev->cfg.device_id), ret ?: -EIO);
+
+	return ret ?: -EIO;
+}
+
+/*
+ * The core warns when the port is already in the requested state, so sample
+ * the bit first; it is a plain register bit and safe to read here.
+ */
+static int thc_ipts_quiesce(struct thc_ipts *idev, bool quiesce)
+{
+	u32 ctrl;
+	int ret;
+
+	ret = regmap_read(idev->thc->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
+			  &ctrl);
+	if (ret)
+		return ret;
+
+	if (!!(ctrl & THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_EN) == quiesce)
+		return 0;
+
+	return thc_interrupt_quiesce(idev->thc, quiesce);
+}
+
+static int thc_ipts_spi_set(struct thc_ipts *idev, u32 hz, u32 mode)
+{
+	int ret;
+
+	/* No multi-packet transfers in this mode, so both sizes stay zero. */
+	ret = thc_spi_read_config(idev->thc, hz, mode, THC_IPTS_SPI_OPCODE, 0);
+	if (ret)
+		return ret;
+
+	return thc_spi_write_config(idev->thc, hz, mode, THC_IPTS_SPI_OPCODE,
+				     0, 0);
+}
+
+/*
+ * The core picks the fastest gear that does not exceed the request, so the
+ * clock is rounded down; a request below the slowest gear is raised to it,
+ * because running faster than asked is the one direction that breaks it.
+ */
+static void thc_ipts_spi_from_cfg(struct thc_ipts *idev, u32 *hz, u32 *mode)
+{
+	u32 cfg = le32_to_cpu(idev->cfg.spi_cfg);
+	u32 div = THC_IPTS_SPI_CLKDIV(cfg);
+
+	if (!div) {
+		/* Nothing valid asks for this; use the slowest gear. */
+		dev_warn_once(idev->dev, "controller asks for divisor 0\n");
+		*hz = THC_IPTS_SPI_MIN_HZ;
+	} else {
+		*hz = THC_IPTS_SPI_BASE_HZ / div;
+		if (cfg & THC_IPTS_SPI_CLKDIV8)
+			*hz /= THC_IPTS_SPI_CLKDIV8_DIV;
+	}
+
+	if (*hz < THC_IPTS_SPI_MIN_HZ) {
+		dev_warn_once(idev->dev,
+			      "controller asks for %u Hz, slowest is %u Hz\n",
+			      *hz, THC_IPTS_SPI_MIN_HZ);
+		*hz = THC_IPTS_SPI_MIN_HZ;
+	}
+
+	if (cfg & THC_IPTS_SPI_QUAD)
+		*mode = THC_QUAD_IO;
+	else if (cfg & THC_IPTS_SPI_DUAL)
+		*mode = THC_DUAL_IO;
+	else
+		*mode = THC_SINGLE_IO;
+}
+
+static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
+{
+	u32 touch = le32_to_cpu(idev->cfg.touch_cfg);
+
+	touch &= ~(THC_IPTS_TOUCH_POWER_MASK | THC_IPTS_TOUCH_ENABLE |
+		   THC_IPTS_TOUCH_HID_REPORT);
+	touch |= THC_IPTS_TOUCH_POWER(enable ? THC_IPTS_TOUCH_POWER_ON : 0);
+	if (enable)
+		touch |= THC_IPTS_TOUCH_ENABLE | THC_IPTS_TOUCH_HID_REPORT;
+
+	return thc_tic_pio_write(idev->thc, THC_IPTS_CFG_TOUCH, sizeof(touch),
+				 &touch);
+}
+
+/**
+ * thc_ipts_port_start - bring one port up
+ * @idev: port to start
+ */
+int thc_ipts_port_start(struct thc_ipts *idev)
+{
+	u32 ctrl, hz, mode;
+	int ret;
+
+	ret = thc_ipts_quiesce(idev, true);
+	if (ret)
+		return ret;
+
+	ret = thc_port_select(idev->thc, THC_PORT_TYPE_SPI);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(idev->thc->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
+			  &ctrl);
+	if (ret)
+		return ret;
+	if (!(ctrl & THC_M_PRT_CONTROL_PORT_SUPPORTED))
+		return -ENODEV;
+
+	/* Without this the engines write into the configuration block. */
+	thc_spi_input_output_address_config(idev->thc, 0, THC_IPTS_BULK_ADDR,
+					    THC_IPTS_BULK_ADDR);
+
+	ret = thc_ipts_spi_set(idev, THC_IPTS_BOOT_SPI_HZ, THC_SINGLE_IO);
+	if (ret)
+		return ret;
+
+	/* The first configuration read is unreliable without edge detect. */
+	thc_int_trigger_type_select(idev->thc, true);
+
+	/* Release the controller from reset; it answers only after this. */
+	ret = regmap_set_bits(idev->thc->thc_regmap,
+			      THC_M_PRT_CONTROL_OFFSET,
+			      THC_M_PRT_CONTROL_DEVRST);
+	if (ret)
+		return ret;
+
+	msleep(THC_IPTS_BOOT_MS);
+
+	ret = thc_ipts_read_cfg(idev);
+	if (ret)
+		return ret;
+
+	idev->vendor = le16_to_cpu(idev->cfg.vendor);
+	idev->product = le16_to_cpu(idev->cfg.product);
+	idev->max_rx = THC_IPTS_RX_BUF_SIZE(le32_to_cpu(idev->cfg.buf_sizes));
+	idev->max_tx = THC_IPTS_TX_BUF_SIZE(le32_to_cpu(idev->cfg.buf_sizes));
+	if (idev->max_rx <= THC_IPTS_RX_HDR_SIZE ||
+	    idev->max_tx <= THC_IPTS_TX_HDR_SIZE) {
+		dev_err(idev->dev, "buffer sizes %zu/%zu are too small\n",
+			idev->max_rx, idev->max_tx);
+		return -ENODEV;
+	}
+	thc_ipts_spi_from_cfg(idev, &hz, &mode);
+	ret = thc_ipts_spi_set(idev, hz, mode);
+	if (ret) {
+		dev_warn(idev->dev,
+			 "cannot follow the SPI timing, going slow\n");
+		ret = thc_ipts_spi_set(idev, THC_IPTS_BOOT_SPI_HZ,
+				       THC_SINGLE_IO);
+		if (ret)
+			return ret;
+	} else {
+		dev_dbg(idev->dev, "SPI clock asked for %u Hz, rounded down\n",
+			hz);
+	}
+
+	ret = thc_ipts_quiesce(idev, false);
+	if (ret)
+		return ret;
+
+	ret = thc_ipts_touch_set(idev, true);
+	if (ret)
+		return ret;
+	idev->running = true;
+
+	return 0;
+}
+
+/**
+ * thc_ipts_port_stop - tear one port down
+ * @idev: port to stop
+ */
+void thc_ipts_port_stop(struct thc_ipts *idev)
+{
+	if (!idev->thc)
+		return;
+
+	if (idev->running)
+		thc_ipts_touch_set(idev, false);
+
+	idev->running = false;
+	regmap_clear_bits(idev->thc->thc_regmap,
+			  THC_M_PRT_CONTROL_OFFSET,
+			  THC_M_PRT_CONTROL_DEVRST);
+	thc_ipts_quiesce(idev, true);
+}
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
new file mode 100644
index 0000000..f0493dc
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -0,0 +1,132 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _THC_IPTS_H_
+#define _THC_IPTS_H_
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/build_bug.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/types.h>
+
+#include "intel-thc-dev.h"
+
+/* Configuration block the controller keeps at address zero. */
+#define THC_IPTS_CFG_SIZE		64
+#define THC_IPTS_CFG_TOUCH		0x0c
+#define THC_IPTS_TIC_ID		0x43495424	/* "$TIC" */
+
+/* Bulk data window. The DMA engines have to be pointed here, not at zero. */
+#define THC_IPTS_BULK_ADDR		0x1000
+
+/* Ports in IPTS mode, one pair per generation. */
+#define PCI_DEVICE_ID_INTEL_THC_LKF_PORT1	0x98d0
+#define PCI_DEVICE_ID_INTEL_THC_LKF_PORT2	0x98d1
+#define PCI_DEVICE_ID_INTEL_THC_TGL_LP_PORT1	0xa0d0
+#define PCI_DEVICE_ID_INTEL_THC_TGL_LP_PORT2	0xa0d1
+#define PCI_DEVICE_ID_INTEL_THC_TGL_H_PORT1	0x43d0
+#define PCI_DEVICE_ID_INTEL_THC_TGL_H_PORT2	0x43d1
+#define PCI_DEVICE_ID_INTEL_THC_ADL_S_PORT1	0x7ad8
+#define PCI_DEVICE_ID_INTEL_THC_ADL_S_PORT2	0x7ad9
+#define PCI_DEVICE_ID_INTEL_THC_ADL_P_PORT1	0x51d0
+#define PCI_DEVICE_ID_INTEL_THC_ADL_P_PORT2	0x51d1
+#define PCI_DEVICE_ID_INTEL_THC_ADL_M_PORT1	0x54d0
+#define PCI_DEVICE_ID_INTEL_THC_ADL_M_PORT2	0x54d1
+#define PCI_DEVICE_ID_INTEL_THC_RPL_S_PORT1	0x7a58
+#define PCI_DEVICE_ID_INTEL_THC_RPL_S_PORT2	0x7a59
+
+/*
+ * The block is partly filled while the controller is still starting, so a
+ * missing $TIC tag is retried rather than treated as an error.
+ */
+#define THC_IPTS_CFG_TRIES		10
+#define THC_IPTS_CFG_RETRY_MS		100
+
+/* Bytes of the configuration block this driver interprets. */
+#define THC_IPTS_CFG_KNOWN		40
+
+/*
+ * Layout of the configuration block, as far as this driver reads it. Only the
+ * fields marked as used are read; the others are here to keep the offsets of
+ * the block, which the driver hands to the core as a whole.
+ */
+struct thc_ipts_cfg {
+	__le32	irq_cause;	/* pending interrupt causes */
+	__le32	error;		/* protocol error flags, not used */
+	__le32	buf_sizes;	/* Rx/Tx sizes, see *_BUF_SIZE() */
+	__le32	touch_cfg;	/* touch engine control, writable */
+	__le32	touch_state;	/* touch engine state, not used */
+	__le32	device_id;	/* THC_IPTS_TIC_ID after a good read */
+	__le32	spi_cfg;	/* SPI timing the controller asks for */
+	__le16	vendor;		/* vendor ID reported to HID */
+	__le16	product;	/* product ID reported to HID */
+	__le32	revision;	/* product revision */
+	__le32	fw_version;	/* firmware version word */
+	u8	rest[THC_IPTS_CFG_SIZE - THC_IPTS_CFG_KNOWN];
+};
+
+static_assert(sizeof(struct thc_ipts_cfg) == THC_IPTS_CFG_SIZE);
+
+/* The core is handed the struct itself, so the offsets are part of the deal. */
+static_assert(offsetof(struct thc_ipts_cfg, buf_sizes) == 0x08);
+static_assert(offsetof(struct thc_ipts_cfg, touch_cfg) == 0x0c);
+static_assert(offsetof(struct thc_ipts_cfg, spi_cfg) == 0x18);
+static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
+
+/* Buffer sizes are stored as (size / 64) - 1, Rx in bits 13:0, Tx in 23:14. */
+#define THC_IPTS_DMA_UNIT		64
+#define THC_IPTS_RX_BUF_SIZE(val)	\
+	((size_t)(FIELD_GET(GENMASK(13, 0), (val)) + 1) * THC_IPTS_DMA_UNIT)
+#define THC_IPTS_TX_BUF_SIZE(val)	\
+	((size_t)(FIELD_GET(GENMASK(23, 14), (val)) + 1) * THC_IPTS_DMA_UNIT)
+
+#define THC_IPTS_TOUCH_ENABLE		BIT(0)
+#define THC_IPTS_TOUCH_HID_REPORT	BIT(2)
+#define THC_IPTS_TOUCH_POWER_ON	3
+#define THC_IPTS_TOUCH_POWER_MASK	GENMASK(5, 3)
+#define THC_IPTS_TOUCH_POWER(val)	\
+	FIELD_PREP(THC_IPTS_TOUCH_POWER_MASK, (val))
+
+#define THC_IPTS_SPI_CLKDIV(val)	FIELD_GET(GENMASK(3, 1), (val))
+#define THC_IPTS_SPI_CLKDIV8		BIT(4)
+#define THC_IPTS_SPI_CLKDIV8_DIV	8
+#define THC_IPTS_SPI_DUAL		BIT(6)
+#define THC_IPTS_SPI_QUAD		BIT(7)
+#define THC_IPTS_SPI_BASE_HZ		125000000
+/* Slowest clock the core can program. */
+#define THC_IPTS_SPI_MIN_HZ		7812500
+
+/*
+ * Every frame carries a code and a length in front of the payload. Frames
+ * from the controller have a long header, frames to it a short one, and only
+ * the second one is padded to a four byte boundary.
+ */
+#define THC_IPTS_RX_HDR_SIZE		64
+#define THC_IPTS_TX_HDR_SIZE		8
+
+/**
+ * struct thc_ipts - state of one THC port in IPTS mode
+ * @dev: device used for resources and logging
+ * @thc: shared THC core context
+ * @cfg: configuration block last read from the controller
+ * @vendor: vendor ID the controller reports for its HID device
+ * @product: product ID the controller reports for its HID device
+ * @max_rx: largest frame the controller may send, in bytes
+ * @max_tx: largest frame the driver may send, in bytes
+ * @running: the controller answered and is out of reset
+ */
+struct thc_ipts {
+	struct device		*dev;
+	struct thc_device	*thc;
+	struct thc_ipts_cfg	cfg;
+	u16			vendor;
+	u16			product;
+	size_t			max_rx;
+	size_t			max_tx;
+	bool			running;
+};
+
+int thc_ipts_port_start(struct thc_ipts *idev);
+void thc_ipts_port_stop(struct thc_ipts *idev);
+
+#endif /* _THC_IPTS_H_ */
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
new file mode 100644
index 0000000..48046cd
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI glue for the Intel Touch Host Controller in IPTS mode. Ports that
+ * the HIDSPI driver already claims are deliberately left out of the ID table.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/workqueue.h>
+
+#include "ipts.h"
+
+static const struct pci_device_id thc_ipts_pci_ids[] = {
+	/* Lakefield */
+	{PCI_DEVICE_DATA(INTEL, THC_LKF_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_LKF_PORT2, NULL)},
+	/* Tiger Lake */
+	{PCI_DEVICE_DATA(INTEL, THC_TGL_LP_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_TGL_LP_PORT2, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_TGL_H_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_TGL_H_PORT2, NULL)},
+	/* Alder Lake */
+	{PCI_DEVICE_DATA(INTEL, THC_ADL_S_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_ADL_S_PORT2, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_ADL_P_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_ADL_P_PORT2, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_ADL_M_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_ADL_M_PORT2, NULL)},
+	/* Raptor Lake */
+	{PCI_DEVICE_DATA(INTEL, THC_RPL_S_PORT1, NULL)},
+	{PCI_DEVICE_DATA(INTEL, THC_RPL_S_PORT2, NULL)},
+	{}
+};
+MODULE_DEVICE_TABLE(pci, thc_ipts_pci_ids);
+
+static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct thc_ipts *idev;
+	void __iomem *mem;
+	int ret;
+
+	idev = devm_kzalloc_obj(&pci->dev, *idev);
+	if (!idev)
+		return -ENOMEM;
+
+	idev->dev = &pci->dev;
+	pci_set_drvdata(pci, idev);
+
+	ret = pcim_enable_device(pci);
+	if (ret) {
+		dev_err(idev->dev,
+			"cannot enable the PCI function, error %d\n", ret);
+		return ret;
+	}
+
+	pci_set_master(pci);
+
+	mem = pcim_iomap_region(pci, 0, KBUILD_MODNAME);
+	ret = PTR_ERR_OR_ZERO(mem);
+	if (ret) {
+		dev_err(idev->dev, "cannot map the register space, error %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = dma_set_mask_and_coherent(idev->dev, DMA_BIT_MASK(64));
+	if (ret)
+		ret = dma_set_mask_and_coherent(idev->dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(idev->dev, "no usable DMA configuration, error %d\n",
+			ret);
+		return ret;
+	}
+
+	idev->thc = thc_dev_init(idev->dev, mem);
+	if (IS_ERR(idev->thc))
+		return PTR_ERR(idev->thc);
+
+	ret = thc_ipts_port_start(idev);
+	if (ret) {
+		thc_ipts_port_stop(idev);
+		return ret;
+	}
+
+	dev_info(idev->dev, "controller %04x:%04x rev %u, firmware 0x%08x\n",
+		 idev->vendor, idev->product, le32_to_cpu(idev->cfg.revision),
+		 le32_to_cpu(idev->cfg.fw_version));
+	dev_info(idev->dev, "spi cfg 0x%08x, rx %zu tx %zu bytes\n",
+		 le32_to_cpu(idev->cfg.spi_cfg), idev->max_rx, idev->max_tx);
+
+	return 0;
+}
+
+static void thc_ipts_remove(struct pci_dev *pci)
+{
+	struct thc_ipts *idev = pci_get_drvdata(pci);
+
+	thc_ipts_port_stop(idev);
+	pci_clear_master(pci);
+}
+
+static void thc_ipts_shutdown(struct pci_dev *pci)
+{
+	struct thc_ipts *idev = pci_get_drvdata(pci);
+
+	if (idev)
+		thc_ipts_port_stop(idev);
+}
+
+static struct pci_driver thc_ipts_driver = {
+	.name		= "intel-ipts",
+	.id_table	= thc_ipts_pci_ids,
+	.probe		= thc_ipts_probe,
+	.remove		= thc_ipts_remove,
+	.shutdown	= thc_ipts_shutdown,
+	/* Bring-up waits for the controller, up to a few seconds. */
+	.driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+};
+module_pci_driver(thc_ipts_driver);
+
+MODULE_AUTHOR("Emfox Zhou <emfox@debian.org>");
+MODULE_DESCRIPTION("IPTS transport for the Intel Touch Host Controller");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("INTEL_THC");

[PATCH v3 2/5] HID: intel-thc-hid: intel-ipts: receive reports

[Emfox Zhou]

Take the port one step further: configure the two DMA engines, one for
the frames the controller sends and one for the frames the host sends,
and register the interrupt that reports them.

Interrupt remapping rejects the MSI requests these ports send: the
requester ID in them is 01:05.0, neither the port's own function nor a
valid DMA alias for it, so only the legacy interrupt pin works once
remapping is on. MSI is the fallback for a platform that does not route
that pin. The pin is shared with other functions on the PCH, and such a
line only takes handlers that agree on oneshot, so the request asks for
it only if a sharer uses it. The hardirq masks the port itself, so a
foreign interrupt costs at most a few register reads in the thread, and
is reported as not handled.

Transfer errors are recovered from a work item and not from the threaded
handler: thc_rxdma_reset() takes the bus lock, and a pending
thc_dma_write() holds that lock while it waits for the very completion the
handler has to deliver. The port stays masked while that item runs and the
handler ignores the line, so the reset cannot race with a frame being read:
masking alone would not help, as another function on the shared pin can
still wake the handler.

Every frame carries a code and a length in front of the payload. The
receive side decodes that header and logs each frame.

Three details are worth spelling out:

  - The interrupt configuration has to be applied before the DMA
    engines are started: it programs the device interrupt type field to
    the invalid value, and starting the receive engine then replaces that
    field with the type it expects. The other order would leave the
    invalid value in place.

  - The line is masked while the threaded handler runs, and an event
    that arrives in that window leaves nothing behind but its status
    bit. The handler therefore drains the status registers instead of
    trusting a single delivery: a write completion taken for granted
    this way makes the next transfer wait for a completion that never
    comes, and the port stays unusable until it is reset.

  - Bulk transfers have no address phase, so the SPI opcode is zero.
    The opcodes the HIDSPI path uses make this controller ignore the
    frames altogether.

Signed-off-by: Emfox Zhou <emfox@debian.org>
---
 drivers/hid/intel-thc-hid/intel-ipts/ipts.c   | 243 ++++++++++++++++++
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   |  17 ++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   |  44 +++-
 3 files changed, 303 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
index 2644796..ac0074a 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -5,8 +5,13 @@
  */
 
 #include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
 #include <linux/regmap.h>
+#include <linux/sizes.h>
 #include <linux/string.h>
+#include <linux/unaligned.h>
+#include <linux/workqueue.h>
 
 #include "intel-thc-hw.h"
 #include "ipts.h"
@@ -20,6 +25,13 @@
 /* Time the controller needs after reset before it answers on the bus. */
 #define THC_IPTS_BOOT_MS		100
 
+/*
+ * thc_interrupt_handler() handles one cause per call, so a burst
+ * needs one call per cause plus the one that reports none. The
+ * bound only stops a controller that never falls quiet.
+ */
+#define THC_IPTS_IRQ_PASSES		(THC_UNKNOWN_INT + 1)
+
 static int thc_ipts_read_cfg(struct thc_ipts *idev)
 {
 	int ret, try;
@@ -127,6 +139,209 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
 				 &touch);
 }
 
+static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
+{
+	size_t capacity = idev->max_rx - THC_IPTS_RX_HDR_SIZE;
+	u32 code, size;
+
+	if (len < THC_IPTS_RX_HDR_SIZE)
+		return;
+
+	code = get_unaligned_le32(idev->rx_frame);
+	/* The header may claim more than what actually arrived. */
+	size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4),
+		     capacity);
+	size = min_t(u32, size, len - THC_IPTS_RX_HDR_SIZE);
+
+	dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
+}
+
+static void thc_ipts_dma_stop(struct thc_ipts *idev)
+{
+	if (!idev->dma_ready)
+		return;
+
+	WRITE_ONCE(idev->dma_ready, false);
+	thc_dma_unconfigure(idev->thc);
+	thc_dma_release(idev->thc);
+}
+
+static int thc_ipts_dma_start(struct thc_ipts *idev)
+{
+	int ret;
+
+	/*
+	 * Allocated once, so each buffer is retried on its own: a failed
+	 * first start must not leave a NULL frame buffer for a resume.
+	 */
+	if (!idev->rx_frame) {
+		/*
+		 * The core rounds the packet size up to a page when it sets
+		 * up the engines, so the buffer has to be that large.
+		 */
+		idev->rx_frame = devm_kzalloc(idev->dev,
+					      round_up(idev->max_rx, SZ_4K),
+					      GFP_KERNEL);
+		if (!idev->rx_frame)
+			return -ENOMEM;
+	}
+
+	ret = thc_dma_set_max_packet_sizes(idev->thc, 0, idev->max_rx,
+					   idev->max_tx, 0);
+	if (ret)
+		return ret;
+
+	ret = thc_dma_allocate(idev->thc);
+	if (ret)
+		return ret;
+
+	ret = thc_dma_configure(idev->thc);
+	if (ret) {
+		thc_dma_release(idev->thc);
+		return ret;
+	}
+
+	WRITE_ONCE(idev->dma_ready, true);
+
+	return 0;
+}
+
+irqreturn_t thc_ipts_hardirq(int irq, void *data)
+{
+	struct thc_ipts *idev = data;
+
+	/*
+	 * The pin is shared and level triggered: mask first, or it stays
+	 * asserted on the way out. The cause itself can only be read in the
+	 * thread.
+	 */
+	thc_interrupt_enable(idev->thc, false);
+
+	/* A reset in flight owns the engine, and the interrupt is ours. */
+	if (READ_ONCE(idev->recovering))
+		return IRQ_HANDLED;
+
+	/*
+	 * Nothing of ours is pending in these two states and the port is
+	 * masked: the interrupt belongs to another function on the line.
+	 */
+	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
+		return IRQ_NONE;
+
+	return IRQ_WAKE_THREAD;
+}
+
+static int thc_ipts_read_frames(struct thc_ipts *idev)
+{
+	for (;;) {
+		int finished = 0;
+		size_t len = 0;
+
+		if (thc_rxdma_read(idev->thc, THC_RXDMA2, idev->rx_frame,
+				   &len, &finished)) {
+			dev_warn_once(idev->dev,
+				      "cannot read a frame from the engine\n");
+			return -EIO;
+		}
+
+		/* Nothing was read: the ring was empty and len is still 0. */
+		if (!len)
+			break;
+
+		thc_ipts_handle_frame(idev, len);
+		if (finished)
+			break;
+	}
+
+	return 0;
+}
+
+/*
+ * 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)) {
+		/* Nothing left: keep the port masked until it restarts. */
+		dev_err(idev->dev, "cannot reset the receive engine\n");
+		return;
+	}
+
+	WRITE_ONCE(idev->recovering, false);
+	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;
+	irqreturn_t ret = IRQ_NONE;	/* no cause of ours yet */
+	bool need_recover = false;
+	int pass;
+
+	/* A reset is in flight; the engine state is not ours to read. */
+	if (READ_ONCE(idev->recovering))
+		return IRQ_HANDLED;
+
+	/*
+	 * 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_PASSES; 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;
+
+		ret = IRQ_HANDLED;
+		if (mask & (BIT(THC_FATAL_ERR_INT) | BIT(THC_TXN_ERR_INT))) {
+			dev_err(idev->dev,
+				"transfer error, resetting the engine\n");
+			need_recover = 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 (need_recover)
+			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))
+			need_recover = true;
+	}
+
+	/*
+	 * The work item resets the engine and unmasks the port again; a frame
+	 * read while it rewrites the ring would corrupt the descriptors, so
+	 * ->recovering keeps the handler off the line until it is done.
+	 */
+	if (need_recover) {
+		WRITE_ONCE(idev->recovering, true);
+		schedule_work(&idev->recover_work);
+		return IRQ_HANDLED;
+	}
+
+	if (!READ_ONCE(idev->stopping))
+		thc_interrupt_enable(idev->thc, true);
+
+	/* An interrupt with no cause of ours is not ours to claim. */
+	return ret;
+}
+
 /**
  * thc_ipts_port_start - bring one port up
  * @idev: port to start
@@ -136,6 +351,9 @@ int thc_ipts_port_start(struct thc_ipts *idev)
 	u32 ctrl, hz, mode;
 	int ret;
 
+	WRITE_ONCE(idev->stopping, false);
+	WRITE_ONCE(idev->recovering, false);
+
 	ret = thc_ipts_quiesce(idev, true);
 	if (ret)
 		return ret;
@@ -208,6 +426,19 @@ int thc_ipts_port_start(struct thc_ipts *idev)
 		return ret;
 	idev->running = true;
 
+	/*
+	 * thc_interrupt_config() programs the interrupt type field to the
+	 * invalid value; starting RXDMA2 then sets it to the type the engine
+	 * expects, so the configuration has to happen first.
+	 */
+	thc_interrupt_config(idev->thc);
+
+	ret = thc_ipts_dma_start(idev);
+	if (ret)
+		return ret;
+
+	thc_interrupt_enable(idev->thc, true);
+
 	return 0;
 }
 
@@ -220,6 +451,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);
+
+	/* Stop the controller before the buffers it writes into go away. */
 	if (idev->running)
 		thc_ipts_touch_set(idev, false);
 
@@ -228,4 +469,6 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
 			  THC_M_PRT_CONTROL_OFFSET,
 			  THC_M_PRT_CONTROL_DEVRST);
 	thc_ipts_quiesce(idev, true);
+
+	thc_ipts_dma_stop(idev);
 }
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
index f0493dc..f233d9c 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/types.h>
+#include <linux/wait.h>
 
 #include "intel-thc-dev.h"
 
@@ -108,24 +109,40 @@ static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
  * struct thc_ipts - state of one THC port in IPTS mode
  * @dev: device used for resources and logging
  * @thc: shared THC core context
+ * @irq: interrupt line of the port
  * @cfg: configuration block last read from the controller
  * @vendor: vendor ID the controller reports for its HID device
  * @product: product ID the controller reports for its HID device
  * @max_rx: largest frame the controller may send, in bytes
  * @max_tx: largest frame the driver may send, in bytes
+ * @rx_frame: buffer for one received frame, including its header
+ * @recover_work: resets the receive engine after a transfer error
  * @running: the controller answered and is out of reset
+ * @dma_ready: DMA engines are configured and may be used
+ * @recovering: a reset is queued or running, ignore the interrupt
+ * @stopping: the port is being torn down, ignore the interrupt
  */
 struct thc_ipts {
 	struct device		*dev;
 	struct thc_device	*thc;
+	int			irq;
 	struct thc_ipts_cfg	cfg;
 	u16			vendor;
 	u16			product;
 	size_t			max_rx;
 	size_t			max_tx;
+	u8			*rx_frame;
+	struct work_struct	recover_work;
 	bool			running;
+	bool			dma_ready;
+	bool			recovering;
+	bool			stopping;
 };
 
+irqreturn_t thc_ipts_hardirq(int irq, void *data);
+irqreturn_t thc_ipts_thread(int irq, void *data);
+void thc_ipts_recover(struct work_struct *work);
+
 int thc_ipts_port_start(struct thc_ipts *idev);
 void thc_ipts_port_stop(struct thc_ipts *idev);
 
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
index 48046cd..161bee9 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -46,6 +46,7 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	idev->dev = &pci->dev;
 	pci_set_drvdata(pci, idev);
+	INIT_WORK(&idev->recover_work, thc_ipts_recover);
 
 	ret = pcim_enable_device(pci);
 	if (ret) {
@@ -77,10 +78,42 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	if (IS_ERR(idev->thc))
 		return PTR_ERR(idev->thc);
 
+	/*
+	 * Interrupt remapping rejects the MSI requests these ports send, so
+	 * take the legacy pin (shared with other functions on the PCH, hence
+	 * IRQF_SHARED); MSI is only for a platform that routes no pin.
+	 */
+	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_INTX);
+	if (ret < 0) {
+		dev_warn(idev->dev, "no interrupt pin, falling back to MSI\n");
+		ret = pci_alloc_irq_vectors(pci, 1, 1,
+					    PCI_IRQ_MSI | PCI_IRQ_MSIX);
+	}
+	if (ret < 0) {
+		dev_err(idev->dev, "no interrupt vector, error %d\n", ret);
+		return ret;
+	}
+
+	idev->irq = pci_irq_vector(pci, 0);
+
+	/*
+	 * A shared line only takes handlers that agree on oneshot, and the
+	 * hardirq masks the port itself, so ask for it only if a sharer has.
+	 */
+	ret = devm_request_threaded_irq(idev->dev, idev->irq, thc_ipts_hardirq,
+					thc_ipts_thread,
+					IRQF_SHARED | IRQF_COND_ONESHOT,
+					KBUILD_MODNAME, idev);
+	if (ret) {
+		dev_err(idev->dev, "cannot get the interrupt, error %d\n", ret);
+		goto err_vectors;
+	}
+
 	ret = thc_ipts_port_start(idev);
 	if (ret) {
 		thc_ipts_port_stop(idev);
-		return ret;
+		devm_free_irq(idev->dev, idev->irq, idev);
+		goto err_vectors;
 	}
 
 	dev_info(idev->dev, "controller %04x:%04x rev %u, firmware 0x%08x\n",
@@ -90,6 +123,11 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		 le32_to_cpu(idev->cfg.spi_cfg), idev->max_rx, idev->max_tx);
 
 	return 0;
+
+err_vectors:
+	pci_free_irq_vectors(pci);
+
+	return ret;
 }
 
 static void thc_ipts_remove(struct pci_dev *pci)
@@ -97,6 +135,10 @@ static void thc_ipts_remove(struct pci_dev *pci)
 	struct thc_ipts *idev = pci_get_drvdata(pci);
 
 	thc_ipts_port_stop(idev);
+
+	/* The handler has to go before the vector it is bound to is freed. */
+	devm_free_irq(idev->dev, idev->irq, idev);
+	pci_free_irq_vectors(pci);
 	pci_clear_master(pci);
 }
 

[PATCH v3 3/5] HID: intel-thc-hid: intel-ipts: register the HID device

[Emfox Zhou]

Register a HID device for the port and let it own the handshake: the
report descriptor is not read once at probe and kept, it is asked for
over the transport whenever the HID core parses the device, and feature
reports the core asks for are fetched the same way.

A feature answer arrives as a frame of its own while the request waits, so
it can land after that request timed out and another one started. The
report ID in it is checked against the request, and the flags and the
buffer are updated together under a lock: matching an answer to a request
and consuming it have to be one step.

Teardown takes the request lock before it masks the line: a request already
inside thc_dma_write needs the interrupt that completes it, and with the
line masked first it would wait out its timeout holding the bus lock.

Input reports go straight to the input layer, and the raw report stream
is available to user space through hidraw. That is where decoding
belongs: the reports carry a heatmap, and turning it into contacts
needs floating point arithmetic the kernel cannot do.

The controller starts streaming once user space asks for it, through a
set feature request on report 5. The driver does not do that on its
own, the daemon that opens hidraw sends it.

Signed-off-by: Emfox Zhou <emfox@debian.org>
---
 drivers/hid/intel-thc-hid/intel-ipts/ipts.c   | 348 ++++++++++++++++--
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   |  47 +++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   |   8 +-
 3 files changed, 376 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
index ac0074a..bf75f8c 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -4,7 +4,9 @@
  * SPI and carries its HID reports in a private framing.
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
+#include <linux/hid.h>
 #include <linux/interrupt.h>
 #include <linux/minmax.h>
 #include <linux/regmap.h>
@@ -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);
+}
+
+static void thc_ipts_descriptor(struct thc_ipts *idev, const u8 *data,
+				size_t len)
+{
+	/* One answer per request: a late reply must not overwrite it. */
+	if (len <= THC_IPTS_DESC_PREAMBLE || READ_ONCE(idev->have_desc))
+		return;
+
+	len = min(len - THC_IPTS_DESC_PREAMBLE, idev->max_rx);
+	memcpy(idev->desc, data + THC_IPTS_DESC_PREAMBLE, len);
+	idev->desc_len = len;
+	WRITE_ONCE(idev->have_desc, true);
+	wake_up(&idev->wq);
+}
+
+/* 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
+	 * ID tells an answer to an earlier request 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;
+	WRITE_ONCE(idev->feature_done, true);
+	spin_unlock_irq(&idev->feature_lock);
+
+	wake_up(&idev->wq);
+}
+
 static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
 {
+	const u8 *payload = idev->rx_frame + THC_IPTS_RX_HDR_SIZE;
 	size_t capacity = idev->max_rx - THC_IPTS_RX_HDR_SIZE;
 	u32 code, size;
 
@@ -149,11 +222,31 @@ static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
 
 	code = get_unaligned_le32(idev->rx_frame);
 	/* The header may claim more than what actually arrived. */
-	size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4),
-		     capacity);
+	size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4), capacity);
 	size = min_t(u32, size, len - THC_IPTS_RX_HDR_SIZE);
 
-	dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
+	switch (code) {
+	case THC_IPTS_RX_INPUT: {
+		struct hid_device *hdev = READ_ONCE(idev->hid);
+
+		if (hdev)
+			hid_safe_input_report(hdev, HID_INPUT_REPORT,
+					      (u8 *)payload, capacity, size, 1);
+		break;
+	}
+	case THC_IPTS_RX_DESC:
+		thc_ipts_descriptor(idev, payload, size);
+		break;
+	case THC_IPTS_RX_FEATURE:
+		thc_ipts_feature(idev, payload, size);
+		break;
+	case THC_IPTS_RX_RESET:
+		dev_warn(idev->dev, "controller asked for a reset\n");
+		break;
+	default:
+		dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
+		break;
+	}
 }
 
 static void thc_ipts_dma_stop(struct thc_ipts *idev)
@@ -182,10 +275,24 @@ static int thc_ipts_dma_start(struct thc_ipts *idev)
 		idev->rx_frame = devm_kzalloc(idev->dev,
 					      round_up(idev->max_rx, SZ_4K),
 					      GFP_KERNEL);
-		if (!idev->rx_frame)
-			return -ENOMEM;
 	}
+	if (!idev->tx_frame)
+		idev->tx_frame = devm_kzalloc(idev->dev,
+					      round_up(idev->max_tx, 4),
+					      GFP_KERNEL);
+	if (!idev->desc)
+		idev->desc = devm_kzalloc(idev->dev, idev->max_rx, GFP_KERNEL);
+	if (!idev->feature)
+		idev->feature = devm_kzalloc(idev->dev, idev->max_rx,
+					     GFP_KERNEL);
+	if (!idev->rx_frame || !idev->tx_frame || !idev->desc || !idev->feature)
+		return -ENOMEM;
 
+	/*
+	 * The sizes the controller advertises are the largest frame it sends
+	 * and the largest it accepts, which is what the engines need; the core
+	 * rounds them up to a page and derives the PRD table sizes from them.
+	 */
 	ret = thc_dma_set_max_packet_sizes(idev->thc, 0, idev->max_rx,
 					   idev->max_tx, 0);
 	if (ret)
@@ -342,6 +449,196 @@ irqreturn_t thc_ipts_thread(int irq, void *data)
 	return ret;
 }
 
+static int thc_ipts_hid_parse(struct hid_device *hdev)
+{
+	struct thc_ipts *idev = hdev->driver_data;
+	unsigned long timeout = msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS);
+	u64 zero = 0;
+	int ret;
+	int try;
+
+	guard(mutex)(&idev->req_lock);
+
+	for (try = 0; try < THC_IPTS_REQ_TRIES; try++) {
+		WRITE_ONCE(idev->have_desc, false);
+
+		/* The controller ignores the eight byte argument. */
+		ret = thc_ipts_send(idev, THC_IPTS_TX_GET_DESC, &zero,
+				    sizeof(zero));
+		if (ret)
+			return ret;
+
+		if (wait_event_timeout(idev->wq, READ_ONCE(idev->have_desc),
+				       timeout) &&
+		    idev->desc_len > 0)
+			return hid_parse_report(hdev, idev->desc,
+						idev->desc_len);
+	}
+
+	dev_err(idev->dev,
+		"controller did not return a report descriptor in %d tries\n",
+		try);
+
+	return -ETIMEDOUT;
+}
+
+/*
+ * 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);
+	int try;
+
+	guard(mutex)(&idev->req_lock);
+
+	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);
+
+	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),
+				       timeout)) {
+			ret = 0;
+			break;
+		}
+
+		ret = -ETIMEDOUT;
+	}
+
+	/* Give up on the answer: a reply still in flight is dropped. */
+	spin_lock_irq(&idev->feature_lock);
+	idev->feature_pending = false;
+	spin_unlock_irq(&idev->feature_lock);
+
+	if (ret) {
+		dev_err(idev->dev, "no feature report %u after %d tries\n",
+			reportnum, try);
+		return ret;
+	}
+
+	/* The payload is the report as user space writes it: ID first. */
+	len = min(len, idev->feature_len);
+	if (!len)
+		return -EPROTO;
+	memcpy(buf, idev->feature, len);
+
+	return len;
+}
+
+static int thc_ipts_hid_raw_request(struct hid_device *hdev,
+				    unsigned char reportnum, __u8 *buf,
+				    size_t len, unsigned char rtype,
+				    int reqtype)
+{
+	struct thc_ipts *idev = hdev->driver_data;
+	u32 code;
+	int ret;
+
+	if (!buf || !len)
+		return -EINVAL;
+
+	if (reqtype == HID_REQ_GET_REPORT && rtype == HID_FEATURE_REPORT)
+		return thc_ipts_get_feature(idev, reportnum, buf, len);
+
+	if (reqtype != HID_REQ_SET_REPORT ||
+	    (rtype != HID_OUTPUT_REPORT && rtype != HID_FEATURE_REPORT))
+		return -EOPNOTSUPP;
+
+	code = rtype == HID_OUTPUT_REPORT ? THC_IPTS_TX_OUTPUT :
+					    THC_IPTS_TX_SET_FEATURE;
+
+	guard(mutex)(&idev->req_lock);
+
+	ret = thc_ipts_send(idev, code, buf, len);
+	if (ret)
+		return ret;
+
+	/* raw_request returns the byte count, which hidraw forwards. */
+	return (int)len;
+}
+
+/*
+ * The HID core calls all four of these unconditionally, and this transport
+ * has nothing to do when a user opens or closes the report stream.
+ */
+static int thc_ipts_hid_start(struct hid_device *hdev)
+{
+	return 0;
+}
+
+static void thc_ipts_hid_stop(struct hid_device *hdev)
+{
+}
+
+static int thc_ipts_hid_open(struct hid_device *hdev)
+{
+	return 0;
+}
+
+static void thc_ipts_hid_close(struct hid_device *hdev)
+{
+}
+
+static const struct hid_ll_driver thc_ipts_ll_driver = {
+	.start		= thc_ipts_hid_start,
+	.stop		= thc_ipts_hid_stop,
+	.open		= thc_ipts_hid_open,
+	.close		= thc_ipts_hid_close,
+	.parse		= thc_ipts_hid_parse,
+	.raw_request	= thc_ipts_hid_raw_request,
+};
+
+static void thc_ipts_hid_remove(struct thc_ipts *idev)
+{
+	if (!READ_ONCE(idev->hid))
+		return;
+
+	hid_destroy_device(idev->hid);
+	WRITE_ONCE(idev->hid, NULL);
+}
+
+static int thc_ipts_hid_add(struct thc_ipts *idev)
+{
+	struct hid_device *hdev;
+	int ret;
+
+	hdev = hid_allocate_device();
+	if (IS_ERR(hdev))
+		return PTR_ERR(hdev);
+
+	hdev->ll_driver = &thc_ipts_ll_driver;
+	hdev->bus = BUS_PCI;
+	hdev->vendor = idev->vendor;
+	hdev->product = idev->product;
+	hdev->version = le32_to_cpu(idev->cfg.revision) & 0xffff;
+	hdev->dev.parent = idev->dev;
+	hdev->driver_data = idev;
+	snprintf(hdev->name, sizeof(hdev->name), "intel-ipts %04X:%04X",
+		 idev->vendor, idev->product);
+	strscpy(hdev->phys, dev_name(idev->dev), sizeof(hdev->phys));
+
+	ret = hid_add_device(hdev);
+	if (ret) {
+		hid_destroy_device(hdev);
+		return ret;
+	}
+
+	WRITE_ONCE(idev->hid, hdev);
+
+	return 0;
+}
+
 /**
  * thc_ipts_port_start - bring one port up
  * @idev: port to start
@@ -439,7 +736,7 @@ int thc_ipts_port_start(struct thc_ipts *idev)
 
 	thc_interrupt_enable(idev->thc, true);
 
-	return 0;
+	return thc_ipts_hid_add(idev);
 }
 
 /**
@@ -452,23 +749,28 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
 		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.
+	 * Requests first: one inside thc_dma_write needs the interrupt that
+	 * completes it, so it finishes before the line is masked. A new one
+	 * sees ->stopping under the same lock and backs out.
 	 */
-	WRITE_ONCE(idev->stopping, true);
-	thc_interrupt_enable(idev->thc, false);
-	synchronize_irq(idev->irq);
-	cancel_work_sync(&idev->recover_work);
-
-	/* Stop the controller before the buffers it writes into go away. */
-	if (idev->running)
-		thc_ipts_touch_set(idev, false);
-
-	idev->running = false;
-	regmap_clear_bits(idev->thc->thc_regmap,
-			  THC_M_PRT_CONTROL_OFFSET,
-			  THC_M_PRT_CONTROL_DEVRST);
-	thc_ipts_quiesce(idev, true);
+	scoped_guard(mutex, &idev->req_lock) {
+		WRITE_ONCE(idev->stopping, true);
+		thc_interrupt_enable(idev->thc, false);
+		synchronize_irq(idev->irq);
+		cancel_work_sync(&idev->recover_work);
+
+		/* Stop the controller before its buffers go away. */
+		if (idev->running)
+			thc_ipts_touch_set(idev, false);
+
+		idev->running = false;
+		regmap_clear_bits(idev->thc->thc_regmap,
+				  THC_M_PRT_CONTROL_OFFSET,
+				  THC_M_PRT_CONTROL_DEVRST);
+		thc_ipts_quiesce(idev, true);
+
+		thc_ipts_dma_stop(idev);
+	}
 
-	thc_ipts_dma_stop(idev);
+	thc_ipts_hid_remove(idev);
 }
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
index f233d9c..6d5a5bf 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -6,7 +6,10 @@
 #include <linux/bits.h>
 #include <linux/build_bug.h>
 #include <linux/device.h>
+#include <linux/hid.h>
 #include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/wait.h>
 
@@ -43,6 +46,13 @@
 #define THC_IPTS_CFG_TRIES		10
 #define THC_IPTS_CFG_RETRY_MS		100
 
+/* A report descriptor arrives with eight bytes of preamble in front of it. */
+#define THC_IPTS_DESC_PREAMBLE	8
+
+/* Retry budget for the two handshakes user space can trigger. */
+#define THC_IPTS_REQ_TRIES		6
+#define THC_IPTS_REQ_TIMEOUT_MS	300
+
 /* Bytes of the configuration block this driver interprets. */
 #define THC_IPTS_CFG_KNOWN		40
 
@@ -105,6 +115,17 @@ static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
 #define THC_IPTS_RX_HDR_SIZE		64
 #define THC_IPTS_TX_HDR_SIZE		8
 
+/* Frame codes. The same numbers mean different things per direction. */
+#define THC_IPTS_RX_INPUT		3
+#define THC_IPTS_RX_FEATURE		4
+#define THC_IPTS_RX_DESC		5
+#define THC_IPTS_RX_RESET		7
+
+#define THC_IPTS_TX_SET_FEATURE	3
+#define THC_IPTS_TX_GET_FEATURE	4
+#define THC_IPTS_TX_OUTPUT		5
+#define THC_IPTS_TX_GET_DESC		7
+
 /**
  * struct thc_ipts - state of one THC port in IPTS mode
  * @dev: device used for resources and logging
@@ -116,6 +137,19 @@ static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
  * @max_rx: largest frame the controller may send, in bytes
  * @max_tx: largest frame the driver may send, in bytes
  * @rx_frame: buffer for one received frame, including its header
+ * @tx_frame: buffer for one frame to send, including its header
+ * @desc: copy of the report descriptor, past its preamble
+ * @desc_len: length of @desc
+ * @have_desc: a descriptor arrived and has not been consumed yet
+ * @feature: buffer a feature report is collected in
+ * @feature_len: length of the report in @feature
+ * @feature_id: report ID that was asked for, checked against the answer
+ * @feature_pending: a feature report was asked for and has not arrived yet
+ * @feature_done: a feature report arrived and has not been consumed yet
+ * @feature_lock: protects the feature report buffer and its flags
+ * @wq: waited on for a report descriptor or a feature report
+ * @req_lock: serialises request frames and the feature handshake
+ * @hid: HID device the reports are handed to, once it has been registered
  * @recover_work: resets the receive engine after a transfer error
  * @running: the controller answered and is out of reset
  * @dma_ready: DMA engines are configured and may be used
@@ -132,6 +166,19 @@ struct thc_ipts {
 	size_t			max_rx;
 	size_t			max_tx;
 	u8			*rx_frame;
+	u8			*tx_frame;
+	u8			*desc;
+	size_t			desc_len;
+	bool			have_desc;
+	u8			*feature;
+	size_t			feature_len;
+	u8			feature_id;
+	bool			feature_pending;
+	bool			feature_done;
+	spinlock_t		feature_lock;	/* feature report handshake */
+	wait_queue_head_t	wq;
+	struct mutex		req_lock;	/* request frames */
+	struct hid_device	*hid;
 	struct work_struct	recover_work;
 	bool			running;
 	bool			dma_ready;
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
index 161bee9..8f8db21 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -45,15 +45,15 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return -ENOMEM;
 
 	idev->dev = &pci->dev;
+	mutex_init(&idev->req_lock);
+	spin_lock_init(&idev->feature_lock);
+	init_waitqueue_head(&idev->wq);
 	pci_set_drvdata(pci, idev);
 	INIT_WORK(&idev->recover_work, thc_ipts_recover);
 
 	ret = pcim_enable_device(pci);
-	if (ret) {
-		dev_err(idev->dev,
-			"cannot enable the PCI function, error %d\n", ret);
+	if (ret)
 		return ret;
-	}
 
 	pci_set_master(pci);
 

[PATCH v3 4/5] HID: intel-thc-hid: intel-ipts: handle system sleep

[Emfox Zhou]

System sleep takes the port down and builds it up again rather than
keeping the HID device across it: the controller forgets the streaming
mode user space asked it for, and hidraw offers no way to tell user
space to ask again. The device goes away and comes back, so the daemon
watching it restarts and repeats the request.

Signed-off-by: Emfox Zhou <emfox@debian.org>
---
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
index 8f8db21..81a4614 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -142,6 +142,41 @@ static void thc_ipts_remove(struct pci_dev *pci)
 	pci_clear_master(pci);
 }
 
+/*
+ * The controller forgets the streaming mode and hidraw cannot ask user space
+ * to repeat itself, so the port is taken down and rebuilt: the HID device
+ * comes back and the daemon restarts with it.
+ */
+static int thc_ipts_suspend(struct device *dev)
+{
+	struct thc_ipts *idev = dev_get_drvdata(dev);
+
+	if (!idev)
+		return -ENODEV;
+
+	thc_ipts_port_stop(idev);
+
+	return 0;
+}
+
+static int thc_ipts_resume(struct device *dev)
+{
+	struct thc_ipts *idev = dev_get_drvdata(dev);
+	int ret;
+
+	if (!idev)
+		return -ENODEV;
+
+	ret = thc_ipts_port_start(idev);
+	if (ret)
+		thc_ipts_port_stop(idev);
+
+	return ret;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(thc_ipts_pm_ops, thc_ipts_suspend,
+				thc_ipts_resume);
+
 static void thc_ipts_shutdown(struct pci_dev *pci)
 {
 	struct thc_ipts *idev = pci_get_drvdata(pci);
@@ -156,6 +191,7 @@ static struct pci_driver thc_ipts_driver = {
 	.probe		= thc_ipts_probe,
 	.remove		= thc_ipts_remove,
 	.shutdown	= thc_ipts_shutdown,
+	.driver.pm	= pm_ptr(&thc_ipts_pm_ops),
 	/* Bring-up waits for the controller, up to a few seconds. */
 	.driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 };

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

[Emfox Zhou]

The THC documentation only covers HIDSPI and HIDI2C. Ports from before
Meteor Lake run in a third mode, and the intel-ipts driver is
written against behaviour that is not described anywhere in tree.

Record what the driver depends on: the layout of the configuration
block, the frame codes and header sizes, the bulk data window, and the
order in which quiesce, interrupt configuration and the DMA engines
have to be set up.

Signed-off-by: Emfox Zhou <emfox@debian.org>
---
 Documentation/hid/intel-thc-hid.rst | 95 ++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/Documentation/hid/intel-thc-hid.rst b/Documentation/hid/intel-thc-hid.rst
index 8b378c5..0f6328a 100644
--- a/Documentation/hid/intel-thc-hid.rst
+++ b/Documentation/hid/intel-thc-hid.rst
@@ -590,7 +590,100 @@ To debug THC, event tracing mechanism is used. To enable debug logs::
   echo 1 > /sys/kernel/debug/tracing/events/intel_thc/enable
   cat /sys/kernel/debug/tracing/trace
 
-7. Reference
+7. IPTS Mode
+============
+
+THC also supports IPTS mode, the private Intel Precise Touch and Stylus
+protocol. The port drives the touch controller over plain SPI and moves the
+data through the same DMA engines, but the framing is private and the
+parameters come from the controller itself instead of ACPI. The
+``intel-ipts`` module handles ports in that mode, which is what Tiger Lake,
+Lakefield, Alder Lake and Raptor Lake hardware use.
+
+This section describes how the driver uses those ports. It is not a
+specification: everything below comes from observing the controller and is
+limited to what this driver relies on.
+
+7.1 Configuration Block
+-----------------------
+
+At SPI address 0 the controller exposes a 64 byte block. The driver reads it
+over the PIO path after releasing the controller from reset, and retries
+until the tag ``$TIC`` (``0x43495424``) appears, because the block is only
+partially filled while the controller is still starting.
+
+Fields the driver uses:
+
+======  ==========================================================
+Offset  Meaning
+======  ==========================================================
+0x00    pending interrupt causes
+0x08    buffer sizes, ``(size / 64) - 1``; Rx bits 13:0, Tx 23:14
+0x0c    touch engine control, writable
+0x14    controller tag, ``$TIC``
+0x18    SPI timing the controller asks for
+0x1c    vendor and product ID reported to HID
+0x20    product revision
+0x24    firmware version word
+======  ==========================================================
+
+The touch engine control word carries an enable bit, a bit asking for HID
+reports instead of raw data, and a three bit power state. The driver enables
+it with reports and full power, and clears it again when the port is shut
+down.
+
+7.2 Frames
+----------
+
+Both directions carry a code and a length in front of the payload. Frames
+from the controller use a 64 byte header, frames to it an 8 byte header, and
+only the latter has the payload padded to a four byte boundary.
+
+==== ========================== =======================
+Code Received from controller   Sent to controller
+==== ========================== =======================
+3    input report               set feature
+4    feature report             get feature
+5    report descriptor          output report
+7    reset request              get report descriptor
+==== ========================== =======================
+
+A report descriptor comes back with eight bytes of preamble that the driver
+skips.
+
+7.3 Data Window
+---------------
+
+Bulk transfers go through address ``0x1000`` of the controller, not through
+address 0. The DMA engines have to be pointed there explicitly, otherwise
+they write into the configuration block and the controller drops the frames
+without reporting an error. The input header address is zero: that is where
+the controller writes the interrupt cause word, at the start of the
+configuration block.
+
+7.4 Enabling and Flow Control
+-----------------------------
+
+The port has to be released from quiesce before the controller will send
+anything: quiescing gates the whole data path, not just the interrupt. The
+interrupt configuration programs the interrupt type field in the device to
+the invalid value, and starting the receive engine then replaces that field
+with the type it expects, so the two have to happen in that order.
+
+The controller starts streaming once user space asks for it, through a set
+feature request on report 5. The driver does not do that on its own; the
+daemon that opens hidraw sends it.
+
+7.5 What Is Left to User Space
+------------------------------
+
+The reports carry heatmap data, and turning that into contacts is a
+substantial image-processing job that already exists in user space. The
+driver therefore stops at the HID layer: the report descriptor comes from
+the controller and is not written by the driver, the report stream is
+available through hidraw, and ``iptsd`` decodes it into input events.
+
+8. Reference
 ============
 - HIDSPI: https://download.microsoft.com/download/c/a/0/ca07aef3-3e10-4022-b1e9-c98cea99465d/HidSpiProtocolSpec.pdf
 - HIDI2C: https://download.microsoft.com/download/7/d/d/7dd44bb7-2a7a-4505-ac1c-7227d3d96d5b/hid-over-i2c-protocol-spec-v1-0.docx

Re: [PATCH v3 1/5] HID: intel-thc-hid: intel-ipts: bring up the port

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: divisor 3 misses 41MHz gear
  in thc_ipts_spi_from_cfg()
--

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

HID: intel-thc-hid: intel-ipts: bring up the port

This commit adds a PCI port driver for Intel Touch Host Controller ports in
IPTS mode, reading the controller configuration block over PIO and applying
SPI timing to enable the touch engine.

> 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..264479630ff6e
> --- /dev/null
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> +static void thc_ipts_spi_from_cfg(struct thc_ipts *idev, u32 *hz, u32 *mode)
> +{
> +	u32 cfg = le32_to_cpu(idev->cfg.spi_cfg);
> +	u32 div = THC_IPTS_SPI_CLKDIV(cfg);
> +
> +	if (!div) {
> +		/* Nothing valid asks for this; use the slowest gear. */
> +		dev_warn_once(idev->dev, "controller asks for divisor 0\n");
> +		*hz = THC_IPTS_SPI_MIN_HZ;
> +	} else {
> +		*hz = THC_IPTS_SPI_BASE_HZ / div;
> +		if (cfg & THC_IPTS_SPI_CLKDIV8)
> +			*hz /= THC_IPTS_SPI_CLKDIV8_DIV;
> +	}
> +
> +	if (*hz < THC_IPTS_SPI_MIN_HZ) {
> +		dev_warn_once(idev->dev,
> +			      "controller asks for %u Hz, slowest is %u Hz\n",
> +			      *hz, THC_IPTS_SPI_MIN_HZ);
> +		*hz = THC_IPTS_SPI_MIN_HZ;
> +	}
[ ... ]
> +int thc_ipts_port_start(struct thc_ipts *idev)
> +{
[ ... ]
> +	thc_ipts_spi_from_cfg(idev, &hz, &mode);
> +	ret = thc_ipts_spi_set(idev, hz, mode);
> +	if (ret) {
> +		dev_warn(idev->dev,
> +			 "cannot follow the SPI timing, going slow\n");
> +		ret = thc_ipts_spi_set(idev, THC_IPTS_BOOT_SPI_HZ,
> +				       THC_SINGLE_IO);
> +		if (ret)
> +			return ret;
> +	} else {
> +		dev_dbg(idev->dev, "SPI clock asked for %u Hz, rounded down\n",
> +			hz);
> +	}

[Severity: Medium]
Does THC_IPTS_SPI_BASE_HZ / div miss the 41 MHz gear when the controller
requests divisor 3?

With THC_IPTS_SPI_BASE_HZ set to 125000000, integer division by 3 yields
41666666 Hz, which is 34 Hz below THC_SPI_FREQUENCY_41M (41666700):

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_spi_read_config() {
    ...
	/* 10M, 35M and 50M CLK need 1.5, 3.5 and 2.5 half divider */
	if ((freq_div == THC_SPI_FRQ_DIV_2 && spi_freq_val >=  THC_SPI_FREQUENCY_50M) ||
		(freq_div == THC_SPI_FRQ_DIV_3 && spi_freq_val <  THC_SPI_FREQUENCY_41M) ||
		(freq_div == THC_SPI_FRQ_DIV_1 && spi_freq_val <  THC_SPI_FREQUENCY_15M)) {
		regmap_write_bits(dev->thc_regmap, THC_M_PRT_SPI_DUTYC_CFG_OFFSET,
				  THC_M_PRT_SPI_DUTYC_CFG_SPI_TCRF_HALF_DIV_EN,
				  THC_M_PRT_SPI_DUTYC_CFG_SPI_TCRF_HALF_DIV_EN);

		regmap_write_bits(dev->thc_regmap, THC_M_PRT_SPARE_REG_OFFSET,
				  THC_M_PRT_SPARE_REG_SPI_CLK_INV_ENABLE,
				  THC_M_PRT_SPARE_REG_SPI_CLK_INV_ENABLE);
    ...
}

Because 41666666 < THC_SPI_FREQUENCY_41M, thc_spi_read_config() and
thc_spi_write_config() program the 3.5 half-divider (35.7 MHz) with SPI
clock inversion enabled instead of plain divisor 3.

Also, because thc_ipts_spi_from_cfg() clamps *hz to at least
THC_IPTS_SPI_MIN_HZ (7812500), thc_get_spi_freq_div_val() never returns
THC_SPI_FRQ_RESERVED, making the error fallback in thc_ipts_port_start()
unreachable.

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

Re: [PATCH v3 3/5] HID: intel-thc-hid: intel-ipts: register the HID device

[sashiko-bot]

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

Re: [PATCH v3 4/5] HID: intel-thc-hid: intel-ipts: handle system sleep

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: race in thc_ipts_port_stop()
  re-enables IRQ
--

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

HID: intel-thc-hid: intel-ipts: handle system sleep

This commit adds system sleep and resume callbacks to the intel-ipts PCI
driver by stopping and restarting the port across suspend and resume.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> index 8f8db21df8250..81a46149be2e9 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> @@ -142,6 +142,41 @@ static void thc_ipts_remove(struct pci_dev *pci)
>  	pci_clear_master(pci);
>  }
>  
> +/*
> + * The controller forgets the streaming mode and hidraw cannot ask user space
> + * to repeat itself, so the port is taken down and rebuilt: the HID device
> + * comes back and the daemon restarts with it.
> + */
> +static int thc_ipts_suspend(struct device *dev)
> +{
> +	struct thc_ipts *idev = dev_get_drvdata(dev);
> +
> +	if (!idev)
> +		return -ENODEV;
> +
> +	thc_ipts_port_stop(idev);

[Severity: Medium]
Can thc_ipts_port_stop() race with thc_ipts_thread() or thc_ipts_recover()
and leave THC interrupts enabled across suspend and resume?

CPU0                                     CPU1
thc_ipts_suspend()
  thc_ipts_port_stop()
                                         thc_ipts_thread()
                                           if (!READ_ONCE(idev->stopping))
    WRITE_ONCE(idev->stopping, true);
    thc_interrupt_enable(idev->thc, false);
                                             thc_interrupt_enable(idev->thc, true);
    synchronize_irq(idev->irq);
    cancel_work_sync(&idev->recover_work);

thc_ipts_port_stop() masks interrupts before synchronize_irq() and
cancel_work_sync(), and does not mask them again after those calls finish.
If the IRQ remains registered across suspend and resume and the controller
raises an interrupt during thc_ipts_touch_set() or during
thc_ipts_resume() -> thc_ipts_port_start() before idev->dma_ready is set,
thc_ipts_hardirq() sees idev->stopping or !idev->dma_ready and returns
IRQ_NONE without clearing the hardware status bits:

drivers/hid/intel-thc-hid/intel-ipts/ipts.c:thc_ipts_hardirq() {
	struct thc_ipts *idev = data;

	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
		return IRQ_NONE;
    ...
}

Calling thc_interrupt_enable(idev->thc, false) again after
synchronize_irq() and cancel_work_sync() in thc_ipts_port_stop() would
ensure interrupts stay masked once both contexts have exited.

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

Re: [PATCH v3 2/5] HID: intel-thc-hid: intel-ipts: receive reports

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: false IRQ_HANDLED in
  thc_ipts_hardirq()
- [Medium] HID: intel-thc-hid: intel-ipts: shared IRQ race unmasks
  thc_ipts_thread()
- [Medium] HID: intel-thc-hid: intel-ipts: stopping race in
  thc_ipts_port_stop()
- [Medium] HID: intel-thc-hid: intel-ipts: PRD descriptors not reset in
  thc_ipts_recover()
- [Medium] HID: intel-ipts: thc_dma_write() still stalls when recovery
  masks IRQ
--

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

HID: intel-thc-hid: intel-ipts: receive reports

This commit configures the RXDMA2 and TXDMA engines on the THC port,
registers a shared threaded interrupt handler to receive and log incoming
frames, and adds a recovery work item to reset RXDMA on transfer errors.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index 264479630ff6e..ac0074a85bfa4 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c

[ ... ]

> @@ -127,6 +139,209 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)

[ ... ]

> +irqreturn_t thc_ipts_hardirq(int irq, void *data)
> +{
> +	struct thc_ipts *idev = data;
> +
> +	/*
> +	 * The pin is shared and level triggered: mask first, or it stays
> +	 * asserted on the way out. The cause itself can only be read in the
> +	 * thread.
> +	 */
> +	thc_interrupt_enable(idev->thc, false);
> +
> +	/* A reset in flight owns the engine, and the interrupt is ours. */
> +	if (READ_ONCE(idev->recovering))
> +		return IRQ_HANDLED;
> +
> +	/*
> +	 * Nothing of ours is pending in these two states and the port is
> +	 * masked: the interrupt belongs to another function on the line.
> +	 */
> +	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
> +		return IRQ_NONE;
> +
> +	return IRQ_WAKE_THREAD;
> +}

[Severity: Medium]
Should thc_ipts_hardirq() and thc_ipts_thread() return IRQ_NONE instead of
IRQ_HANDLED when idev->recovering is set?

idev->recovering is only set in thc_ipts_thread() after thc_ipts_hardirq()
has already masked the port via thc_interrupt_enable(idev->thc, false),
and the port remains masked until thc_ipts_recover() finishes.  While
masked, this port cannot assert the shared INTx line, so any interrupt
arriving during recovery comes from another device on the shared pin.
Furthermore, if thc_rxdma_reset() fails in thc_ipts_recover() (or if
thc_ipts_port_stop() cancels recover_work), idev->recovering stays true
forever and causes thc_ipts_hardirq() to claim every subsequent interrupt
on the shared line, defeating spurious-interrupt detection.

[ ... ]

> +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;
> +	}

[Severity: Medium]
Does thc_rxdma_reset() leave mutated PRD descriptors in the RXDMA2 ring
after an error?

When RXDMA2 writes a frame into a PRD table, the hardware updates each
consumed entry's len, sets end_of_prd on the final entry, and sets the
PRD table hw_status.  During normal operation, thc_dma_read() calls
thc_copy_one_sgl_to_prd() to restore those fields before advancing the
write pointer.  When thc_ipts_thread() sets need_recover on a transfer
error and skips thc_ipts_read_frames() (or when read_dma_buffer() fails
before thc_copy_one_sgl_to_prd() runs), thc_rxdma_reset() only calls
thc_dma_unconfigure() and thc_dma_configure(), which reset the hardware
ring pointers without calling thc_copy_sgls_to_prd().  Those unconsumed
PRD tables retain truncated entry lengths and stale end_of_prd bits, so
later larger frames can trigger PRD buffer overrun errors.

> +
> +	WRITE_ONCE(idev->recovering, false);
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +}
> +
> +irqreturn_t thc_ipts_thread(int irq, void *data)
> +{

[ ... ]

> +	/*
> +	 * The work item resets the engine and unmasks the port again; a frame
> +	 * read while it rewrites the ring would corrupt the descriptors, so
> +	 * ->recovering keeps the handler off the line until it is done.
> +	 */
> +	if (need_recover) {
> +		WRITE_ONCE(idev->recovering, true);
> +		schedule_work(&idev->recover_work);
> +		return IRQ_HANDLED;
> +	}

[Severity: Medium]
Can a concurrent thc_dma_write() still stall until its timeout when
recovery masks the port?

thc_rxdma_reset() is deferred to recover_work because thc_dma_write()
holds dev->thc_bus_lock while waiting on dev->write_complete_wait for
thc_interrupt_handler() to process THC_M_PRT_WRITE_INT_STS_THC_WRDMA_CMPL_STATUS.
However, when need_recover is true, thc_ipts_thread() sets idev->recovering
and returns with the port interrupt still masked, and both
thc_ipts_hardirq() and thc_ipts_thread() ignore further interrupts until
thc_ipts_recover() finishes.  If an in-flight TxDMA transfer completes
after the polling loop in thc_ipts_thread() exits (or if a caller enters
thc_dma_write() and takes dev->thc_bus_lock before recover_work runs),
thc_ipts_recover() blocks on dev->thc_bus_lock while the TxDMA completion
interrupt stays masked, stalling thc_dma_write() for its full timeout.

> +
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +
> +	/* An interrupt with no cause of ours is not ours to claim. */
> +	return ret;
> +}

[Severity: Medium]
Can a shared interrupt race with port unmasking and run thc_ipts_thread()
while the port interrupt is enabled?

thc_ipts_probe() requests the interrupt with IRQF_SHARED |
IRQF_COND_ONESHOT, which leaves IRQS_ONESHOT unset when no earlier handler
on the line requested IRQF_ONESHOT, so thc_ipts_hardirq() can run on
another CPU while thc_ipts_thread() is executing:

CPU0                              CPU1
thc_ipts_thread()
  for (pass = 0; ...)
    thc_interrupt_handler()
                                  <hardirq: shared INTx>
                                  thc_ipts_hardirq()
                                    thc_interrupt_enable(thc, false)
                                    return IRQ_WAKE_THREAD
                                    (sets IRQTF_RUNTHREAD)
  thc_interrupt_enable(thc, true)
thc_ipts_thread()                 <-- runs with port unmasked

The same inverted state also occurs even with IRQS_ONESHOT if a shared
interrupt fires between WRITE_ONCE(idev->recovering, false) and
thc_interrupt_enable(idev->thc, true) in thc_ipts_recover(), or between
WRITE_ONCE(idev->dma_ready, true) and thc_interrupt_enable(idev->thc, true)
in thc_ipts_port_start().  If that second thc_ipts_thread() run hits an
error and takes the need_recover path, neither it nor thc_ipts_recover()
masks the port, so thc_rxdma_reset() runs with interrupts enabled on the
port.

[ ... ]

> @@ -220,6 +451,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);

[Severity: Medium]
Can thc_ipts_thread() or thc_ipts_recover() re-enable interrupts during
thc_ipts_port_stop()?

CPU0                              CPU1
thc_ipts_thread()
  if (!READ_ONCE(idev->stopping))
                                  thc_ipts_port_stop()
                                    WRITE_ONCE(idev->stopping, true)
                                    thc_interrupt_enable(thc, false)
    thc_interrupt_enable(thc, true) <-- re-enables IRQ during stop
                                    synchronize_irq(idev->irq)
                                    cancel_work_sync(&idev->recover_work)

Because thc_ipts_port_stop() masks the port before synchronize_irq() and
cancel_work_sync(), an in-flight thread or recovery work item that has
already passed its !READ_ONCE(idev->stopping) check will unmask the port
again, leaving THC_M_PRT_INT_EN_GBL_INT_EN set while the controller and
DMA buffers are torn down.  Calling thc_interrupt_enable(idev->thc, false)
after synchronize_irq() and cancel_work_sync() as well would close this
window.

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