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

[PATCH 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
stays as a 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 backs out with -ENODEV 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.


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: expose the port as a 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   | 681 ++++++++++++++++++
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   | 189 +++++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   | 197 +++++
 6 files changed, 1179 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 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.

Two things are not obvious from the register names:

  - The bulk data window is at address 0x1000, not at zero. It is
    programmed here already because the DMA engines added later 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 out-of-tree driver found the first configuration read
    unreliable until it is on.

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   | 230 ++++++++++++++++++
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   | 132 ++++++++++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   | 122 ++++++++++
 5 files changed, 502 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..b16dfdc
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -0,0 +1,230 @@
+// 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 stays 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) {
+		/* No valid configuration 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, the slowest gear 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;
+
+	/*
+	 * Set edge detect before the first config read: the out-of-tree
+	 * driver found that this makes reading the block reliable.
+	 */
+	thc_int_trigger_type_select(idev->thc, true);
+
+	/* Release the controller from reset; it answers only after this. */
+	ret = regmap_write_bits(idev->thc->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
+				THC_M_PRT_CONTROL_DEVRST, 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 controller's 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 by the core\n",
+			hz);
+	}
+
+	ret = thc_ipts_quiesce(idev, false);
+	if (ret)
+		return ret;
+
+	ret = thc_ipts_touch_set(idev, true);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Mask the line, then wait for a handler that may be holding a frame for the
+ * HID device, and only then take the engines down.
+ */
+/**
+ * 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_write_bits(idev->thc->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
+			  THC_M_PRT_CONTROL_DEVRST, 0);
+	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..b936ac1
--- /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 and Tx buffer sizes, see *_BUF_SIZE() */
+	__le32	touch_cfg;	/* touch engine control, the only writable one */
+	__le32	touch_state;	/* touch engine state, not used */
+	__le32	device_id;	/* THC_IPTS_TIC_ID once the read worked */
+	__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..e45f6ab
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -0,0 +1,122 @@
+// 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 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 stays as a fallback for a platform that does not
route that pin. It is shared with other functions on the PCH, so it is
requested with IRQF_SHARED and a foreign interrupt only costs the thread a
few register reads.

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.

Every frame carries a code and a length in front of the payload. The
receive side decodes that header. Frames are still only logged here,
the next patch hands them to the HID core.

Three details cost time on hardware:

  - 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 stays 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   | 193 ++++++++++++++++++
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   |  15 ++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   |  40 +++-
 3 files changed, 247 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 b16dfdc..8529b91 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,12 @@
 /* Time the controller needs after reset before it answers on the bus. */
 #define THC_IPTS_BOOT_MS		100
 
+/*
+ * The core returns after the first cause it finds, so one interrupt can need
+ * up to four reads: non-DMA, transaction error, overrun, then the data path.
+ */
+#define THC_IPTS_IRQ_CAUSES		4
+
 static int thc_ipts_read_cfg(struct thc_ipts *idev)
 {
 	int ret, try;
@@ -124,6 +135,162 @@ 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;
+
+	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 resume after a
+	 * failed first start must not end up with a NULL frame buffer.
+	 */
+	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;
+	}
+
+	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 this may be another function's interrupt; the
+	 * cause can only be read in the thread, so claim it. Mask first, or a
+	 * level triggered line stays asserted on the way out.
+	 */
+	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready)) {
+		thc_interrupt_enable(idev->thc, false);
+		return IRQ_HANDLED;
+	}
+
+	thc_interrupt_enable(idev->thc, false);
+
+	return IRQ_WAKE_THREAD;
+}
+
+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;
+}
+
+/*
+ * 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");
+}
+
+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);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * thc_ipts_port_start - bring one port up
  * @idev: port to start
@@ -133,6 +300,8 @@ int thc_ipts_port_start(struct thc_ipts *idev)
 	u32 ctrl, hz, mode;
 	int ret;
 
+	WRITE_ONCE(idev->stopping, false);
+
 	ret = thc_ipts_quiesce(idev, true);
 	if (ret)
 		return ret;
@@ -203,6 +372,20 @@ int thc_ipts_port_start(struct thc_ipts *idev)
 	ret = thc_ipts_touch_set(idev, true);
 	if (ret)
 		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 +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);
 
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
index b936ac1..3b5002c 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,38 @@ 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
+ * @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			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 e45f6ab..7e899df 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -74,10 +74,39 @@ 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, "cannot allocate an interrupt vector, error %d\n",
+			ret);
+		return ret;
+	}
+
+	idev->irq = pci_irq_vector(pci, 0);
+
+	ret = devm_request_threaded_irq(idev->dev, idev->irq, thc_ipts_hardirq,
+					thc_ipts_thread,
+					IRQF_ONESHOT | IRQF_SHARED,
+					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",
@@ -87,6 +116,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)
@@ -94,6 +128,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 3/5] HID: intel-thc-hid: intel-ipts: expose the port as a 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.

Input reports go straight to the input layer, and the raw report stream
stays 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   | 272 +++++++++++++++++-
 drivers/hid/intel-thc-hid/intel-ipts/ipts.h   |  42 +++
 .../hid/intel-thc-hid/intel-ipts/pci-ipts.c   |   7 +-
 3 files changed, 311 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
index 8529b91..5520620 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>
@@ -135,8 +137,62 @@ 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 (!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)
+{
+	if (len < THC_IPTS_DESC_PREAMBLE)
+		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 (!READ_ONCE(idev->feature_pending))
+		return;
+
+	len = min(len, idev->max_rx);
+	memcpy(idev->feature, data, len);
+	idev->feature_len = len;
+	WRITE_ONCE(idev->feature_pending, false);
+	WRITE_ONCE(idev->feature_done, true);
+	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;
 
@@ -145,11 +201,28 @@ 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:
+		if (idev->hid)
+			hid_safe_input_report(idev->hid, 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)
@@ -178,11 +251,25 @@ 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;
 
-	ret = thc_dma_set_max_packet_sizes(idev->thc, 0, idev->max_rx, idev->max_tx, 0);
+	/*
+	 * 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)
 		return ret;
 
@@ -291,6 +378,175 @@ irqreturn_t thc_ipts_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int thc_ipts_hid_parse(struct hid_device *hdev)
+{
+	struct thc_ipts *idev = hdev->driver_data;
+	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),
+				       msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS)))
+			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;
+	int try;
+
+	guard(mutex)(&idev->req_lock);
+
+	WRITE_ONCE(idev->feature_done, false);
+	WRITE_ONCE(idev->feature_pending, true);
+
+	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),
+				       msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS))) {
+			ret = 0;
+			break;
+		}
+
+		ret = -ETIMEDOUT;
+	}
+
+	WRITE_ONCE(idev->feature_pending, false);
+
+	if (ret) {
+		dev_err(idev->dev, "controller did not return feature %u in %d tries\n",
+			reportnum, try);
+		return ret;
+	}
+
+	/* The payload is the report as user space writes it, report ID first. */
+	len = min(len, idev->feature_len);
+	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;
+
+	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;
+
+	guard(mutex)(&idev->req_lock);
+
+	if (rtype == HID_OUTPUT_REPORT)
+		return thc_ipts_send(idev, THC_IPTS_TX_OUTPUT, buf, len);
+
+	return thc_ipts_send(idev, THC_IPTS_TX_SET_FEATURE, buf, 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 (!idev->hid)
+		return;
+
+	hid_destroy_device(idev->hid);
+	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;
+	}
+
+	idev->hid = hdev;
+
+	return 0;
+}
+
 /**
  * thc_ipts_port_start - bring one port up
  * @idev: port to start
@@ -387,7 +643,7 @@ int thc_ipts_port_start(struct thc_ipts *idev)
 
 	thc_interrupt_enable(idev->thc, true);
 
-	return 0;
+	return thc_ipts_hid_add(idev);
 }
 
 /*
@@ -413,6 +669,8 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
 	cancel_work_sync(&idev->recover_work);
 	thc_ipts_dma_stop(idev);
 
+	thc_ipts_hid_remove(idev);
+
 	if (idev->running)
 		thc_ipts_touch_set(idev, false);
 
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
index 3b5002c..be45f71 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -6,7 +6,9 @@
 #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/types.h>
 #include <linux/wait.h>
 
@@ -43,6 +45,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 +114,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 +136,17 @@ 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_pending: a feature report was asked for and has not arrived yet
+ * @feature_done: a feature report arrived and has not been consumed yet
+ * @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
@@ -131,6 +162,17 @@ 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;
+	bool			feature_pending;
+	bool			feature_done;
+	wait_queue_head_t	wq;
+	struct mutex		req_lock;	/* request frames are serialised */
+	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 7e899df..5160ab5 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -45,13 +45,14 @@ 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);
+	init_waitqueue_head(&idev->wq);
+	INIT_WORK(&idev->recover_work, thc_ipts_recover);
 	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);
+	if (ret)
 		return ret;
-	}
 
 	pci_set_master(pci);
 

[PATCH 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 5160ab5..06ba8ed 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -136,6 +136,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);
@@ -150,6 +185,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 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..bc1a015 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 was established by 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 stays 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 Stays in 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 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:
- [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 <emfox@debian.org>

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.

> 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

[ ... ]

> @@ -124,6 +135,162 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)

[ ... ]

> +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.

> +
> +/*
> + * 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().

> +
> +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.

> +
> +	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.

[ ... ]

> @@ -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

Re: [PATCH 3/5] HID: intel-thc-hid: intel-ipts: expose the port as a HID device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] HID: intel-thc-hid: zero return on SET_REPORT in
  thc_ipts_hid_raw_request()
- [High] HID: intel-thc-hid: DMA stopped before HID removal in
  thc_ipts_port_stop()
- [High] HID: intel-thc-hid: uninitialized frame len in
  thc_ipts_read_frames()
- [Medium] HID: intel-thc-hid: unvalidated feature report ID in
  thc_ipts_get_feature()
- [Medium] HID: intel-thc-hid: racy and zero-length descriptor in
  thc_ipts_descriptor()
--

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

HID: intel-thc-hid: intel-ipts: expose the port as a HID device

This commit registers a HID device for the IPTS port, adds TX frame helpers,
and implements report descriptor parsing, feature report handshakes, and
input report forwarding.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index 8529b91e063f7..55206205f2115 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> @@ -135,8 +137,62 @@ 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 (!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)
> +{
> +	if (len < THC_IPTS_DESC_PREAMBLE)
> +		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);
> +}

[Severity: Medium]
Can a delayed retry response in thc_ipts_descriptor() overwrite idev->desc
and idev->desc_len while thc_ipts_hid_parse() is passing them to
hid_parse_report()?

Unlike thc_ipts_feature(), thc_ipts_descriptor() does not check a pending
flag or whether idev->have_desc is already set. If the first attempt in
thc_ipts_hid_parse() times out and a second THC_IPTS_TX_GET_DESC request is
sent, the delayed reply to the first attempt wakes thc_ipts_hid_parse(),
while the second reply on the IRQ thread races with hid_parse_report():

CPU0                                     CPU1 (thc_ipts_thread)
thc_ipts_hid_parse()
  try 0: thc_ipts_send(GET_DESC)
  wait_event_timeout() -> times out
  try 1: thc_ipts_send(GET_DESC)
                                         thc_ipts_descriptor() (reply 0)
                                           WRITE_ONCE(idev->have_desc, true)
                                           wake_up(&idev->wq)
  hid_parse_report(hdev, idev->desc,
                   idev->desc_len)
                                         thc_ipts_descriptor() (reply 1)
                                           memcpy(idev->desc, ...)
                                           idev->desc_len = len;
                                             <-- data race

Also, checking len < THC_IPTS_DESC_PREAMBLE allows an 8-byte preamble-only
frame to set idev->desc_len = 0; hid_parse_report() then sets
hdev->dev_rdesc = ZERO_SIZE_PTR via kmemdup() and registers a broken HID
device with a 0-byte report descriptor instead of retrying.

> +
> +/* 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 (!READ_ONCE(idev->feature_pending))
> +		return;
> +
> +	len = min(len, idev->max_rx);
> +	memcpy(idev->feature, data, len);
> +	idev->feature_len = len;
> +	WRITE_ONCE(idev->feature_pending, false);
> +	WRITE_ONCE(idev->feature_done, true);
> +	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;
>  
> @@ -145,11 +201,28 @@ 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:
> +		if (idev->hid)
> +			hid_safe_input_report(idev->hid, 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;

[Severity: High]
Can thc_ipts_handle_frame() be called with an uninitialized len and
redispatch a stale idev->rx_frame buffer when the RxDMA2 ring is already
empty?

In thc_ipts_read_frames(), len is not initialized before calling
thc_rxdma_read(), and thc_ipts_handle_frame(idev, len) runs before checking
finished:

drivers/hid/intel-thc-hid/intel-ipts/ipts.c:thc_ipts_read_frames() {
	for (;;) {
		int finished = 0;
		size_t len;

		if (thc_rxdma_read(idev->thc, THC_RXDMA2, idev->rx_frame,
				   &len, &finished)) {
...
		}

		thc_ipts_handle_frame(idev, len);
		if (finished)
			break;
	}
}

In thc_dma_read(), if is_dma_buf_empty() is true on entry, *read_len is
left untouched while *read_finished is set to 1 and the function returns 0:

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)) {
		...
		*read_len = status;
		...
	}

	if (read_finished)
		*read_finished = is_dma_buf_empty(dev, read_config, &read_ptr, &write_ptr) ? 1 : 0;

	return 0;
}

If a second frame arrives after pass 0 of thc_ipts_thread() clears the
RxDMA2 EOF interrupt status bit in thc_interrupt_handler() and before pass
0's thc_ipts_read_frames() loop finishes draining the ring, pass 0 consumes
both frames, but the newly latched status bit causes pass 1 to call
thc_ipts_read_frames() on an empty ring. thc_rxdma_read() then returns 0
without setting len, and thc_ipts_handle_frame() uses the uninitialized
stack value to reprocess the previous frame in idev->rx_frame.

[ ... ]

> @@ -291,6 +378,175 @@ irqreturn_t thc_ipts_thread(int irq, void *data)
[ ... ]
> +/*
> + * 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;
> +	int try;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	WRITE_ONCE(idev->feature_done, false);
> +	WRITE_ONCE(idev->feature_pending, true);
> +
> +	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),
> +				       msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS))) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	WRITE_ONCE(idev->feature_pending, false);
> +
> +	if (ret) {
> +		dev_err(idev->dev, "controller did not return feature %u in %d tries\n",
> +			reportnum, try);
> +		return ret;
> +	}
> +
> +	/* The payload is the report as user space writes it, report ID first. */
> +	len = min(len, idev->feature_len);
> +	memcpy(buf, idev->feature, len);
> +
> +	return len;
> +}

[Severity: Medium]
Does thc_ipts_feature() need to verify that the incoming payload is
non-empty and starts with the requested reportnum?

If try 0 times out after 300 ms and try 1 sends a second request, a delayed
reply to try 0 completes try 1 while the second request remains in flight.
On a subsequent thc_ipts_get_feature() call for a different report ID, the
late reply from the earlier request is accepted by thc_ipts_feature() and
returned to the caller as if it matched the new reportnum. Also, if a
THC_IPTS_RX_FEATURE frame arrives with len == 0, thc_ipts_get_feature()
returns 0 without writing anything to buf.

> +
> +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;
> +
> +	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;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	if (rtype == HID_OUTPUT_REPORT)
> +		return thc_ipts_send(idev, THC_IPTS_TX_OUTPUT, buf, len);
> +
> +	return thc_ipts_send(idev, THC_IPTS_TX_SET_FEATURE, buf, len);
> +}

[Severity: High]
Should thc_ipts_hid_raw_request() return len on a successful
HID_REQ_SET_REPORT instead of the 0 returned by thc_ipts_send()?

thc_ipts_send() returns the result of thc_dma_write(), which is 0 on
success. However, struct hid_ll_driver.raw_request must return the number
of transferred bytes on success. In hidraw_send_report(), the return value
of __hid_hw_raw_request() is returned directly to user space for write()
and for HIDIOCSFEATURE and HIDIOCSOUTPUT ioctls, so returning 0 causes user
space (such as the daemon sending the report 5 set-feature request) to see
0 bytes transferred.

[ ... ]

> @@ -413,6 +669,8 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
>  	cancel_work_sync(&idev->recover_work);
>  	thc_ipts_dma_stop(idev);
>  
> +	thc_ipts_hid_remove(idev);
> +
>  	if (idev->running)
>  		thc_ipts_touch_set(idev, false);

[Severity: High]
Can stopping DMA before removing the HID device in thc_ipts_port_stop()
cause a NULL pointer dereference or use-after-free in thc_dma_write()?

Until thc_ipts_hid_remove() destroys idev->hid, user space (via hidraw) or
a HID driver can still invoke thc_ipts_hid_raw_request() -> thc_ipts_send().
Because thc_ipts_dma_stop() does not take idev->req_lock or
idev->thc->thc_bus_lock, it can free the DMA scatterlists and PRD tables
while thc_ipts_send() is entering thc_dma_write():

CPU0                                     CPU1
thc_ipts_hid_raw_request()
  guard(mutex)(&idev->req_lock)
  thc_ipts_send()
    if (!idev->dma_ready) (true)
                                         thc_ipts_port_stop()
                                           thc_interrupt_enable(..., false)
                                           ...
                                           thc_ipts_dma_stop()
                                             idev->dma_ready = false
                                             thc_dma_unconfigure(idev->thc)
                                             thc_dma_release(idev->thc)
                                               sgl_free(config->sgls[0])
                                               config->sgls[0] = NULL
                                               config->prd_tbls = NULL
    thc_dma_write()
      write_dma_buffer()
        sg_copy_from_buffer(write_config->sgls[0], ...)
          <-- NULL pointer dereference / use-after-free

In addition, masking interrupts before thc_ipts_hid_remove() drains active
callers causes any in-flight thc_dma_write(), thc_ipts_get_feature(), or
thc_ipts_hid_parse() to stall until its timeout expires. Calling
thc_ipts_hid_remove() before disabling interrupts and stopping DMA avoids
both issues.

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