[PATCH v4 00/11] Add spi-hid transport driver

[PATCH v4 00/11] Add spi-hid transport driver

[Jingyuan Liang]

This series picks up the spi-hid driver work originally started by
Microsoft. The patch breakdown has been modified and the implementation
has been refactored to address upstream feedback and testing issues. We
are submitting this as a new series while keeping the original sign-off
chain to reflect the history.

Same as the original series, there is a change to HID documentation, some
HID core changes to support a SPI device, the SPI HID transport driver,
and HID over SPI Device Tree binding. We have added the HID over SPI ACPI
support, power management, panel follower, and quirks for Ilitek touch
controllers.

Original authors: Jarrett Schultz <jaschultz@microsoft.com>,
		  Dmitry Antipov <dmanti@microsoft.com>
Link: https://lore.kernel.org/r/86b63b7b-afda-d7f4-7bfa-175085d5a8ef@gmail.com

Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
Changes in v4:
- Extended io_lock scope to protect the shid->hid pointer lifecycle
  against races with the IRQ handler
- Cacheline-aligned DMA buffers, enforced 4-byte alignment
- Added error rollback for failed suspend/resume transitions
- Moved IRQ request to probe with IRQF_NO_AUTOEN (disabled by default)
- DT Bindings & OF Driver:
  - Required a device-specific compatible string in the schema.
  - Added description to explain why opcodes and addresses properties
    need to be defined in the schema
  - Added `spi-peripheral-props.yaml` reference and switched to
    `unevaluatedProperties: false`.
  - Added fallback to default timing parameters in OF driver if match
    data is missing.
- Link to v3: https://lore.kernel.org/r/20260402-send-upstream-v3-0-6091c458d357@chromium.org

Changes in v3:
- Add io_lock init
- Relocate tracepoints to drivers/hid/spi-hid/ and fix tracepoint macros
- Add tracepoints for sync, error handling, reset, and report processing
- Clean up internal includes and fix Makefile CFLAGS
- Add more details in v2 changelog
- Link to v2: https://lore.kernel.org/r/20260324-send-upstream-v2-0-521ce8afff86@chromium.org

Changes in v2:
- Clean up DT bindings: fix formatting and remove timing and flags properties
- Update DT binding example: use a device-specific compatible and drop
  reset_assert
- Simplify ACPI/OF match tables by removing ACPI_PTR/of_match_ptr
- Refactor OF driver to use match data for timing parameters instead
  of DT properties
- Switch to fsleep() for delays in ACPI and OF drivers
- Drop patch 12 as it is vendor specific
- Add a lock to fix input/output concurrency race
- Link to v1: https://lore.kernel.org/r/20260303-send-upstream-v1-0-1515ba218f3d@chromium.org

---
Angela Czubak (2):
      HID: spi-hid: add transport driver skeleton for HID over SPI bus
      HID: spi_hid: add ACPI support for SPI over HID

Jarrett Schultz (3):
      Documentation: Correction in HID output_report callback description.
      HID: Add BUS_SPI support and define HID_SPI_DEVICE macro
      HID: spi_hid: add device tree support for SPI over HID

Jingyuan Liang (6):
      HID: spi-hid: add spi-hid driver HID layer
      HID: spi-hid: add HID SPI protocol implementation
      HID: spi_hid: add spi_hid traces
      dt-bindings: input: Document hid-over-spi DT schema
      HID: spi-hid: add power management implementation
      HID: spi-hid: add panel follower support

 .../devicetree/bindings/input/hid-over-spi.yaml    |  128 ++
 Documentation/hid/hid-transport.rst                |    4 +-
 drivers/hid/Kconfig                                |    2 +
 drivers/hid/Makefile                               |    2 +
 drivers/hid/hid-core.c                             |    3 +
 drivers/hid/spi-hid/Kconfig                        |   45 +
 drivers/hid/spi-hid/Makefile                       |   12 +
 drivers/hid/spi-hid/spi-hid-acpi.c                 |  254 ++++
 drivers/hid/spi-hid/spi-hid-core.c                 | 1521 ++++++++++++++++++++
 drivers/hid/spi-hid/spi-hid-core.h                 |   98 ++
 drivers/hid/spi-hid/spi-hid-of.c                   |  247 ++++
 drivers/hid/spi-hid/spi-hid-trace.h                |  169 +++
 drivers/hid/spi-hid/spi-hid.h                      |   46 +
 include/linux/hid.h                                |    2 +
 14 files changed, 2531 insertions(+), 2 deletions(-)
---
base-commit: 05f7e89ab9731565d8a62e3b5d1ec206485eeb0b
change-id: 20260212-send-upstream-75f6fd9ed92e

Best regards,
-- 
Jingyuan Liang <jingyliang@chromium.org>

[PATCH v4 01/11] Documentation: Correction in HID output_report callback description.

[Jingyuan Liang]

From: Jarrett Schultz <jaschultz@microsoft.com>

Originally output_report callback was described as must-be asynchronous,
but that is not the case in some implementations, namely i2c-hid.
Correct the documentation to say that it may be asynchronous.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 Documentation/hid/hid-transport.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/hid/hid-transport.rst b/Documentation/hid/hid-transport.rst
index 6f1692da296c..2008cf432af1 100644
--- a/Documentation/hid/hid-transport.rst
+++ b/Documentation/hid/hid-transport.rst
@@ -327,8 +327,8 @@ The available HID callbacks are:
 
    Send raw output report via intr channel. Used by some HID device drivers
    which require high throughput for outgoing requests on the intr channel. This
-   must not cause SET_REPORT calls! This must be implemented as asynchronous
-   output report on the intr channel!
+   must not cause SET_REPORT calls! This call might be asynchronous, so the
+   caller should not expect an immediate response!
 
    ::
 

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro

[Jingyuan Liang]

From: Jarrett Schultz <jaschultz@microsoft.com>

If connecting a hid_device with bus field indicating BUS_SPI print out
"SPI" in the debug print.

Macro sets the bus field to BUS_SPI and uses arguments to set vendor
product fields.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/hid-core.c | 3 +++
 include/linux/hid.h    | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a5b3a8ca2fcb..813c9c743ccd 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2316,6 +2316,9 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 	case BUS_I2C:
 		bus = "I2C";
 		break;
+	case BUS_SPI:
+		bus = "SPI";
+		break;
 	case BUS_SDW:
 		bus = "SOUNDWIRE";
 		break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index dce862cafbbd..957f322a0ebd 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -786,6 +786,8 @@ struct hid_descriptor {
 	.bus = BUS_BLUETOOTH, .vendor = (ven), .product = (prod)
 #define HID_I2C_DEVICE(ven, prod)				\
 	.bus = BUS_I2C, .vendor = (ven), .product = (prod)
+#define HID_SPI_DEVICE(ven, prod)				\
+	.bus = BUS_SPI, .vendor = (ven), .product = (prod)
 
 #define HID_REPORT_ID(rep) \
 	.report_type = (rep)

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus

[Jingyuan Liang]

From: Angela Czubak <acz@semihalf.com>

Create spi-hid folder and add Kconfig and Makefile for spi-hid driver.
Add basic device structure, definitions, and probe/remove functions.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/Kconfig                |   2 +
 drivers/hid/Makefile               |   2 +
 drivers/hid/spi-hid/Kconfig        |  15 +++
 drivers/hid/spi-hid/Makefile       |   9 ++
 drivers/hid/spi-hid/spi-hid-core.c | 213 +++++++++++++++++++++++++++++++++++++
 5 files changed, 241 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 920a64b66b25..c6ae23bfb75d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1434,6 +1434,8 @@ source "drivers/hid/bpf/Kconfig"
 
 source "drivers/hid/i2c-hid/Kconfig"
 
+source "drivers/hid/spi-hid/Kconfig"
+
 source "drivers/hid/intel-ish-hid/Kconfig"
 
 source "drivers/hid/amd-sfh-hid/Kconfig"
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 361a7daedeb8..6b43e789b39a 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -169,6 +169,8 @@ obj-$(CONFIG_USB_KBD)		+= usbhid/
 
 obj-$(CONFIG_I2C_HID_CORE)	+= i2c-hid/
 
+obj-$(CONFIG_SPI_HID_CORE)	+= spi-hid/
+
 obj-$(CONFIG_INTEL_ISH_HID)	+= intel-ish-hid/
 
 obj-$(CONFIG_AMD_SFH_HID)       += amd-sfh-hid/
diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
new file mode 100644
index 000000000000..836fdefe8345
--- /dev/null
+++ b/drivers/hid/spi-hid/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Copyright (c) 2021 Microsoft Corporation
+#
+
+menuconfig SPI_HID
+	tristate "SPI HID support"
+	default y
+	depends on SPI
+
+if SPI_HID
+
+config SPI_HID_CORE
+	tristate
+endif
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
new file mode 100644
index 000000000000..92e24cddbfc2
--- /dev/null
+++ b/drivers/hid/spi-hid/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the SPI HID input drivers
+#
+# Copyright (c) 2021 Microsoft Corporation
+#
+
+obj-$(CONFIG_SPI_HID_CORE)	+= spi-hid.o
+spi-hid-objs 			= spi-hid-core.o
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
new file mode 100644
index 000000000000..02a7608c4b88
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol implementation
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ *
+ * This code is partly based on "HID over I2C protocol implementation:
+ *
+ *  Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ *  Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ *  Copyright (c) 2012 Red Hat, Inc
+ *
+ *  which in turn is partly based on "USB HID support for Linux":
+ *
+ *  Copyright (c) 1999 Andreas Gal
+ *  Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ *  Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ *  Copyright (c) 2007-2008 Oliver Neukum
+ *  Copyright (c) 2006-2010 Jiri Kosina
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/hid-over-spi.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+/* struct spi_hid_conf - Conf provided to the core */
+struct spi_hid_conf {
+	u32 input_report_header_address;
+	u32 input_report_body_address;
+	u32 output_report_address;
+	u8 read_opcode;
+	u8 write_opcode;
+};
+
+/**
+ * struct spihid_ops - Ops provided to the core
+ * @power_up: do sequencing to power up the device
+ * @power_down: do sequencing to power down the device
+ * @assert_reset: do sequencing to assert the reset line
+ * @deassert_reset: do sequencing to deassert the reset line
+ * @sleep_minimal_reset_delay: minimal sleep delay during reset
+ */
+struct spihid_ops {
+	int (*power_up)(struct spihid_ops *ops);
+	int (*power_down)(struct spihid_ops *ops);
+	int (*assert_reset)(struct spihid_ops *ops);
+	int (*deassert_reset)(struct spihid_ops *ops);
+	void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
+};
+
+/* Driver context */
+struct spi_hid {
+	struct spi_device	*spi;	/* spi device. */
+	struct hid_device	*hid;	/* pointer to corresponding HID dev. */
+
+	struct spihid_ops	*ops;
+	struct spi_hid_conf	*conf;
+
+	enum hidspi_power_state power_state;
+
+	u32 regulator_error_count;
+	int regulator_last_error;
+	u32 bus_error_count;
+	int bus_last_error;
+	u32 dir_count;	/* device initiated reset count. */
+};
+
+static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state)
+{
+	switch (power_state) {
+	case HIDSPI_ON:
+		return "d0";
+	case HIDSPI_SLEEP:
+		return "d2";
+	case HIDSPI_OFF:
+		return "d3";
+	default:
+		return "unknown";
+	}
+}
+
+static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
+{
+	return IRQ_HANDLED;
+}
+
+static ssize_t bus_error_count_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct spi_hid *shid = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%u (%d)\n",
+			  shid->bus_error_count, shid->bus_last_error);
+}
+static DEVICE_ATTR_RO(bus_error_count);
+
+static ssize_t regulator_error_count_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct spi_hid *shid = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%u (%d)\n",
+			  shid->regulator_error_count,
+			  shid->regulator_last_error);
+}
+static DEVICE_ATTR_RO(regulator_error_count);
+
+static ssize_t device_initiated_reset_count_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct spi_hid *shid = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%u\n", shid->dir_count);
+}
+static DEVICE_ATTR_RO(device_initiated_reset_count);
+
+static struct attribute *spi_hid_attrs[] = {
+	&dev_attr_bus_error_count.attr,
+	&dev_attr_regulator_error_count.attr,
+	&dev_attr_device_initiated_reset_count.attr,
+	NULL	/* Terminator */
+};
+
+static const struct attribute_group spi_hid_group = {
+	.attrs = spi_hid_attrs,
+};
+
+const struct attribute_group *spi_hid_groups[] = {
+	&spi_hid_group,
+	NULL
+};
+EXPORT_SYMBOL_GPL(spi_hid_groups);
+
+int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
+		       struct spi_hid_conf *conf)
+{
+	struct device *dev = &spi->dev;
+	struct spi_hid *shid;
+	int error;
+
+	if (spi->irq <= 0)
+		return dev_err_probe(dev, spi->irq ?: -EINVAL, "Missing IRQ\n");
+
+	shid = devm_kzalloc(dev, sizeof(*shid), GFP_KERNEL);
+	if (!shid)
+		return -ENOMEM;
+
+	shid->spi = spi;
+	shid->power_state = HIDSPI_ON;
+	shid->ops = ops;
+	shid->conf = conf;
+
+	spi_set_drvdata(spi, shid);
+
+	/*
+	 * At the end of probe we initialize the device:
+	 *   0) assert reset, bias the interrupt line
+	 *   1) sleep minimal reset delay
+	 *   2) request IRQ
+	 *   3) power up the device
+	 *   4) deassert reset (high)
+	 * After this we expect an IRQ with a reset response.
+	 */
+
+	shid->ops->assert_reset(shid->ops);
+
+	shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+	error = devm_request_threaded_irq(dev, spi->irq, NULL, spi_hid_dev_irq,
+					  IRQF_ONESHOT, dev_name(&spi->dev), shid);
+	if (error) {
+		dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
+		return error;
+	}
+
+	error = shid->ops->power_up(shid->ops);
+	if (error) {
+		dev_err(dev, "%s: could not power up\n", __func__);
+		return error;
+	}
+
+	shid->ops->deassert_reset(shid->ops);
+
+	dev_dbg(dev, "%s: d3 -> %s\n", __func__,
+		spi_hid_power_mode_string(shid->power_state));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_hid_core_probe);
+
+void spi_hid_core_remove(struct spi_device *spi)
+{
+	struct spi_hid *shid = spi_get_drvdata(spi);
+	struct device *dev = &spi->dev;
+	int error;
+
+	shid->ops->assert_reset(shid->ops);
+	error = shid->ops->power_down(shid->ops);
+	if (error)
+		dev_err(dev, "failed to disable regulator\n");
+}
+EXPORT_SYMBOL_GPL(spi_hid_core_remove);
+
+MODULE_DESCRIPTION("HID over SPI transport driver");
+MODULE_AUTHOR("Dmitry Antipov <dmanti@microsoft.com>");
+MODULE_LICENSE("GPL");

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 04/11] HID: spi-hid: add spi-hid driver HID layer

[Jingyuan Liang]

Add HID low level driver callbacks to register SPI as a HID driver, and
an external touch device as a HID device.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/spi-hid-core.c | 563 +++++++++++++++++++++++++++++++++++++
 1 file changed, 563 insertions(+)

diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 02a7608c4b88..72c2e1ce3e8d 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -20,13 +20,69 @@
  *  Copyright (c) 2006-2010 Jiri Kosina
  */
 
+#include <linux/completion.h>
+#include <linux/crc32.h>
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/hid.h>
 #include <linux/hid-over-spi.h>
 #include <linux/interrupt.h>
+#include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/unaligned.h>
+
+#define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST	0x00
+
+#define SPI_HID_RESP_TIMEOUT	1000
+
+/* Protocol message size constants */
+#define SPI_HID_OUTPUT_HEADER_LEN		8
+
+/* flags */
+/*
+ * ready flag indicates that the FW is ready to accept commands and
+ * requests. The FW becomes ready after sending the report descriptor.
+ */
+#define SPI_HID_READY	0
+
+/* Raw input buffer with data from the bus */
+struct spi_hid_input_buf {
+	u8 header[HIDSPI_INPUT_HEADER_SIZE];
+	u8 body[HIDSPI_INPUT_BODY_HEADER_SIZE];
+	u8 content[];
+};
+
+/* Raw output report buffer to be put on the bus */
+struct spi_hid_output_buf {
+	u8 header[SPI_HID_OUTPUT_HEADER_LEN];
+	u8 content[];
+};
+
+/* Data necessary to send an output report */
+struct spi_hid_output_report {
+	u8 report_type;
+	u16 content_length;
+	u8 content_id;
+	u8 *content;
+};
+
+/* Processed data from a device descriptor */
+struct spi_hid_device_descriptor {
+	u16 hid_version;
+	u16 report_descriptor_length;
+	u16 max_input_length;
+	u16 max_output_length;
+	u16 max_fragment_length;
+	u16 vendor_id;
+	u16 product_id;
+	u16 version_id;
+	u8 no_output_report_ack;
+};
 
 /* struct spi_hid_conf - Conf provided to the core */
 struct spi_hid_conf {
@@ -61,8 +117,26 @@ struct spi_hid {
 	struct spihid_ops	*ops;
 	struct spi_hid_conf	*conf;
 
+	struct spi_hid_device_descriptor desc;	/* HID device descriptor. */
+	struct spi_hid_output_buf *output;	/* Output buffer. */
+	struct spi_hid_input_buf *input;	/* Input buffer. */
+	struct spi_hid_input_buf *response;	/* Response buffer. */
+
+	u16 response_length;
+	u16 bufsize;
+
 	enum hidspi_power_state power_state;
 
+	u8 reset_attempts;	/* The number of reset attempts. */
+
+	unsigned long flags;	/* device flags. */
+
+	/* Control lock to make sure one output transaction at a time. */
+	struct mutex output_lock;
+	struct completion output_done;
+
+	u32 report_descriptor_crc32;	/* HID report descriptor crc32 checksum. */
+
 	u32 regulator_error_count;
 	int regulator_last_error;
 	u32 bus_error_count;
@@ -70,6 +144,33 @@ struct spi_hid {
 	u32 dir_count;	/* device initiated reset count. */
 };
 
+static struct hid_ll_driver spi_hid_ll_driver;
+
+static void spi_hid_populate_output_header(u8 *buf,
+					   const struct spi_hid_conf *conf,
+					   const struct spi_hid_output_report *report)
+{
+	buf[0] = conf->write_opcode;
+	put_unaligned_be24(conf->output_report_address, &buf[1]);
+	buf[4] = report->report_type;
+	put_unaligned_le16(report->content_length, &buf[5]);
+	buf[7] = report->content_id;
+}
+
+static int spi_hid_output(struct spi_hid *shid, const void *buf, u16 length)
+{
+	int error;
+
+	error = spi_write(shid->spi, buf, length);
+
+	if (error) {
+		shid->bus_error_count++;
+		shid->bus_last_error = error;
+	}
+
+	return error;
+}
+
 static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state)
 {
 	switch (power_state) {
@@ -84,11 +185,455 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
 	}
 }
 
+static void spi_hid_stop_hid(struct spi_hid *shid)
+{
+	struct hid_device *hid = shid->hid;
+
+	shid->hid = NULL;
+	clear_bit(SPI_HID_READY, &shid->flags);
+
+	if (hid)
+		hid_destroy_device(hid);
+}
+
+static int __spi_hid_send_output_report(struct spi_hid *shid,
+					struct spi_hid_output_report *report)
+{
+	struct spi_hid_output_buf *buf = shid->output;
+	struct device *dev = &shid->spi->dev;
+	u16 report_length;
+	u16 padded_length;
+	u8 padding;
+	int error;
+
+	if (report->content_length > shid->desc.max_output_length ||
+	    report->content_length > shid->bufsize) {
+		dev_err(dev, "Output report too big, content_length 0x%x\n",
+			report->content_length);
+		return -E2BIG;
+	}
+
+	spi_hid_populate_output_header(buf->header, shid->conf, report);
+
+	if (report->content_length)
+		memcpy(&buf->content, report->content, report->content_length);
+
+	report_length = sizeof(buf->header) + report->content_length;
+	padded_length = round_up(report_length, 4);
+	padding = padded_length - report_length;
+	memset(&buf->content[report->content_length], 0, padding);
+
+	error = spi_hid_output(shid, buf, padded_length);
+	if (error)
+		dev_err(dev, "Failed output transfer: %d\n", error);
+
+	return error;
+}
+
+static int spi_hid_send_output_report(struct spi_hid *shid,
+				      struct spi_hid_output_report *report)
+{
+	guard(mutex)(&shid->output_lock);
+	return __spi_hid_send_output_report(shid, report);
+}
+
+static int spi_hid_sync_request(struct spi_hid *shid,
+				struct spi_hid_output_report *report)
+{
+	struct device *dev = &shid->spi->dev;
+	int error;
+
+	guard(mutex)(&shid->output_lock);
+
+	reinit_completion(&shid->output_done);
+
+	error = __spi_hid_send_output_report(shid, report);
+	if (error)
+		return error;
+
+	error = wait_for_completion_interruptible_timeout(&shid->output_done,
+							  msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
+	if (error == 0) {
+		dev_err(dev, "Response timed out\n");
+		return -ETIMEDOUT;
+	}
+	if (error < 0)
+		return error;
+
+	return 0;
+}
+
+/*
+ * This function returns the length of the report descriptor, or a negative
+ * error code if something went wrong.
+ */
+static int spi_hid_report_descriptor_request(struct spi_hid *shid)
+{
+	struct device *dev = &shid->spi->dev;
+	struct spi_hid_output_report report = {
+		.report_type = REPORT_DESCRIPTOR,
+		.content_length = 0,
+		.content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
+		.content = NULL,
+	};
+	int ret;
+
+	ret =  spi_hid_sync_request(shid, &report);
+	if (ret) {
+		dev_err(dev,
+			"Expected report descriptor not received: %d\n", ret);
+		return ret;
+	}
+
+	ret = shid->response_length;
+	if (ret != shid->desc.report_descriptor_length) {
+		ret = min_t(unsigned int, ret, shid->desc.report_descriptor_length);
+		dev_err(dev, "Received report descriptor length doesn't match device descriptor field, using min of the two: %d\n",
+			ret);
+	}
+
+	return ret;
+}
+
+static int spi_hid_create_device(struct spi_hid *shid)
+{
+	struct hid_device *hid;
+	struct device *dev = &shid->spi->dev;
+	int error;
+
+	hid = hid_allocate_device();
+	error = PTR_ERR_OR_ZERO(hid);
+	if (error) {
+		dev_err(dev, "Failed to allocate hid device: %d\n", error);
+		return error;
+	}
+
+	hid->driver_data = shid->spi;
+	hid->ll_driver = &spi_hid_ll_driver;
+	hid->dev.parent = &shid->spi->dev;
+	hid->bus = BUS_SPI;
+	hid->version = shid->desc.hid_version;
+	hid->vendor = shid->desc.vendor_id;
+	hid->product = shid->desc.product_id;
+
+	snprintf(hid->name, sizeof(hid->name), "spi %04X:%04X",
+		 hid->vendor, hid->product);
+	strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
+
+	shid->hid = hid;
+
+	error = hid_add_device(hid);
+	if (error) {
+		dev_err(dev, "Failed to add hid device: %d\n", error);
+		/*
+		 * We likely got here because report descriptor request timed
+		 * out. Let's disconnect and destroy the hid_device structure.
+		 */
+		spi_hid_stop_hid(shid);
+		return error;
+	}
+
+	return 0;
+}
+
+static int spi_hid_get_request(struct spi_hid *shid, u8 content_id)
+{
+	struct device *dev = &shid->spi->dev;
+	struct spi_hid_output_report report = {
+		.report_type = GET_FEATURE,
+		.content_length = 0,
+		.content_id = content_id,
+		.content = NULL,
+	};
+	int error;
+
+	error = spi_hid_sync_request(shid, &report);
+	if (error) {
+		dev_err(dev,
+			"Expected get request response not received! Error %d\n",
+			error);
+		return error;
+	}
+
+	return 0;
+}
+
+static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
+			       u8 content_id)
+{
+	struct spi_hid_output_report report = {
+		.report_type = SET_FEATURE,
+		.content_length = arg_len,
+		.content_id = content_id,
+		.content = arg_buf,
+	};
+
+	return spi_hid_sync_request(shid, &report);
+}
+
+/* This is a placeholder. Will be implemented in the next patch. */
 static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 {
 	return IRQ_HANDLED;
 }
 
+static int spi_hid_alloc_buffers(struct spi_hid *shid, size_t report_size)
+{
+	struct device *dev = &shid->spi->dev;
+	int inbufsize = round_up(sizeof(shid->input->header) +
+				 sizeof(shid->input->body) + report_size, 4);
+	int outbufsize = round_up(sizeof(shid->output->header) + report_size, 4);
+	void *tmp;
+
+	tmp = devm_krealloc(dev, shid->output, outbufsize, GFP_KERNEL | __GFP_ZERO);
+	if (!tmp)
+		return -ENOMEM;
+	shid->output = tmp;
+
+	tmp = devm_krealloc(dev, shid->input, inbufsize, GFP_KERNEL | __GFP_ZERO);
+	if (!tmp)
+		return -ENOMEM;
+	shid->input = tmp;
+
+	tmp = devm_krealloc(dev, shid->response, inbufsize, GFP_KERNEL | __GFP_ZERO);
+	if (!tmp)
+		return -ENOMEM;
+	shid->response = tmp;
+
+	if (!shid->output || !shid->input || !shid->response)
+		return -ENOMEM;
+
+	shid->bufsize = report_size;
+
+	return 0;
+}
+
+static int spi_hid_get_report_length(struct hid_report *report)
+{
+	return DIV_ROUND_UP(report->size, 8) +
+		report->device->report_enum[report->type].numbered + 2;
+}
+
+/*
+ * Traverse the supplied list of reports and find the longest
+ */
+static void spi_hid_find_max_report(struct hid_device *hid, u32 type,
+				    u16 *max)
+{
+	struct hid_report *report;
+	u16 size;
+
+	/*
+	 * We should not rely on wMaxInputLength, as some devices may set it to
+	 * a wrong length.
+	 */
+	list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
+		size = spi_hid_get_report_length(report);
+		if (*max < size)
+			*max = size;
+	}
+}
+
+/* hid_ll_driver interface functions */
+
+static int spi_hid_ll_start(struct hid_device *hid)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+	int error = 0;
+	u16 bufsize = 0;
+
+	spi_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
+	spi_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
+	spi_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
+
+	if (bufsize < HID_MIN_BUFFER_SIZE) {
+		dev_err(&spi->dev,
+			"HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
+			bufsize);
+		return -EINVAL;
+	}
+
+	if (bufsize > shid->bufsize) {
+		guard(disable_irq)(&shid->spi->irq);
+
+		error = spi_hid_alloc_buffers(shid, bufsize);
+		if (error)
+			return error;
+	}
+
+	return 0;
+}
+
+static void spi_hid_ll_stop(struct hid_device *hid)
+{
+	hid->claimed = 0;
+}
+
+static int spi_hid_ll_open(struct hid_device *hid)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+
+	set_bit(SPI_HID_READY, &shid->flags);
+	return 0;
+}
+
+static void spi_hid_ll_close(struct hid_device *hid)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+
+	clear_bit(SPI_HID_READY, &shid->flags);
+	shid->reset_attempts = 0;
+}
+
+static int spi_hid_ll_power(struct hid_device *hid, int level)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+	int error = 0;
+
+	guard(mutex)(&shid->output_lock);
+	if (!shid->hid)
+		error = -ENODEV;
+
+	return error;
+}
+
+static int spi_hid_ll_parse(struct hid_device *hid)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+	struct device *dev = &spi->dev;
+	unsigned int rsize = shid->desc.report_descriptor_length;
+	int error, len;
+
+	if (rsize > HID_MAX_DESCRIPTOR_SIZE) {
+		dev_err(dev,
+			"Report descriptor size %d is greater than HID_MAX_DESCRIPTOR_SIZE %d\n",
+			rsize, HID_MAX_DESCRIPTOR_SIZE);
+		return -EINVAL;
+	}
+
+	if (rsize > shid->bufsize) {
+		error = spi_hid_alloc_buffers(shid, rsize);
+		if (error)
+			return error;
+	}
+
+	len = spi_hid_report_descriptor_request(shid);
+	if (len < 0) {
+		dev_err(dev, "Report descriptor request failed, %d\n", len);
+		return len;
+	}
+
+	/*
+	 * FIXME: below call returning 0 doesn't mean that the report descriptor
+	 * is good. We might be caching a crc32 of a corrupted r. d. or who
+	 * knows what the FW sent. Need to have a feedback loop about r. d.
+	 * being ok and only then cache it.
+	 */
+	error = hid_parse_report(hid, (u8 *)shid->response->content, len);
+	if (error) {
+		dev_err(dev, "failed parsing report: %d\n", error);
+		return error;
+	}
+	shid->report_descriptor_crc32 = crc32_le(0,
+						 (unsigned char const *)shid->response->content,
+						 len);
+
+	return 0;
+}
+
+static int spi_hid_ll_raw_request(struct hid_device *hid,
+				  unsigned char reportnum, __u8 *buf,
+				  size_t len, unsigned char rtype, int reqtype)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+	struct device *dev = &spi->dev;
+	int ret;
+
+	switch (reqtype) {
+	case HID_REQ_SET_REPORT:
+		if (buf[0] != reportnum) {
+			dev_err(dev, "report id mismatch\n");
+			return -EINVAL;
+		}
+
+		ret = spi_hid_set_request(shid, &buf[1], len - 1,
+					  reportnum);
+		if (ret) {
+			dev_err(dev, "failed to set report\n");
+			return ret;
+		}
+
+		ret = len;
+		break;
+	case HID_REQ_GET_REPORT:
+		ret = spi_hid_get_request(shid, reportnum);
+		if (ret) {
+			dev_err(dev, "failed to get report\n");
+			return ret;
+		}
+
+		ret = min_t(size_t, len,
+			    (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
+		buf[0] = shid->response->body[3];
+		memcpy(&buf[1], &shid->response->content, ret);
+		break;
+	default:
+		dev_err(dev, "invalid request type\n");
+		return -EIO;
+	}
+
+	return ret;
+}
+
+static int spi_hid_ll_output_report(struct hid_device *hid, __u8 *buf,
+				    size_t len)
+{
+	struct spi_device *spi = hid->driver_data;
+	struct spi_hid *shid = spi_get_drvdata(spi);
+	struct device *dev = &spi->dev;
+	struct spi_hid_output_report report = {
+		.report_type = OUTPUT_REPORT,
+		.content_length = len - 1,
+		.content_id = buf[0],
+		.content = &buf[1],
+	};
+	int error;
+
+	if (!test_bit(SPI_HID_READY, &shid->flags)) {
+		dev_err(dev, "%s called in unready state\n", __func__);
+		return -ENODEV;
+	}
+
+	if (shid->desc.no_output_report_ack)
+		error = spi_hid_send_output_report(shid, &report);
+	else
+		error = spi_hid_sync_request(shid, &report);
+
+	if (error) {
+		dev_err(dev, "failed to send output report\n");
+		return error;
+	}
+
+	return len;
+}
+
+static struct hid_ll_driver spi_hid_ll_driver = {
+	.start = spi_hid_ll_start,
+	.stop = spi_hid_ll_stop,
+	.open = spi_hid_ll_open,
+	.close = spi_hid_ll_close,
+	.power = spi_hid_ll_power,
+	.parse = spi_hid_ll_parse,
+	.output_report = spi_hid_ll_output_report,
+	.raw_request = spi_hid_ll_raw_request,
+};
+
 static ssize_t bus_error_count_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
@@ -159,6 +704,18 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 
 	spi_set_drvdata(spi, shid);
 
+	mutex_init(&shid->output_lock);
+	init_completion(&shid->output_done);
+
+	/*
+	 * we need to allocate the buffer without knowing the maximum
+	 * size of the reports. Let's use SZ_2K, then we do the
+	 * real computation later.
+	 */
+	error = spi_hid_alloc_buffers(shid, SZ_2K);
+	if (error)
+		return error;
+
 	/*
 	 * At the end of probe we initialize the device:
 	 *   0) assert reset, bias the interrupt line
@@ -191,6 +748,10 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 	dev_dbg(dev, "%s: d3 -> %s\n", __func__,
 		spi_hid_power_mode_string(shid->power_state));
 
+	error = spi_hid_create_device(shid);
+	if (error)
+		return error;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spi_hid_core_probe);
@@ -201,6 +762,8 @@ void spi_hid_core_remove(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	int error;
 
+	spi_hid_stop_hid(shid);
+
 	shid->ops->assert_reset(shid->ops);
 	error = shid->ops->power_down(shid->ops);
 	if (error)

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 05/11] HID: spi-hid: add HID SPI protocol implementation

[Jingyuan Liang]

This driver follows HID Over SPI Protocol Specification 1.0 available at
https://www.microsoft.com/en-us/download/details.aspx?id=103325. The
initial version of the driver does not support: 1) multi-fragment input
reports, 2) sending GET_INPUT and COMMAND output report types and
processing their respective acknowledge input reports, and 3) device
sleep power state.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/spi-hid-core.c | 608 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 592 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 72c2e1ce3e8d..f6ea2d4365a7 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -20,14 +20,20 @@
  *  Copyright (c) 2006-2010 Jiri Kosina
  */
 
+#include <linux/cache.h>
 #include <linux/completion.h>
 #include <linux/crc32.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/hid.h>
 #include <linux/hid-over-spi.h>
+#include <linux/input.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -35,12 +41,22 @@
 #include <linux/string.h>
 #include <linux/sysfs.h>
 #include <linux/unaligned.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+/* Protocol constants */
+#define SPI_HID_READ_APPROVAL_CONSTANT		0xff
+#define SPI_HID_INPUT_HEADER_SYNC_BYTE		0x5a
+#define SPI_HID_INPUT_HEADER_VERSION		0x03
+#define SPI_HID_SUPPORTED_VERSION		0x0300
 
 #define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST	0x00
 
-#define SPI_HID_RESP_TIMEOUT	1000
+#define SPI_HID_MAX_RESET_ATTEMPTS	3
+#define SPI_HID_RESP_TIMEOUT		1000
 
 /* Protocol message size constants */
+#define SPI_HID_READ_APPROVAL_LEN		5
 #define SPI_HID_OUTPUT_HEADER_LEN		8
 
 /* flags */
@@ -49,6 +65,22 @@
  * requests. The FW becomes ready after sending the report descriptor.
  */
 #define SPI_HID_READY	0
+/*
+ * refresh_in_progress is set to true while the refresh_device worker
+ * thread is destroying and recreating the hidraw device. When this flag
+ * is set to true, the ll_close and ll_open functions will not cause
+ * power state changes.
+ */
+#define SPI_HID_REFRESH_IN_PROGRESS	1
+/*
+ * reset_pending indicates that the device is being reset. When this flag
+ * is set to true, garbage interrupts triggered during reset will be
+ * dropped and will not cause error handling.
+ */
+#define SPI_HID_RESET_PENDING	2
+#define SPI_HID_RESET_RESPONSE	3
+#define SPI_HID_CREATE_DEVICE	4
+#define SPI_HID_ERROR	5
 
 /* Raw input buffer with data from the bus */
 struct spi_hid_input_buf {
@@ -57,6 +89,22 @@ struct spi_hid_input_buf {
 	u8 content[];
 };
 
+/* Processed data from input report header */
+struct spi_hid_input_header {
+	u8 version;
+	u16 report_length;
+	u8 last_fragment_flag;
+	u8 sync_const;
+};
+
+/* Processed data from an input report */
+struct spi_hid_input_report {
+	u8 report_type;
+	u16 content_length;
+	u8 content_id;
+	u8 *content;
+};
+
 /* Raw output report buffer to be put on the bus */
 struct spi_hid_output_buf {
 	u8 header[SPI_HID_OUTPUT_HEADER_LEN];
@@ -114,6 +162,9 @@ struct spi_hid {
 	struct spi_device	*spi;	/* spi device. */
 	struct hid_device	*hid;	/* pointer to corresponding HID dev. */
 
+	struct spi_transfer	input_transfer[2];	/* Transfer buffer for read and write. */
+	struct spi_message	input_message;	/* used to execute a sequence of spi transfers. */
+
 	struct spihid_ops	*ops;
 	struct spi_hid_conf	*conf;
 
@@ -131,8 +182,15 @@ struct spi_hid {
 
 	unsigned long flags;	/* device flags. */
 
-	/* Control lock to make sure one output transaction at a time. */
+	struct work_struct reset_work;
+
+	/* Control lock to ensure complete output transaction. */
 	struct mutex output_lock;
+	/* Power lock to make sure one power state change at a time. */
+	struct mutex power_lock;
+	/* Protect bus I/O and shid->hid pointer lifecycle. */
+	struct mutex io_lock;
+
 	struct completion output_done;
 
 	u32 report_descriptor_crc32;	/* HID report descriptor crc32 checksum. */
@@ -142,10 +200,74 @@ struct spi_hid {
 	u32 bus_error_count;
 	int bus_last_error;
 	u32 dir_count;	/* device initiated reset count. */
+
+	/* DMA-safe transfer buffers */
+	u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN] ____cacheline_aligned;
+	u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
 };
 
 static struct hid_ll_driver spi_hid_ll_driver;
 
+static void spi_hid_populate_read_approvals(const struct spi_hid_conf *conf,
+					    u8 *header_buf, u8 *body_buf)
+{
+	header_buf[0] = conf->read_opcode;
+	put_unaligned_be24(conf->input_report_header_address, &header_buf[1]);
+	header_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
+
+	body_buf[0] = conf->read_opcode;
+	put_unaligned_be24(conf->input_report_body_address, &body_buf[1]);
+	body_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
+}
+
+static void spi_hid_parse_dev_desc(const struct hidspi_dev_descriptor *raw,
+				   struct spi_hid_device_descriptor *desc)
+{
+	desc->hid_version = le16_to_cpu(raw->bcd_ver);
+	desc->report_descriptor_length = le16_to_cpu(raw->rep_desc_len);
+	desc->max_input_length = le16_to_cpu(raw->max_input_len);
+	desc->max_output_length = le16_to_cpu(raw->max_output_len);
+
+	/* FIXME: multi-fragment not supported, field below not used */
+	desc->max_fragment_length = le16_to_cpu(raw->max_frag_len);
+
+	desc->vendor_id = le16_to_cpu(raw->vendor_id);
+	desc->product_id = le16_to_cpu(raw->product_id);
+	desc->version_id = le16_to_cpu(raw->version_id);
+	desc->no_output_report_ack = le16_to_cpu(raw->flags) & BIT(0);
+}
+
+static void spi_hid_populate_input_header(const u8 *buf,
+					  struct spi_hid_input_header *header)
+{
+	header->version            = buf[0] & 0xf;
+	header->report_length      = (get_unaligned_le16(&buf[1]) & 0x3fff) * 4;
+	header->last_fragment_flag = (buf[2] & 0x40) >> 6;
+	header->sync_const         = buf[3];
+}
+
+static void spi_hid_populate_input_body(const u8 *buf,
+					struct input_report_body_header *body)
+{
+	body->input_report_type = buf[0];
+	body->content_len = get_unaligned_le16(&buf[1]);
+	body->content_id = buf[3];
+}
+
+static void spi_hid_input_report_prepare(struct spi_hid_input_buf *buf,
+					 struct spi_hid_input_report *report)
+{
+	struct spi_hid_input_header header;
+	struct input_report_body_header body;
+
+	spi_hid_populate_input_header(buf->header, &header);
+	spi_hid_populate_input_body(buf->body, &body);
+	report->report_type = body.input_report_type;
+	report->content_length = body.content_len;
+	report->content_id = body.content_id;
+	report->content = buf->content;
+}
+
 static void spi_hid_populate_output_header(u8 *buf,
 					   const struct spi_hid_conf *conf,
 					   const struct spi_hid_output_report *report)
@@ -157,6 +279,33 @@ static void spi_hid_populate_output_header(u8 *buf,
 	buf[7] = report->content_id;
 }
 
+static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
+			      bool is_header)
+{
+	int error;
+
+	shid->input_transfer[0].tx_buf = is_header ?
+					 shid->read_approval_header :
+					 shid->read_approval_body;
+	shid->input_transfer[0].len = SPI_HID_READ_APPROVAL_LEN;
+
+	shid->input_transfer[1].rx_buf = buf;
+	shid->input_transfer[1].len = length;
+
+	spi_message_init_with_transfers(&shid->input_message,
+					shid->input_transfer, 2);
+
+	error = spi_sync(shid->spi, &shid->input_message);
+	if (error) {
+		dev_err(&shid->spi->dev, "Error starting sync transfer: %d\n", error);
+		shid->bus_error_count++;
+		shid->bus_last_error = error;
+		return error;
+	}
+
+	return 0;
+}
+
 static int spi_hid_output(struct spi_hid *shid, const void *buf, u16 length)
 {
 	int error;
@@ -187,15 +336,64 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
 
 static void spi_hid_stop_hid(struct spi_hid *shid)
 {
-	struct hid_device *hid = shid->hid;
+	struct hid_device *hid;
 
-	shid->hid = NULL;
-	clear_bit(SPI_HID_READY, &shid->flags);
+	scoped_guard(mutex, &shid->io_lock) {
+		hid = shid->hid;
+		shid->hid = NULL;
+		clear_bit(SPI_HID_READY, &shid->flags);
+	}
 
 	if (hid)
 		hid_destroy_device(hid);
 }
 
+static void spi_hid_error_handler(struct spi_hid *shid)
+{
+	struct device *dev = &shid->spi->dev;
+	int error;
+
+	guard(mutex)(&shid->power_lock);
+	if (shid->power_state == HIDSPI_OFF)
+		return;
+
+	guard(disable_irq)(&shid->spi->irq);
+
+	if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
+		dev_err(dev, "unresponsive device, aborting\n");
+		spi_hid_stop_hid(shid);
+		shid->ops->assert_reset(shid->ops);
+		error = shid->ops->power_down(shid->ops);
+		if (error) {
+			dev_err(dev, "failed to disable regulator\n");
+			shid->regulator_error_count++;
+			shid->regulator_last_error = error;
+		}
+		return;
+	}
+
+	clear_bit(SPI_HID_READY, &shid->flags);
+	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+
+	shid->ops->assert_reset(shid->ops);
+
+	shid->power_state = HIDSPI_OFF;
+
+	/*
+	 * We want to cancel pending reset work as the device is being reset
+	 * to recover from an error. cancel_work_sync will put us in a deadlock
+	 * because this function is scheduled in 'reset_work' and we should
+	 * avoid waiting for itself.
+	 */
+	cancel_work(&shid->reset_work);
+
+	shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+	shid->power_state = HIDSPI_ON;
+
+	shid->ops->deassert_reset(shid->ops);
+}
+
 static int __spi_hid_send_output_report(struct spi_hid *shid,
 					struct spi_hid_output_report *report)
 {
@@ -213,6 +411,7 @@ static int __spi_hid_send_output_report(struct spi_hid *shid,
 		return -E2BIG;
 	}
 
+	guard(mutex)(&shid->io_lock);
 	spi_hid_populate_output_header(buf->header, shid->conf, report);
 
 	if (report->content_length)
@@ -263,6 +462,88 @@ static int spi_hid_sync_request(struct spi_hid *shid,
 	return 0;
 }
 
+/*
+ * Handle the reset response from the FW by sending a request for the device
+ * descriptor.
+ */
+static void spi_hid_reset_response(struct spi_hid *shid)
+{
+	struct device *dev = &shid->spi->dev;
+	struct spi_hid_output_report report = {
+		.report_type = DEVICE_DESCRIPTOR,
+		.content_length = 0x0,
+		.content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
+		.content = NULL,
+	};
+	int error;
+
+	if (test_bit(SPI_HID_READY, &shid->flags)) {
+		dev_err(dev, "Spontaneous FW reset!\n");
+		clear_bit(SPI_HID_READY, &shid->flags);
+		shid->dir_count++;
+	}
+
+	if (shid->power_state == HIDSPI_OFF)
+		return;
+
+	error = spi_hid_sync_request(shid, &report);
+	if (error) {
+		dev_WARN_ONCE(dev, true,
+			      "Failed to send device descriptor request: %d\n", error);
+		set_bit(SPI_HID_ERROR, &shid->flags);
+		schedule_work(&shid->reset_work);
+	}
+}
+
+static int spi_hid_input_report_handler(struct spi_hid *shid,
+					struct spi_hid_input_buf *buf)
+{
+	struct device *dev = &shid->spi->dev;
+	struct spi_hid_input_report r;
+	int error = 0;
+
+	guard(mutex)(&shid->io_lock);
+
+	if (!test_bit(SPI_HID_READY, &shid->flags) ||
+	    test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
+		dev_err(dev, "HID not ready\n");
+		return 0;
+	}
+
+	spi_hid_input_report_prepare(buf, &r);
+
+	error = hid_input_report(shid->hid, HID_INPUT_REPORT,
+				 r.content - 1, r.content_length + 1, 1);
+
+	if (error == -ENODEV || error == -EBUSY) {
+		dev_err(dev, "ignoring report --> %d\n", error);
+		return 0;
+	} else if (error) {
+		dev_err(dev, "Bad input report: %d\n", error);
+	}
+
+	return error;
+}
+
+static void spi_hid_response_handler(struct spi_hid *shid,
+				     struct input_report_body_header *body)
+{
+	shid->response_length = body->content_len;
+	/* completion_done returns 0 if there are waiters, otherwise 1 */
+	if (completion_done(&shid->output_done)) {
+		dev_err(&shid->spi->dev, "Unexpected response report\n");
+	} else {
+		if (body->input_report_type == REPORT_DESCRIPTOR_RESPONSE ||
+		    body->input_report_type == GET_FEATURE_RESPONSE) {
+			memcpy(shid->response->body, shid->input->body,
+			       sizeof(shid->input->body));
+			memcpy(shid->response->content, shid->input->content,
+			       body->content_len);
+		}
+		complete(&shid->output_done);
+	}
+}
+
 /*
  * This function returns the length of the report descriptor, or a negative
  * error code if something went wrong.
@@ -282,6 +563,8 @@ static int spi_hid_report_descriptor_request(struct spi_hid *shid)
 	if (ret) {
 		dev_err(dev,
 			"Expected report descriptor not received: %d\n", ret);
+		set_bit(SPI_HID_ERROR, &shid->flags);
+		schedule_work(&shid->reset_work);
 		return ret;
 	}
 
@@ -320,7 +603,9 @@ static int spi_hid_create_device(struct spi_hid *shid)
 		 hid->vendor, hid->product);
 	strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
 
-	shid->hid = hid;
+	scoped_guard(mutex, &shid->io_lock) {
+		shid->hid = hid;
+	}
 
 	error = hid_add_device(hid);
 	if (error) {
@@ -336,6 +621,208 @@ static int spi_hid_create_device(struct spi_hid *shid)
 	return 0;
 }
 
+static void spi_hid_refresh_device(struct spi_hid *shid)
+{
+	struct device *dev = &shid->spi->dev;
+	u32 new_crc32 = 0;
+	int error = 0;
+
+	error = spi_hid_report_descriptor_request(shid);
+	if (error < 0) {
+		dev_err(dev,
+			"%s: failed report descriptor request: %d\n",
+			__func__, error);
+		return;
+	}
+	new_crc32 = crc32_le(0, (unsigned char const *)shid->response->content,
+			     (size_t)error);
+
+	/* Same report descriptor, so no need to create a new hid device. */
+	if (new_crc32 == shid->report_descriptor_crc32) {
+		set_bit(SPI_HID_READY, &shid->flags);
+		return;
+	}
+
+	shid->report_descriptor_crc32 = new_crc32;
+
+	set_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags);
+
+	spi_hid_stop_hid(shid);
+
+	error = spi_hid_create_device(shid);
+	if (error) {
+		dev_err(dev, "%s: Failed to create hid device: %d\n", __func__, error);
+		return;
+	}
+
+	clear_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags);
+}
+
+static void spi_hid_reset_work(struct work_struct *work)
+{
+	struct spi_hid *shid =
+		container_of(work, struct spi_hid, reset_work);
+	struct device *dev = &shid->spi->dev;
+	int error = 0;
+	bool resched = false;
+
+	if (test_and_clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags)) {
+		spi_hid_reset_response(shid);
+		resched = true;
+	} else if (test_and_clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags)) {
+		guard(mutex)(&shid->power_lock);
+		if (shid->power_state != HIDSPI_OFF) {
+			if (!shid->hid) {
+				error = spi_hid_create_device(shid);
+				if (error) {
+					dev_err(dev, "%s: Failed to create hid device: %d\n",
+						__func__, error);
+				}
+			} else {
+				spi_hid_refresh_device(shid);
+			}
+		} else {
+			dev_err(dev, "%s: Powered off, returning\n", __func__);
+		}
+		resched = true;
+	} else if (test_and_clear_bit(SPI_HID_ERROR, &shid->flags)) {
+		spi_hid_error_handler(shid);
+	}
+
+	/*
+	 * If other flags are still pending, safely reschedule ourselves
+	 * to process them in the next workqueue cycle.
+	 */
+	if (resched && (shid->flags & (BIT(SPI_HID_RESET_RESPONSE) |
+				       BIT(SPI_HID_CREATE_DEVICE) |
+				       BIT(SPI_HID_ERROR)))) {
+		schedule_work(&shid->reset_work);
+	}
+}
+
+static int spi_hid_process_input_report(struct spi_hid *shid,
+					struct spi_hid_input_buf *buf)
+{
+	struct spi_hid_input_header header;
+	struct input_report_body_header body;
+	struct device *dev = &shid->spi->dev;
+	struct hidspi_dev_descriptor *raw;
+
+	spi_hid_populate_input_header(buf->header, &header);
+	spi_hid_populate_input_body(buf->body, &body);
+
+	if (HIDSPI_INPUT_BODY_SIZE(body.content_len) > header.report_length) {
+		dev_err(dev, "Bad body length %zu > %u\n", HIDSPI_INPUT_BODY_SIZE(body.content_len),
+			header.report_length);
+		return -EPROTO;
+	}
+
+	switch (body.input_report_type) {
+	case DATA:
+		return spi_hid_input_report_handler(shid, buf);
+	case RESET_RESPONSE:
+		clear_bit(SPI_HID_RESET_PENDING, &shid->flags);
+		set_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
+		schedule_work(&shid->reset_work);
+		break;
+	case DEVICE_DESCRIPTOR_RESPONSE:
+		/* Mark the completion done to avoid timeout */
+		spi_hid_response_handler(shid, &body);
+
+		/* Reset attempts at every device descriptor fetch */
+		shid->reset_attempts = 0;
+		raw = (struct hidspi_dev_descriptor *)buf->content;
+
+		/* Validate device descriptor length before parsing */
+		if (body.content_len != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
+			dev_err(dev, "Invalid content length %d, expected %zu\n",
+				body.content_len,
+				HIDSPI_DEVICE_DESCRIPTOR_SIZE);
+			return -EPROTO;
+		}
+
+		if (le16_to_cpu(raw->dev_desc_len) !=
+		    HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
+			dev_err(dev,
+				"Invalid wDeviceDescLength %d, expected %zu\n",
+				le16_to_cpu(raw->dev_desc_len),
+				HIDSPI_DEVICE_DESCRIPTOR_SIZE);
+			return -EPROTO;
+		}
+
+		spi_hid_parse_dev_desc(raw, &shid->desc);
+
+		if (shid->desc.hid_version != SPI_HID_SUPPORTED_VERSION) {
+			dev_err(dev,
+				"Unsupported device descriptor version %4x\n",
+				shid->desc.hid_version);
+			return -EPROTONOSUPPORT;
+		}
+
+		set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
+		schedule_work(&shid->reset_work);
+
+		break;
+	case OUTPUT_REPORT_RESPONSE:
+		if (shid->desc.no_output_report_ack) {
+			dev_err(dev, "Unexpected output report response\n");
+			break;
+		}
+		fallthrough;
+	case GET_FEATURE_RESPONSE:
+	case SET_FEATURE_RESPONSE:
+	case REPORT_DESCRIPTOR_RESPONSE:
+		spi_hid_response_handler(shid, &body);
+		break;
+	/*
+	 * FIXME: sending GET_INPUT and COMMAND reports not supported, thus
+	 * throw away responses to those, they should never come.
+	 */
+	case GET_INPUT_REPORT_RESPONSE:
+	case COMMAND_RESPONSE:
+		dev_err(dev, "Not a supported report type: 0x%x\n",
+			body.input_report_type);
+		break;
+	default:
+		dev_err(dev, "Unknown input report: 0x%x\n", body.input_report_type);
+		return -EPROTO;
+	}
+
+	return 0;
+}
+
+static int spi_hid_bus_validate_header(struct spi_hid *shid,
+				       struct spi_hid_input_header *header)
+{
+	struct device *dev = &shid->spi->dev;
+
+	if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
+		dev_err(dev, "Unknown input report version (v 0x%x)\n",
+			header->version);
+		return -EINVAL;
+	}
+
+	if (shid->desc.max_input_length != 0 &&
+	    header->report_length > shid->desc.max_input_length) {
+		dev_err(dev, "Input report body size %u > max expected of %u\n",
+			header->report_length, shid->desc.max_input_length);
+		return -EMSGSIZE;
+	}
+
+	if (header->last_fragment_flag != 1) {
+		dev_err(dev, "Multi-fragment reports not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (header->sync_const != SPI_HID_INPUT_HEADER_SYNC_BYTE) {
+		dev_err(dev, "Invalid input report sync constant (0x%x)\n",
+			header->sync_const);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int spi_hid_get_request(struct spi_hid *shid, u8 content_id)
 {
 	struct device *dev = &shid->spi->dev;
@@ -352,6 +839,8 @@ static int spi_hid_get_request(struct spi_hid *shid, u8 content_id)
 		dev_err(dev,
 			"Expected get request response not received! Error %d\n",
 			error);
+		set_bit(SPI_HID_ERROR, &shid->flags);
+		schedule_work(&shid->reset_work);
 		return error;
 	}
 
@@ -371,9 +860,83 @@ static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
 	return spi_hid_sync_request(shid, &report);
 }
 
-/* This is a placeholder. Will be implemented in the next patch. */
 static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 {
+	struct spi_hid *shid = _shid;
+	struct device *dev = &shid->spi->dev;
+	struct spi_hid_input_header header;
+	int error = 0;
+
+	scoped_guard(mutex, &shid->io_lock) {
+		error = spi_hid_input_sync(shid, shid->input->header,
+					   sizeof(shid->input->header), true);
+		if (error) {
+			dev_err(dev, "Failed to transfer header: %d\n", error);
+			goto err;
+		}
+
+		if (shid->power_state == HIDSPI_OFF) {
+			dev_warn(dev, "Device is off after header was received\n");
+			goto out;
+		}
+
+		if (shid->input_message.status < 0) {
+			dev_warn(dev, "Error reading header: %d\n",
+				 shid->input_message.status);
+			shid->bus_error_count++;
+			shid->bus_last_error = shid->input_message.status;
+			goto err;
+		}
+
+		spi_hid_populate_input_header(shid->input->header, &header);
+
+		error = spi_hid_bus_validate_header(shid, &header);
+		if (error) {
+			if (!test_bit(SPI_HID_RESET_PENDING, &shid->flags)) {
+				dev_err(dev, "Failed to validate header: %d\n", error);
+				print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
+					       DUMP_PREFIX_NONE, 16, 1, shid->input->header,
+					       sizeof(shid->input->header), false);
+				shid->bus_error_count++;
+				shid->bus_last_error = error;
+				goto err;
+			}
+			goto out;
+		}
+
+		error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
+					   false);
+		if (error) {
+			dev_err(dev, "Failed to transfer body: %d\n", error);
+			goto err;
+		}
+
+		if (shid->power_state == HIDSPI_OFF) {
+			dev_warn(dev, "Device is off after body was received\n");
+			goto out;
+		}
+
+		if (shid->input_message.status < 0) {
+			dev_warn(dev, "Error reading body: %d\n",
+				 shid->input_message.status);
+			shid->bus_error_count++;
+			shid->bus_last_error = shid->input_message.status;
+			goto err;
+		}
+	}
+
+	error = spi_hid_process_input_report(shid, shid->input);
+	if (error) {
+		dev_err(dev, "Failed to process input report: %d\n", error);
+		goto err;
+	}
+
+out:
+	return IRQ_HANDLED;
+
+err:
+	set_bit(SPI_HID_ERROR, &shid->flags);
+	schedule_work(&shid->reset_work);
 	return IRQ_HANDLED;
 }
 
@@ -610,10 +1173,13 @@ static int spi_hid_ll_output_report(struct hid_device *hid, __u8 *buf,
 		return -ENODEV;
 	}
 
-	if (shid->desc.no_output_report_ack)
-		error = spi_hid_send_output_report(shid, &report);
-	else
+	if (shid->desc.no_output_report_ack) {
+		scoped_guard(mutex, &shid->output_lock) {
+			error = spi_hid_send_output_report(shid, &report);
+		}
+	} else {
 		error = spi_hid_sync_request(shid, &report);
+	}
 
 	if (error) {
 		dev_err(dev, "failed to send output report\n");
@@ -701,14 +1267,23 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 	shid->power_state = HIDSPI_ON;
 	shid->ops = ops;
 	shid->conf = conf;
+	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
 
 	spi_set_drvdata(spi, shid);
 
+	/* Using now populated conf let's pre-calculate the read approvals */
+	spi_hid_populate_read_approvals(shid->conf, shid->read_approval_header,
+					shid->read_approval_body);
+
 	mutex_init(&shid->output_lock);
+	mutex_init(&shid->power_lock);
+	mutex_init(&shid->io_lock);
 	init_completion(&shid->output_done);
 
+	INIT_WORK(&shid->reset_work, spi_hid_reset_work);
+
 	/*
-	 * we need to allocate the buffer without knowing the maximum
+	 * We need to allocate the buffer without knowing the maximum
 	 * size of the reports. Let's use SZ_2K, then we do the
 	 * real computation later.
 	 */
@@ -731,7 +1306,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 	shid->ops->sleep_minimal_reset_delay(shid->ops);
 
 	error = devm_request_threaded_irq(dev, spi->irq, NULL, spi_hid_dev_irq,
-					  IRQF_ONESHOT, dev_name(&spi->dev), shid);
+					  IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(&spi->dev), shid);
 	if (error) {
 		dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
 		return error;
@@ -745,13 +1320,11 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 
 	shid->ops->deassert_reset(shid->ops);
 
+	enable_irq(spi->irq);
+
 	dev_dbg(dev, "%s: d3 -> %s\n", __func__,
 		spi_hid_power_mode_string(shid->power_state));
 
-	error = spi_hid_create_device(shid);
-	if (error)
-		return error;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spi_hid_core_probe);
@@ -762,6 +1335,9 @@ void spi_hid_core_remove(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	int error;
 
+	disable_irq(spi->irq);
+	cancel_work_sync(&shid->reset_work);
+
 	spi_hid_stop_hid(shid);
 
 	shid->ops->assert_reset(shid->ops);

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 06/11] HID: spi_hid: add spi_hid traces

[Jingyuan Liang]

Add traces for purposed of debugging spi_hid driver.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/Makefile        |   1 +
 drivers/hid/spi-hid/spi-hid-core.c  | 114 +++++++++---------------
 drivers/hid/spi-hid/spi-hid-core.h  |  91 +++++++++++++++++++
 drivers/hid/spi-hid/spi-hid-trace.h | 169 ++++++++++++++++++++++++++++++++++++
 4 files changed, 300 insertions(+), 75 deletions(-)

diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
index 92e24cddbfc2..733e006df56e 100644
--- a/drivers/hid/spi-hid/Makefile
+++ b/drivers/hid/spi-hid/Makefile
@@ -7,3 +7,4 @@
 
 obj-$(CONFIG_SPI_HID_CORE)	+= spi-hid.o
 spi-hid-objs 			= spi-hid-core.o
+CFLAGS_spi-hid-core.o 		:= -I$(src)
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index f6ea2d4365a7..ef527999d6dc 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -44,6 +44,11 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 
+#include "spi-hid-core.h"
+
+#define CREATE_TRACE_POINTS
+#include "spi-hid-trace.h"
+
 /* Protocol constants */
 #define SPI_HID_READ_APPROVAL_CONSTANT		0xff
 #define SPI_HID_INPUT_HEADER_SYNC_BYTE		0x5a
@@ -82,13 +87,6 @@
 #define SPI_HID_CREATE_DEVICE	4
 #define SPI_HID_ERROR	5
 
-/* Raw input buffer with data from the bus */
-struct spi_hid_input_buf {
-	u8 header[HIDSPI_INPUT_HEADER_SIZE];
-	u8 body[HIDSPI_INPUT_BODY_HEADER_SIZE];
-	u8 content[];
-};
-
 /* Processed data from input report header */
 struct spi_hid_input_header {
 	u8 version;
@@ -105,12 +103,6 @@ struct spi_hid_input_report {
 	u8 *content;
 };
 
-/* Raw output report buffer to be put on the bus */
-struct spi_hid_output_buf {
-	u8 header[SPI_HID_OUTPUT_HEADER_LEN];
-	u8 content[];
-};
-
 /* Data necessary to send an output report */
 struct spi_hid_output_report {
 	u8 report_type;
@@ -119,19 +111,6 @@ struct spi_hid_output_report {
 	u8 *content;
 };
 
-/* Processed data from a device descriptor */
-struct spi_hid_device_descriptor {
-	u16 hid_version;
-	u16 report_descriptor_length;
-	u16 max_input_length;
-	u16 max_output_length;
-	u16 max_fragment_length;
-	u16 vendor_id;
-	u16 product_id;
-	u16 version_id;
-	u8 no_output_report_ack;
-};
-
 /* struct spi_hid_conf - Conf provided to the core */
 struct spi_hid_conf {
 	u32 input_report_header_address;
@@ -157,55 +136,6 @@ struct spihid_ops {
 	void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
 };
 
-/* Driver context */
-struct spi_hid {
-	struct spi_device	*spi;	/* spi device. */
-	struct hid_device	*hid;	/* pointer to corresponding HID dev. */
-
-	struct spi_transfer	input_transfer[2];	/* Transfer buffer for read and write. */
-	struct spi_message	input_message;	/* used to execute a sequence of spi transfers. */
-
-	struct spihid_ops	*ops;
-	struct spi_hid_conf	*conf;
-
-	struct spi_hid_device_descriptor desc;	/* HID device descriptor. */
-	struct spi_hid_output_buf *output;	/* Output buffer. */
-	struct spi_hid_input_buf *input;	/* Input buffer. */
-	struct spi_hid_input_buf *response;	/* Response buffer. */
-
-	u16 response_length;
-	u16 bufsize;
-
-	enum hidspi_power_state power_state;
-
-	u8 reset_attempts;	/* The number of reset attempts. */
-
-	unsigned long flags;	/* device flags. */
-
-	struct work_struct reset_work;
-
-	/* Control lock to ensure complete output transaction. */
-	struct mutex output_lock;
-	/* Power lock to make sure one power state change at a time. */
-	struct mutex power_lock;
-	/* Protect bus I/O and shid->hid pointer lifecycle. */
-	struct mutex io_lock;
-
-	struct completion output_done;
-
-	u32 report_descriptor_crc32;	/* HID report descriptor crc32 checksum. */
-
-	u32 regulator_error_count;
-	int regulator_last_error;
-	u32 bus_error_count;
-	int bus_last_error;
-	u32 dir_count;	/* device initiated reset count. */
-
-	/* DMA-safe transfer buffers */
-	u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN] ____cacheline_aligned;
-	u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
-};
-
 static struct hid_ll_driver spi_hid_ll_driver;
 
 static void spi_hid_populate_read_approvals(const struct spi_hid_conf *conf,
@@ -295,6 +225,11 @@ static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
 	spi_message_init_with_transfers(&shid->input_message,
 					shid->input_transfer, 2);
 
+	trace_spi_hid_input_sync(shid,	shid->input_transfer[0].tx_buf,
+				 shid->input_transfer[0].len,
+				 shid->input_transfer[1].rx_buf,
+				 shid->input_transfer[1].len, 0);
+
 	error = spi_sync(shid->spi, &shid->input_message);
 	if (error) {
 		dev_err(&shid->spi->dev, "Error starting sync transfer: %d\n", error);
@@ -353,6 +288,8 @@ static void spi_hid_error_handler(struct spi_hid *shid)
 	struct device *dev = &shid->spi->dev;
 	int error;
 
+	trace_spi_hid_error_handler(shid);
+
 	guard(mutex)(&shid->power_lock);
 	if (shid->power_state == HIDSPI_OFF)
 		return;
@@ -477,6 +414,8 @@ static void spi_hid_reset_response(struct spi_hid *shid)
 	};
 	int error;
 
+	trace_spi_hid_reset_response(shid);
+
 	if (test_bit(SPI_HID_READY, &shid->flags)) {
 		dev_err(dev, "Spontaneous FW reset!\n");
 		clear_bit(SPI_HID_READY, &shid->flags);
@@ -503,6 +442,7 @@ static int spi_hid_input_report_handler(struct spi_hid *shid,
 	int error = 0;
 
 	guard(mutex)(&shid->io_lock);
+	trace_spi_hid_input_report_handler(shid);
 
 	if (!test_bit(SPI_HID_READY, &shid->flags) ||
 	    test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
@@ -528,6 +468,8 @@ static int spi_hid_input_report_handler(struct spi_hid *shid,
 static void spi_hid_response_handler(struct spi_hid *shid,
 				     struct input_report_body_header *body)
 {
+	trace_spi_hid_response_handler(shid);
+
 	shid->response_length = body->content_len;
 	/* completion_done returns 0 if there are waiters, otherwise 1 */
 	if (completion_done(&shid->output_done)) {
@@ -584,6 +526,8 @@ static int spi_hid_create_device(struct spi_hid *shid)
 	struct device *dev = &shid->spi->dev;
 	int error;
 
+	trace_spi_hid_create_device(shid);
+
 	hid = hid_allocate_device();
 	error = PTR_ERR_OR_ZERO(hid);
 	if (error) {
@@ -627,6 +571,8 @@ static void spi_hid_refresh_device(struct spi_hid *shid)
 	u32 new_crc32 = 0;
 	int error = 0;
 
+	trace_spi_hid_refresh_device(shid);
+
 	error = spi_hid_report_descriptor_request(shid);
 	if (error < 0) {
 		dev_err(dev,
@@ -708,6 +654,8 @@ static int spi_hid_process_input_report(struct spi_hid *shid,
 	struct device *dev = &shid->spi->dev;
 	struct hidspi_dev_descriptor *raw;
 
+	trace_spi_hid_process_input_report(shid);
+
 	spi_hid_populate_input_header(buf->header, &header);
 	spi_hid_populate_input_body(buf->body, &body);
 
@@ -867,6 +815,9 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 	struct spi_hid_input_header header;
 	int error = 0;
 
+	trace_spi_hid_dev_irq(shid, irq);
+	trace_spi_hid_header_transfer(shid);
+
 	scoped_guard(mutex, &shid->io_lock) {
 		error = spi_hid_input_sync(shid, shid->input->header,
 					   sizeof(shid->input->header), true);
@@ -880,6 +831,13 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 			goto out;
 		}
 
+		trace_spi_hid_input_header_complete(shid,
+						    shid->input_transfer[0].tx_buf,
+						    shid->input_transfer[0].len,
+						    shid->input_transfer[1].rx_buf,
+						    shid->input_transfer[1].len,
+						    shid->input_message.status);
+
 		if (shid->input_message.status < 0) {
 			dev_warn(dev, "Error reading header: %d\n",
 				 shid->input_message.status);
@@ -916,6 +874,12 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 			goto out;
 		}
 
+		trace_spi_hid_input_body_complete(shid, shid->input_transfer[0].tx_buf,
+						  shid->input_transfer[0].len,
+						  shid->input_transfer[1].rx_buf,
+						  shid->input_transfer[1].len,
+						  shid->input_message.status);
+
 		if (shid->input_message.status < 0) {
 			dev_warn(dev, "Error reading body: %d\n",
 				 shid->input_message.status);
diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
new file mode 100644
index 000000000000..293e2cfcfbf7
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ */
+
+#ifndef SPI_HID_CORE_H
+#define SPI_HID_CORE_H
+
+#include <linux/hid-over-spi.h>
+#include <linux/spi/spi.h>
+
+/* Protocol message size constants */
+#define SPI_HID_READ_APPROVAL_LEN		5
+#define SPI_HID_OUTPUT_HEADER_LEN		8
+
+/* Raw input buffer with data from the bus */
+struct spi_hid_input_buf {
+	u8 header[HIDSPI_INPUT_HEADER_SIZE];
+	u8 body[HIDSPI_INPUT_BODY_HEADER_SIZE];
+	u8 content[];
+};
+
+/* Raw output report buffer to be put on the bus */
+struct spi_hid_output_buf {
+	u8 header[SPI_HID_OUTPUT_HEADER_LEN];
+	u8 content[];
+};
+
+/* Processed data from a device descriptor */
+struct spi_hid_device_descriptor {
+	u16 hid_version;
+	u16 report_descriptor_length;
+	u16 max_input_length;
+	u16 max_output_length;
+	u16 max_fragment_length;
+	u16 vendor_id;
+	u16 product_id;
+	u16 version_id;
+	u8 no_output_report_ack;
+};
+
+/* Driver context */
+struct spi_hid {
+	struct spi_device	*spi;	/* spi device. */
+	struct hid_device	*hid;	/* pointer to corresponding HID dev. */
+
+	struct spi_transfer	input_transfer[2];	/* Transfer buffer for read and write. */
+	struct spi_message	input_message;	/* used to execute a sequence of spi transfers. */
+
+	struct spihid_ops	*ops;
+	struct spi_hid_conf	*conf;
+
+	struct spi_hid_device_descriptor desc;	/* HID device descriptor. */
+	struct spi_hid_output_buf *output;	/* Output buffer. */
+	struct spi_hid_input_buf *input;	/* Input buffer. */
+	struct spi_hid_input_buf *response;	/* Response buffer. */
+
+	u16 response_length;
+	u16 bufsize;
+
+	enum hidspi_power_state power_state;
+
+	u8 reset_attempts;	/* The number of reset attempts. */
+
+	unsigned long flags;	/* device flags. */
+
+	struct work_struct reset_work;
+
+	/* Control lock to ensure complete output transaction. */
+	struct mutex output_lock;
+	/* Power lock to make sure one power state change at a time. */
+	struct mutex power_lock;
+	/* I/O lock to prevent concurrent output writes during the input read. */
+	struct mutex io_lock;
+
+	struct completion output_done;
+
+	u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN];
+	u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
+
+	u32 report_descriptor_crc32;	/* HID report descriptor crc32 checksum. */
+
+	u32 regulator_error_count;
+	int regulator_last_error;
+	u32 bus_error_count;
+	int bus_last_error;
+	u32 dir_count;		/* device initiated reset count. */
+};
+
+#endif /* SPI_HID_CORE_H */
diff --git a/drivers/hid/spi-hid/spi-hid-trace.h b/drivers/hid/spi-hid/spi-hid-trace.h
new file mode 100644
index 000000000000..841ec491826d
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-trace.h
@@ -0,0 +1,169 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Microsoft Corporation
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM spi_hid
+
+#if !defined(_SPI_HID_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _SPI_HID_TRACE_H
+
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+#include "spi-hid-core.h"
+
+DECLARE_EVENT_CLASS(spi_hid_transfer,
+	TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+		 const void *rx_buf, u16 rx_len, int ret),
+
+	TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret),
+
+	TP_STRUCT__entry(
+		__field(int, bus_num)
+		__field(int, chip_select)
+		__field(int, ret)
+		__dynamic_array(u8, rx_buf, rx_len)
+		__dynamic_array(u8, tx_buf, tx_len)
+	),
+
+	TP_fast_assign(
+		__entry->bus_num = shid->spi->controller->bus_num;
+		__entry->chip_select = spi_get_chipselect(shid->spi, 0);
+		__entry->ret = ret;
+
+		memcpy(__get_dynamic_array(tx_buf), tx_buf, tx_len);
+		memcpy(__get_dynamic_array(rx_buf), rx_buf, rx_len);
+	),
+
+	TP_printk("spi%d.%d: len=%d tx=[%*phD] rx=[%*phD] --> %d",
+		  __entry->bus_num, __entry->chip_select,
+		  __get_dynamic_array_len(tx_buf) + __get_dynamic_array_len(rx_buf),
+		  __get_dynamic_array_len(tx_buf), __get_dynamic_array(tx_buf),
+		  __get_dynamic_array_len(rx_buf), __get_dynamic_array(rx_buf),
+		  __entry->ret)
+);
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_input_sync,
+	     TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+		      const void *rx_buf, u16 rx_len, int ret),
+	     TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret));
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_input_header_complete,
+	     TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+		      const void *rx_buf, u16 rx_len, int ret),
+	     TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret));
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_input_body_complete,
+	     TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+		      const void *rx_buf, u16 rx_len, int ret),
+	     TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret));
+
+DECLARE_EVENT_CLASS(spi_hid_irq,
+	TP_PROTO(struct spi_hid *shid, int irq),
+
+	TP_ARGS(shid, irq),
+
+	TP_STRUCT__entry(
+		__field(int, bus_num)
+		__field(int, chip_select)
+		__field(int, irq)
+	),
+
+	TP_fast_assign(
+		__entry->bus_num = shid->spi->controller->bus_num;
+		__entry->chip_select = spi_get_chipselect(shid->spi, 0);
+		__entry->irq = irq;
+	),
+
+	TP_printk("spi%d.%d: IRQ %d",
+		  __entry->bus_num, __entry->chip_select, __entry->irq)
+);
+
+DEFINE_EVENT(spi_hid_irq, spi_hid_dev_irq,
+	     TP_PROTO(struct spi_hid *shid, int irq), TP_ARGS(shid, irq));
+
+DECLARE_EVENT_CLASS(spi_hid,
+	TP_PROTO(struct spi_hid *shid),
+
+	TP_ARGS(shid),
+
+	TP_STRUCT__entry(
+		__field(int, bus_num)
+		__field(int, chip_select)
+		__field(int, power_state)
+		__field(u32, flags)
+
+		__field(int, vendor_id)
+		__field(int, product_id)
+		__field(int, max_input_length)
+		__field(int, max_output_length)
+		__field(u16, hid_version)
+		__field(u16, report_descriptor_length)
+		__field(u16, version_id)
+	),
+
+	TP_fast_assign(
+		__entry->bus_num = shid->spi->controller->bus_num;
+		__entry->chip_select = spi_get_chipselect(shid->spi, 0);
+		__entry->power_state = shid->power_state;
+		__entry->flags = shid->flags;
+
+		__entry->vendor_id = shid->desc.vendor_id;
+		__entry->product_id = shid->desc.product_id;
+		__entry->max_input_length = shid->desc.max_input_length;
+		__entry->max_output_length = shid->desc.max_output_length;
+		__entry->hid_version = shid->desc.hid_version;
+		__entry->report_descriptor_length =
+					shid->desc.report_descriptor_length;
+		__entry->version_id = shid->desc.version_id;
+	),
+
+	TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state p:%d len i:%d o:%d r:%d flags 0x%08x",
+		  __entry->bus_num, __entry->chip_select,
+		  __entry->vendor_id, __entry->product_id, __entry->version_id,
+		  __entry->hid_version >> 8, __entry->hid_version & 0xff,
+		  __entry->power_state,	__entry->max_input_length,
+		  __entry->max_output_length, __entry->report_descriptor_length,
+		  __entry->flags)
+);
+
+DEFINE_EVENT(spi_hid, spi_hid_header_transfer, TP_PROTO(struct spi_hid *shid),
+	     TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_process_input_report,
+	     TP_PROTO(struct spi_hid *shid), TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_input_report_handler,
+	     TP_PROTO(struct spi_hid *shid), TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_reset_response, TP_PROTO(struct spi_hid *shid),
+	     TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_create_device, TP_PROTO(struct spi_hid *shid),
+	     TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_refresh_device, TP_PROTO(struct spi_hid *shid),
+	     TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_response_handler, TP_PROTO(struct spi_hid *shid),
+	     TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_error_handler, TP_PROTO(struct spi_hid *shid),
+	     TP_ARGS(shid));
+
+#endif /* _SPI_HID_TRACE_H */
+
+/*
+ * The following must be outside the protection of the above #if block.
+ */
+#undef TRACE_INCLUDE_PATH
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_PATH .
+
+/*
+ * It is required that the TRACE_INCLUDE_FILE be the same
+ * as this file without the ".h".
+ */
+#define TRACE_INCLUDE_FILE spi-hid-trace
+#include <trace/define_trace.h>

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 07/11] HID: spi_hid: add ACPI support for SPI over HID

[Jingyuan Liang]

From: Angela Czubak <acz@semihalf.com>

Detect SPI HID devices described in ACPI.

Signed-off-by: Angela Czubak <acz@semihalf.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/Kconfig        |  15 +++
 drivers/hid/spi-hid/Makefile       |   1 +
 drivers/hid/spi-hid/spi-hid-acpi.c | 253 +++++++++++++++++++++++++++++++++++++
 drivers/hid/spi-hid/spi-hid-core.c |  26 +---
 drivers/hid/spi-hid/spi-hid.h      |  45 +++++++
 5 files changed, 315 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
index 836fdefe8345..114b1e00da39 100644
--- a/drivers/hid/spi-hid/Kconfig
+++ b/drivers/hid/spi-hid/Kconfig
@@ -10,6 +10,21 @@ menuconfig SPI_HID
 
 if SPI_HID
 
+config SPI_HID_ACPI
+	tristate "HID over SPI transport layer ACPI driver"
+	depends on ACPI
+	select SPI_HID_CORE
+	help
+	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+	  other HID based devices which are connected to your computer via SPI.
+	  This driver supports ACPI-based systems.
+
+	  If unsure, say N.
+
+	  This support is also available as a module.  If so, the module
+	  will be called spi-hid-acpi. It will also build/depend on the
+	  module spi-hid.
+
 config SPI_HID_CORE
 	tristate
 endif
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
index 733e006df56e..3ca326602643 100644
--- a/drivers/hid/spi-hid/Makefile
+++ b/drivers/hid/spi-hid/Makefile
@@ -8,3 +8,4 @@
 obj-$(CONFIG_SPI_HID_CORE)	+= spi-hid.o
 spi-hid-objs 			= spi-hid-core.o
 CFLAGS_spi-hid-core.o 		:= -I$(src)
+obj-$(CONFIG_SPI_HID_ACPI)	+= spi-hid-acpi.o
diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
new file mode 100644
index 000000000000..298e3ba44d8a
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-acpi.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol, ACPI related code
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ *
+ * This code was forked out of the HID over SPI core code, which is partially
+ * based on "HID over I2C protocol implementation:
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * which in turn is partially based on "USB HID support for Linux":
+ *
+ * Copyright (c) 1999 Andreas Gal
+ * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ * Copyright (c) 2007-2008 Oliver Neukum
+ * Copyright (c) 2006-2010 Jiri Kosina
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/reset.h>
+#include <linux/uuid.h>
+
+#include "spi-hid.h"
+
+/* Config structure is filled with data from ACPI */
+struct spi_hid_acpi_config {
+	struct spihid_ops ops;
+
+	struct spi_hid_conf property_conf;
+	u32 post_power_on_delay_ms;
+	u32 minimal_reset_delay_ms;
+	struct acpi_device *adev;
+};
+
+/* HID SPI Device: 6e2ac436-0fcf41af-a265-b32a220dcfab */
+static guid_t spi_hid_guid =
+	GUID_INIT(0x6E2AC436, 0x0FCF, 0x41AF,
+		  0xA2, 0x65, 0xB3, 0x2A, 0x22, 0x0D, 0xCF, 0xAB);
+
+static int spi_hid_acpi_populate_config(struct spi_hid_acpi_config *conf,
+					struct acpi_device *adev)
+{
+	acpi_handle handle = acpi_device_handle(adev);
+	union acpi_object *obj;
+
+	conf->adev = adev;
+
+	/* Revision 3 for HID over SPI V1, see specification. */
+	obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 1, NULL,
+				      ACPI_TYPE_INTEGER);
+	if (!obj) {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID input report header address failed");
+		return -ENODEV;
+	}
+	conf->property_conf.input_report_header_address = obj->integer.value;
+	ACPI_FREE(obj);
+
+	obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 2, NULL,
+				      ACPI_TYPE_INTEGER);
+	if (!obj) {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID input report body address failed");
+		return -ENODEV;
+	}
+	conf->property_conf.input_report_body_address = obj->integer.value;
+	ACPI_FREE(obj);
+
+	obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 3, NULL,
+				      ACPI_TYPE_INTEGER);
+	if (!obj) {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID output report header address failed");
+		return -ENODEV;
+	}
+	conf->property_conf.output_report_address = obj->integer.value;
+	ACPI_FREE(obj);
+
+	obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 4, NULL,
+				      ACPI_TYPE_BUFFER);
+	if (!obj) {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID read opcode failed");
+		return -ENODEV;
+	}
+	if (obj->buffer.length == 1) {
+		conf->property_conf.read_opcode = obj->buffer.pointer[0];
+	} else {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID read opcode, too long buffer");
+		ACPI_FREE(obj);
+		return -ENODEV;
+	}
+	ACPI_FREE(obj);
+
+	obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 5, NULL,
+				      ACPI_TYPE_BUFFER);
+	if (!obj) {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID write opcode failed");
+		return -ENODEV;
+	}
+	if (obj->buffer.length == 1) {
+		conf->property_conf.write_opcode = obj->buffer.pointer[0];
+	} else {
+		acpi_handle_err(handle,
+				"Error _DSM call to get HID write opcode, too long buffer");
+		ACPI_FREE(obj);
+		return -ENODEV;
+	}
+	ACPI_FREE(obj);
+
+	/* Value not provided in ACPI,*/
+	conf->post_power_on_delay_ms = 5;
+	conf->minimal_reset_delay_ms = 150;
+
+	if (!acpi_has_method(handle, "_RST")) {
+		acpi_handle_err(handle, "No reset method for acpi handle");
+		return -EINVAL;
+	}
+
+	/* FIXME: not reading hid-over-spi-flags, multi-SPI not supported */
+
+	return 0;
+}
+
+static int spi_hid_acpi_power_none(struct spihid_ops *ops)
+{
+	return 0;
+}
+
+static int spi_hid_acpi_power_down(struct spihid_ops *ops)
+{
+	struct spi_hid_acpi_config *conf = container_of(ops,
+							struct spi_hid_acpi_config,
+							ops);
+
+	return acpi_device_set_power(conf->adev, ACPI_STATE_D3);
+}
+
+static int spi_hid_acpi_power_up(struct spihid_ops *ops)
+{
+	struct spi_hid_acpi_config *conf = container_of(ops,
+							struct spi_hid_acpi_config,
+							ops);
+	int error;
+
+	error = acpi_device_set_power(conf->adev, ACPI_STATE_D0);
+	if (error) {
+		dev_err(&conf->adev->dev, "Error could not power up ACPI device: %d.", error);
+		return error;
+	}
+
+	if (conf->post_power_on_delay_ms)
+		msleep(conf->post_power_on_delay_ms);
+
+	return 0;
+}
+
+static int spi_hid_acpi_assert_reset(struct spihid_ops *ops)
+{
+	return 0;
+}
+
+static int spi_hid_acpi_deassert_reset(struct spihid_ops *ops)
+{
+	struct spi_hid_acpi_config *conf = container_of(ops,
+							struct spi_hid_acpi_config,
+							ops);
+
+	return device_reset(&conf->adev->dev);
+}
+
+static void spi_hid_acpi_sleep_minimal_reset_delay(struct spihid_ops *ops)
+{
+	struct spi_hid_acpi_config *conf = container_of(ops,
+							struct spi_hid_acpi_config,
+							ops);
+	fsleep(1000 * conf->minimal_reset_delay_ms);
+}
+
+static int spi_hid_acpi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct acpi_device *adev;
+	struct spi_hid_acpi_config *config;
+	int error;
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev) {
+		dev_err(dev, "Error could not get ACPI device.");
+		return -ENODEV;
+	}
+
+	config = devm_kzalloc(dev, sizeof(struct spi_hid_acpi_config),
+			      GFP_KERNEL);
+	if (!config)
+		return -ENOMEM;
+
+	if (acpi_device_power_manageable(adev)) {
+		config->ops.power_up = spi_hid_acpi_power_up;
+		config->ops.power_down = spi_hid_acpi_power_down;
+	} else {
+		config->ops.power_up = spi_hid_acpi_power_none;
+		config->ops.power_down = spi_hid_acpi_power_none;
+	}
+	config->ops.assert_reset = spi_hid_acpi_assert_reset;
+	config->ops.deassert_reset = spi_hid_acpi_deassert_reset;
+	config->ops.sleep_minimal_reset_delay =
+		spi_hid_acpi_sleep_minimal_reset_delay;
+
+	error = spi_hid_acpi_populate_config(config, adev);
+	if (error) {
+		dev_err(dev, "%s: unable to populate config data.", __func__);
+		return error;
+	}
+
+	return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
+}
+
+static const struct acpi_device_id spi_hid_acpi_match[] = {
+	{ "ACPI0C51", 0 },
+	{ "PNP0C51", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, spi_hid_acpi_match);
+
+static struct spi_driver spi_hid_acpi_driver = {
+	.driver = {
+		.name	= "spi_hid_acpi",
+		.owner	= THIS_MODULE,
+		.acpi_match_table = spi_hid_acpi_match,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.dev_groups = spi_hid_groups,
+	},
+	.probe		= spi_hid_acpi_probe,
+	.remove		= spi_hid_core_remove,
+};
+
+module_spi_driver(spi_hid_acpi_driver);
+
+MODULE_DESCRIPTION("HID over SPI ACPI transport driver");
+MODULE_AUTHOR("Angela Czubak <aczubak@google.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index ef527999d6dc..698e72102c11 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -44,6 +44,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 
+#include "spi-hid.h"
 #include "spi-hid-core.h"
 
 #define CREATE_TRACE_POINTS
@@ -111,31 +112,6 @@ struct spi_hid_output_report {
 	u8 *content;
 };
 
-/* struct spi_hid_conf - Conf provided to the core */
-struct spi_hid_conf {
-	u32 input_report_header_address;
-	u32 input_report_body_address;
-	u32 output_report_address;
-	u8 read_opcode;
-	u8 write_opcode;
-};
-
-/**
- * struct spihid_ops - Ops provided to the core
- * @power_up: do sequencing to power up the device
- * @power_down: do sequencing to power down the device
- * @assert_reset: do sequencing to assert the reset line
- * @deassert_reset: do sequencing to deassert the reset line
- * @sleep_minimal_reset_delay: minimal sleep delay during reset
- */
-struct spihid_ops {
-	int (*power_up)(struct spihid_ops *ops);
-	int (*power_down)(struct spihid_ops *ops);
-	int (*assert_reset)(struct spihid_ops *ops);
-	int (*deassert_reset)(struct spihid_ops *ops);
-	void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
-};
-
 static struct hid_ll_driver spi_hid_ll_driver;
 
 static void spi_hid_populate_read_approvals(const struct spi_hid_conf *conf,
diff --git a/drivers/hid/spi-hid/spi-hid.h b/drivers/hid/spi-hid/spi-hid.h
new file mode 100644
index 000000000000..f5a5f4d54beb
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ */
+
+#ifndef SPI_HID_H
+#define SPI_HID_H
+
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+
+/* struct spi_hid_conf - Conf provided to the core */
+struct spi_hid_conf {
+	u32 input_report_header_address;
+	u32 input_report_body_address;
+	u32 output_report_address;
+	u8 read_opcode;
+	u8 write_opcode;
+};
+
+/**
+ * struct spihid_ops - Ops provided to the core
+ * @power_up: do sequencing to power up the device
+ * @power_down: do sequencing to power down the device
+ * @assert_reset: do sequencing to assert the reset line
+ * @deassert_reset: do sequencing to deassert the reset line
+ * @sleep_minimal_reset_delay: minimal sleep delay during reset
+ */
+struct spihid_ops {
+	int (*power_up)(struct spihid_ops *ops);
+	int (*power_down)(struct spihid_ops *ops);
+	int (*assert_reset)(struct spihid_ops *ops);
+	int (*deassert_reset)(struct spihid_ops *ops);
+	void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
+};
+
+int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
+		       struct spi_hid_conf *conf);
+
+void spi_hid_core_remove(struct spi_device *spi);
+
+extern const struct attribute_group *spi_hid_groups[];
+
+#endif /* SPI_HID_H */

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 08/11] HID: spi_hid: add device tree support for SPI over HID

[Jingyuan Liang]

From: Jarrett Schultz <jaschultz@microsoft.com>

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/Kconfig      |  15 +++
 drivers/hid/spi-hid/Makefile     |   1 +
 drivers/hid/spi-hid/spi-hid-of.c | 246 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 262 insertions(+)

diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
index 114b1e00da39..76a2cd587a3e 100644
--- a/drivers/hid/spi-hid/Kconfig
+++ b/drivers/hid/spi-hid/Kconfig
@@ -25,6 +25,21 @@ config SPI_HID_ACPI
 	  will be called spi-hid-acpi. It will also build/depend on the
 	  module spi-hid.
 
+config SPI_HID_OF
+	tristate "HID over SPI transport layer Open Firmware driver"
+	depends on OF
+	select SPI_HID_CORE
+	help
+	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+	  other HID based devices which are connected to your computer via SPI.
+	  This driver supports Open Firmware (Device Tree)-based systems.
+
+	  If unsure, say N.
+
+	  This support is also available as a module.  If so, the module
+	  will be called spi-hid-of. It will also build/depend on the
+	  module spi-hid.
+
 config SPI_HID_CORE
 	tristate
 endif
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
index 3ca326602643..31192e71edae 100644
--- a/drivers/hid/spi-hid/Makefile
+++ b/drivers/hid/spi-hid/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_SPI_HID_CORE)	+= spi-hid.o
 spi-hid-objs 			= spi-hid-core.o
 CFLAGS_spi-hid-core.o 		:= -I$(src)
 obj-$(CONFIG_SPI_HID_ACPI)	+= spi-hid-acpi.o
+obj-$(CONFIG_SPI_HID_OF)	+= spi-hid-of.o
diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
new file mode 100644
index 000000000000..ba7d5338f5d8
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol, Open Firmware related code
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ *
+ * This code was forked out of the HID over SPI core code, which is partially
+ * based on "HID over I2C protocol implementation:
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * which in turn is partially based on "USB HID support for Linux":
+ *
+ * Copyright (c) 1999 Andreas Gal
+ * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ * Copyright (c) 2007-2008 Oliver Neukum
+ * Copyright (c) 2006-2010 Jiri Kosina
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+
+#include "spi-hid.h"
+
+struct spi_hid_timing_data {
+	u32 post_power_on_delay_ms;
+	u32 minimal_reset_delay_ms;
+};
+
+/* Config structure is filled with data from Device Tree */
+struct spi_hid_of_config {
+	struct spihid_ops ops;
+
+	struct spi_hid_conf property_conf;
+	const struct spi_hid_timing_data *timing_data;
+
+	struct gpio_desc *reset_gpio;
+	struct regulator *supply;
+	bool supply_enabled;
+	u16 hid_over_spi_flags;
+};
+
+static const struct spi_hid_timing_data timing_data = {
+	.post_power_on_delay_ms = 10,
+	.minimal_reset_delay_ms = 100,
+};
+
+static int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
+				      struct device *dev)
+{
+	int error;
+	u32 val;
+
+	error = device_property_read_u32(dev, "input-report-header-address",
+					 &val);
+	if (error) {
+		dev_err(dev, "Input report header address not provided.");
+		return -ENODEV;
+	}
+	conf->property_conf.input_report_header_address = val;
+
+	error = device_property_read_u32(dev, "input-report-body-address", &val);
+	if (error) {
+		dev_err(dev, "Input report body address not provided.");
+		return -ENODEV;
+	}
+	conf->property_conf.input_report_body_address = val;
+
+	error = device_property_read_u32(dev, "output-report-address", &val);
+	if (error) {
+		dev_err(dev, "Output report address not provided.");
+		return -ENODEV;
+	}
+	conf->property_conf.output_report_address = val;
+
+	error = device_property_read_u32(dev, "read-opcode", &val);
+	if (error) {
+		dev_err(dev, "Read opcode not provided.");
+		return -ENODEV;
+	}
+	conf->property_conf.read_opcode = val;
+
+	error = device_property_read_u32(dev, "write-opcode", &val);
+	if (error) {
+		dev_err(dev, "Write opcode not provided.");
+		return -ENODEV;
+	}
+	conf->property_conf.write_opcode = val;
+
+	conf->supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(conf->supply)) {
+		if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get regulator: %ld.",
+				PTR_ERR(conf->supply));
+		return PTR_ERR(conf->supply);
+	}
+	conf->supply_enabled = false;
+
+	conf->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(conf->reset_gpio)) {
+		dev_err(dev, "%s: error getting reset GPIO.", __func__);
+		return PTR_ERR(conf->reset_gpio);
+	}
+
+	return 0;
+}
+
+static int spi_hid_of_power_down(struct spihid_ops *ops)
+{
+	struct spi_hid_of_config *conf = container_of(ops,
+						      struct spi_hid_of_config,
+						      ops);
+	int error;
+
+	if (!conf->supply_enabled)
+		return 0;
+
+	error = regulator_disable(conf->supply);
+	if (error == 0)
+		conf->supply_enabled = false;
+
+	return error;
+}
+
+static int spi_hid_of_power_up(struct spihid_ops *ops)
+{
+	struct spi_hid_of_config *conf = container_of(ops,
+						      struct spi_hid_of_config,
+						      ops);
+	int error;
+
+	if (conf->supply_enabled)
+		return 0;
+
+	error = regulator_enable(conf->supply);
+
+	if (error == 0) {
+		conf->supply_enabled = true;
+		fsleep(1000 * conf->timing_data->post_power_on_delay_ms);
+	}
+
+	return error;
+}
+
+static int spi_hid_of_assert_reset(struct spihid_ops *ops)
+{
+	struct spi_hid_of_config *conf = container_of(ops,
+						      struct spi_hid_of_config,
+						      ops);
+
+	gpiod_set_value(conf->reset_gpio, 1);
+	return 0;
+}
+
+static int spi_hid_of_deassert_reset(struct spihid_ops *ops)
+{
+	struct spi_hid_of_config *conf = container_of(ops,
+						      struct spi_hid_of_config,
+						      ops);
+
+	gpiod_set_value(conf->reset_gpio, 0);
+	return 0;
+}
+
+static void spi_hid_of_sleep_minimal_reset_delay(struct spihid_ops *ops)
+{
+	struct spi_hid_of_config *conf = container_of(ops,
+						      struct spi_hid_of_config,
+						      ops);
+	fsleep(1000 * conf->timing_data->minimal_reset_delay_ms);
+}
+
+static int spi_hid_of_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct spi_hid_of_config *config;
+	int error;
+
+	config = devm_kzalloc(dev, sizeof(struct spi_hid_of_config),
+			      GFP_KERNEL);
+	if (!config)
+		return -ENOMEM;
+
+	config->ops.power_up = spi_hid_of_power_up;
+	config->ops.power_down = spi_hid_of_power_down;
+	config->ops.assert_reset = spi_hid_of_assert_reset;
+	config->ops.deassert_reset = spi_hid_of_deassert_reset;
+	config->ops.sleep_minimal_reset_delay =
+		spi_hid_of_sleep_minimal_reset_delay;
+
+	config->timing_data = device_get_match_data(dev);
+	if (!config->timing_data)
+		config->timing_data = &timing_data;
+
+	/*
+	 * FIXME: hid_over_spi_flags could be retrieved from spi mode.
+	 * It is always 0 because multi-SPI not supported.
+	 */
+	config->hid_over_spi_flags = 0;
+
+	error = spi_hid_of_populate_config(config, dev);
+	if (error) {
+		dev_err(dev, "%s: unable to populate config data.", __func__);
+		return error;
+	}
+
+	return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
+}
+
+static const struct of_device_id spi_hid_of_match[] = {
+	{ .compatible = "hid-over-spi", .data = &timing_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, spi_hid_of_match);
+
+static const struct spi_device_id spi_hid_of_id_table[] = {
+	{ "hid", 0 },
+	{ "hid-over-spi", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, spi_hid_of_id_table);
+
+static struct spi_driver spi_hid_of_driver = {
+	.driver = {
+		.name	= "spi_hid_of",
+		.owner	= THIS_MODULE,
+		.of_match_table = spi_hid_of_match,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.dev_groups = spi_hid_groups,
+	},
+	.probe		= spi_hid_of_probe,
+	.remove		= spi_hid_core_remove,
+	.id_table	= spi_hid_of_id_table,
+};
+
+module_spi_driver(spi_hid_of_driver);
+
+MODULE_DESCRIPTION("HID over SPI OF transport driver");
+MODULE_AUTHOR("Dmitry Antipov <dmanti@microsoft.com>");
+MODULE_LICENSE("GPL");

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 09/11] dt-bindings: input: Document hid-over-spi DT schema

[Jingyuan Liang]

Documentation describes the required and optional properties for
implementing Device Tree for a Microsoft G6 Touch Digitizer that
supports HID over SPI Protocol 1.0 specification.

The properties are common to HID over SPI.

Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 .../devicetree/bindings/input/hid-over-spi.yaml    | 128 +++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
new file mode 100644
index 000000000000..27cf311e0aab
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
@@ -0,0 +1,128 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/hid-over-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HID over SPI Devices
+
+maintainers:
+  - Benjamin Tissoires <benjamin.tissoires@redhat.com>
+  - Jiri Kosina <jkosina@suse.cz>
+  - Jingyuan Liang <jingyliang@chromium.org>
+
+description: |+
+  HID over SPI provides support for various Human Interface Devices over the
+  SPI bus. These devices can be for example touchpads, keyboards, touch screens
+  or sensors.
+
+  The specification has been written by Microsoft and is currently available
+  here: https://www.microsoft.com/en-us/download/details.aspx?id=103325
+
+  The Microsoft HID over SPI specification explicitly dictates that SPI
+  opcodes and register addresses (such as input/output report addresses)
+  are not standardized. Instead, the specification requires the system
+  firmware (e.g., ACPI or Device Tree) to provide these board-specific
+  parameters to the OS. Therefore, these varying parameters must be
+  defined as properties in the Device Tree.
+
+allOf:
+  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - microsoft,g6-touch-digitizer
+      - const: hid-over-spi
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      GPIO specifier for the digitizer's reset pin (active low). The line must
+      be flagged with GPIO_ACTIVE_LOW.
+
+  vdd-supply:
+    description:
+      Regulator for the VDD supply voltage.
+
+  input-report-header-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 0xffffff
+    description:
+      A value to be included in the Read Approval packet, listing an address of
+      the input report header to be put on the SPI bus. This address has 24
+      bits.
+
+  input-report-body-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 0xffffff
+    description:
+      A value to be included in the Read Approval packet, listing an address of
+      the input report body to be put on the SPI bus. This address has 24 bits.
+
+  output-report-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 0xffffff
+    description:
+      A value to be included in the Output Report sent by the host, listing an
+      address where the output report on the SPI bus is to be written to. This
+      address has 24 bits.
+
+  read-opcode:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Value to be used in Read Approval packets. 1 byte.
+
+  write-opcode:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Value to be used in Write Approval packets. 1 byte.
+
+required:
+  - compatible
+  - interrupts
+  - reset-gpios
+  - vdd-supply
+  - input-report-header-address
+  - input-report-body-address
+  - output-report-address
+  - read-opcode
+  - write-opcode
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      hid@0 {
+        compatible = "microsoft,g6-touch-digitizer", "hid-over-spi";
+        reg = <0x0>;
+        interrupts-extended = <&gpio 42 IRQ_TYPE_EDGE_FALLING>;
+        reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+        vdd-supply = <&pm8350c_l3>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&ts_d6_int_bias>;
+        input-report-header-address = <0x1000>;
+        input-report-body-address = <0x1004>;
+        output-report-address = <0x2000>;
+        read-opcode = /bits/ 8 <0x0b>;
+        write-opcode = /bits/ 8 <0x02>;
+      };
+    };

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 10/11] HID: spi-hid: add power management implementation

[Jingyuan Liang]

Implement HID over SPI driver power management callbacks.

Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/spi-hid-acpi.c |   1 +
 drivers/hid/spi-hid/spi-hid-core.c | 133 +++++++++++++++++++++++++++++++++++--
 drivers/hid/spi-hid/spi-hid-of.c   |   1 +
 drivers/hid/spi-hid/spi-hid.h      |   1 +
 4 files changed, 131 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
index 298e3ba44d8a..15cfc4e6cc2f 100644
--- a/drivers/hid/spi-hid/spi-hid-acpi.c
+++ b/drivers/hid/spi-hid/spi-hid-acpi.c
@@ -238,6 +238,7 @@ static struct spi_driver spi_hid_acpi_driver = {
 	.driver = {
 		.name	= "spi_hid_acpi",
 		.owner	= THIS_MODULE,
+		.pm	= &spi_hid_core_pm,
 		.acpi_match_table = spi_hid_acpi_match,
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 		.dev_groups = spi_hid_groups,
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 698e72102c11..517f06913477 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -36,6 +36,8 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/string.h>
@@ -245,6 +247,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
 	}
 }
 
+static int spi_hid_suspend(struct spi_hid *shid)
+{
+	int error;
+	struct device *dev = &shid->spi->dev;
+
+	guard(mutex)(&shid->power_lock);
+	if (shid->power_state == HIDSPI_OFF)
+		return 0;
+
+	if (shid->hid) {
+		error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
+		if (error) {
+			dev_err(dev, "%s failed to suspend hid driver: %d\n",
+				__func__, error);
+			return error;
+		}
+	}
+
+	disable_irq(shid->spi->irq);
+
+	if (!device_may_wakeup(dev)) {
+		set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+
+		shid->ops->assert_reset(shid->ops);
+
+		error = shid->ops->power_down(shid->ops);
+		if (error) {
+			dev_err(dev, "%s: could not power down\n", __func__);
+			shid->regulator_error_count++;
+			shid->regulator_last_error = error;
+			/* Undo partial suspend before returning error */
+			shid->ops->deassert_reset(shid->ops);
+			clear_bit(SPI_HID_RESET_PENDING, &shid->flags);
+			enable_irq(shid->spi->irq);
+			if (shid->hid)
+				hid_driver_reset_resume(shid->hid);
+			return error;
+		}
+
+		shid->power_state = HIDSPI_OFF;
+	}
+	return 0;
+}
+
+static int spi_hid_resume(struct spi_hid *shid)
+{
+	int error;
+	struct device *dev = &shid->spi->dev;
+
+	guard(mutex)(&shid->power_lock);
+
+	if (!device_may_wakeup(dev)) {
+		if (shid->power_state == HIDSPI_OFF) {
+			shid->ops->assert_reset(shid->ops);
+
+			shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+			error = shid->ops->power_up(shid->ops);
+			if (error) {
+				dev_err(dev, "%s: could not power up\n", __func__);
+				shid->regulator_error_count++;
+				shid->regulator_last_error = error;
+				return error;
+			}
+			shid->power_state = HIDSPI_ON;
+			shid->ops->deassert_reset(shid->ops);
+		}
+	}
+
+	enable_irq(shid->spi->irq);
+
+	if (shid->hid) {
+		error = hid_driver_reset_resume(shid->hid);
+		if (error) {
+			dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
+				__func__, error);
+			/* Undo partial resume before returning error */
+			disable_irq(shid->spi->irq);
+			if (!device_may_wakeup(dev)) {
+				set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+				shid->ops->assert_reset(shid->ops);
+				shid->ops->power_down(shid->ops);
+				shid->power_state = HIDSPI_OFF;
+			}
+			return error;
+		}
+	}
+	return 0;
+}
+
 static void spi_hid_stop_hid(struct spi_hid *shid)
 {
 	struct hid_device *hid;
@@ -795,6 +887,11 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 	trace_spi_hid_header_transfer(shid);
 
 	scoped_guard(mutex, &shid->io_lock) {
+		if (shid->power_state == HIDSPI_OFF) {
+			dev_warn(dev, "Device is off, ignoring interrupt\n");
+			goto out;
+		}
+
 		error = spi_hid_input_sync(shid, shid->input->header,
 					   sizeof(shid->input->header), true);
 		if (error) {
@@ -802,11 +899,6 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
 			goto err;
 		}
 
-		if (shid->power_state == HIDSPI_OFF) {
-			dev_warn(dev, "Device is off after header was received\n");
-			goto out;
-		}
-
 		trace_spi_hid_input_header_complete(shid,
 						    shid->input_transfer[0].tx_buf,
 						    shid->input_transfer[0].len,
@@ -1251,10 +1343,19 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 		dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
 		return error;
 	}
+	if (device_may_wakeup(dev)) {
+		error = dev_pm_set_wake_irq(dev, spi->irq);
+		if (error) {
+			dev_err(dev, "%s: failed to set wake IRQ\n", __func__);
+			return error;
+		}
+	}
 
 	error = shid->ops->power_up(shid->ops);
 	if (error) {
 		dev_err(dev, "%s: could not power up\n", __func__);
+		if (device_may_wakeup(dev))
+			dev_pm_clear_wake_irq(dev);
 		return error;
 	}
 
@@ -1284,9 +1385,31 @@ void spi_hid_core_remove(struct spi_device *spi)
 	error = shid->ops->power_down(shid->ops);
 	if (error)
 		dev_err(dev, "failed to disable regulator\n");
+
+	if (device_may_wakeup(dev))
+		dev_pm_clear_wake_irq(dev);
 }
 EXPORT_SYMBOL_GPL(spi_hid_core_remove);
 
+static int spi_hid_core_pm_suspend(struct device *dev)
+{
+	struct spi_hid *shid = dev_get_drvdata(dev);
+
+	return spi_hid_suspend(shid);
+}
+
+static int spi_hid_core_pm_resume(struct device *dev)
+{
+	struct spi_hid *shid = dev_get_drvdata(dev);
+
+	return spi_hid_resume(shid);
+}
+
+const struct dev_pm_ops spi_hid_core_pm = {
+	SYSTEM_SLEEP_PM_OPS(spi_hid_core_pm_suspend, spi_hid_core_pm_resume)
+};
+EXPORT_SYMBOL_GPL(spi_hid_core_pm);
+
 MODULE_DESCRIPTION("HID over SPI transport driver");
 MODULE_AUTHOR("Dmitry Antipov <dmanti@microsoft.com>");
 MODULE_LICENSE("GPL");
diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
index ba7d5338f5d8..561cf453e44a 100644
--- a/drivers/hid/spi-hid/spi-hid-of.c
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -230,6 +230,7 @@ static struct spi_driver spi_hid_of_driver = {
 	.driver = {
 		.name	= "spi_hid_of",
 		.owner	= THIS_MODULE,
+		.pm	= &spi_hid_core_pm,
 		.of_match_table = spi_hid_of_match,
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 		.dev_groups = spi_hid_groups,
diff --git a/drivers/hid/spi-hid/spi-hid.h b/drivers/hid/spi-hid/spi-hid.h
index f5a5f4d54beb..17b2fdf192ed 100644
--- a/drivers/hid/spi-hid/spi-hid.h
+++ b/drivers/hid/spi-hid/spi-hid.h
@@ -41,5 +41,6 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 void spi_hid_core_remove(struct spi_device *spi);
 
 extern const struct attribute_group *spi_hid_groups[];
+extern const struct dev_pm_ops spi_hid_core_pm;
 
 #endif /* SPI_HID_H */

-- 
2.54.0.1064.gd145956f57-goog

[PATCH v4 11/11] HID: spi-hid: add panel follower support

[Jingyuan Liang]

Add support to spi-hid to be a panel follower.

Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
 drivers/hid/spi-hid/spi-hid-core.c | 164 ++++++++++++++++++++++++++++++-------
 drivers/hid/spi-hid/spi-hid-core.h |   7 ++
 2 files changed, 142 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 517f06913477..27f25d95ed28 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -1281,6 +1281,106 @@ const struct attribute_group *spi_hid_groups[] = {
 };
 EXPORT_SYMBOL_GPL(spi_hid_groups);
 
+/*
+ * At the end of probe we initialize the device:
+ *   0) assert reset, bias the interrupt line
+ *   1) sleep minimal reset delay
+ *   2) power up the device
+ *   3) deassert reset (high)
+ * After this we expect an IRQ with a reset response.
+ */
+static int spi_hid_dev_init(struct spi_hid *shid)
+{
+	struct spi_device *spi = shid->spi;
+	struct device *dev = &spi->dev;
+	int error;
+
+	shid->ops->assert_reset(shid->ops);
+
+	shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+	error = shid->ops->power_up(shid->ops);
+	if (error) {
+		dev_err(dev, "%s: could not power up\n", __func__);
+		shid->regulator_error_count++;
+		shid->regulator_last_error = error;
+		return error;
+	}
+
+	shid->ops->deassert_reset(shid->ops);
+
+	enable_irq(spi->irq);
+
+	return 0;
+}
+
+static void spi_hid_panel_follower_work(struct work_struct *work)
+{
+	struct spi_hid *shid = container_of(work, struct spi_hid,
+					    panel_follower_work);
+	int error;
+
+	if (!shid->desc.hid_version)
+		error = spi_hid_dev_init(shid);
+	else
+		error = spi_hid_resume(shid);
+	if (error)
+		dev_warn(&shid->spi->dev, "Power on failed: %d\n", error);
+	else
+		WRITE_ONCE(shid->panel_follower_work_finished, true);
+}
+
+static int spi_hid_panel_follower_resume(struct drm_panel_follower *follower)
+{
+	struct spi_hid *shid = container_of(follower, struct spi_hid, panel_follower);
+
+	/*
+	 * Powering on a touchscreen can be a slow process. Queue the work to
+	 * the system workqueue so we don't block the panel's power up.
+	 */
+	WRITE_ONCE(shid->panel_follower_work_finished, false);
+	schedule_work(&shid->panel_follower_work);
+
+	return 0;
+}
+
+static int spi_hid_panel_follower_suspend(struct drm_panel_follower *follower)
+{
+	struct spi_hid *shid = container_of(follower, struct spi_hid, panel_follower);
+
+	cancel_work_sync(&shid->panel_follower_work);
+
+	if (!READ_ONCE(shid->panel_follower_work_finished))
+		return 0;
+
+	return spi_hid_suspend(shid);
+}
+
+static const struct drm_panel_follower_funcs
+				spi_hid_panel_follower_prepare_funcs = {
+	.panel_prepared = spi_hid_panel_follower_resume,
+	.panel_unpreparing = spi_hid_panel_follower_suspend,
+};
+
+static int spi_hid_register_panel_follower(struct spi_hid *shid)
+{
+	struct device *dev = &shid->spi->dev;
+
+	shid->panel_follower.funcs = &spi_hid_panel_follower_prepare_funcs;
+
+	/*
+	 * If we're not in control of our own power up/power down then we can't
+	 * do the logic to manage wakeups. Give a warning if a user thought
+	 * that was possible then force the capability off.
+	 */
+	if (device_can_wakeup(dev)) {
+		dev_warn(dev, "Can't wakeup if following panel\n");
+		device_set_wakeup_capable(dev, false);
+	}
+
+	return drm_panel_add_follower(dev, &shid->panel_follower);
+}
+
 int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 		       struct spi_hid_conf *conf)
 {
@@ -1300,6 +1400,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 	shid->ops = ops;
 	shid->conf = conf;
 	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+	shid->is_panel_follower = drm_is_panel_follower(&spi->dev);
 
 	spi_set_drvdata(spi, shid);
 
@@ -1313,6 +1414,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 	init_completion(&shid->output_done);
 
 	INIT_WORK(&shid->reset_work, spi_hid_reset_work);
+	INIT_WORK(&shid->panel_follower_work, spi_hid_panel_follower_work);
 
 	/*
 	 * We need to allocate the buffer without knowing the maximum
@@ -1323,20 +1425,6 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 	if (error)
 		return error;
 
-	/*
-	 * At the end of probe we initialize the device:
-	 *   0) assert reset, bias the interrupt line
-	 *   1) sleep minimal reset delay
-	 *   2) request IRQ
-	 *   3) power up the device
-	 *   4) deassert reset (high)
-	 * After this we expect an IRQ with a reset response.
-	 */
-
-	shid->ops->assert_reset(shid->ops);
-
-	shid->ops->sleep_minimal_reset_delay(shid->ops);
-
 	error = devm_request_threaded_irq(dev, spi->irq, NULL, spi_hid_dev_irq,
 					  IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(&spi->dev), shid);
 	if (error) {
@@ -1351,22 +1439,28 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
 		}
 	}
 
-	error = shid->ops->power_up(shid->ops);
-	if (error) {
-		dev_err(dev, "%s: could not power up\n", __func__);
-		if (device_may_wakeup(dev))
-			dev_pm_clear_wake_irq(dev);
-		return error;
+	if (shid->is_panel_follower) {
+		error = spi_hid_register_panel_follower(shid);
+		if (error) {
+			dev_err_probe(dev, error,
+				      "Failed to register panel follower");
+			goto err_wake_irq;
+		}
+	} else {
+		error = spi_hid_dev_init(shid);
+		if (error)
+			goto err_wake_irq;
 	}
 
-	shid->ops->deassert_reset(shid->ops);
-
-	enable_irq(spi->irq);
-
 	dev_dbg(dev, "%s: d3 -> %s\n", __func__,
 		spi_hid_power_mode_string(shid->power_state));
 
 	return 0;
+
+err_wake_irq:
+	if (device_may_wakeup(dev))
+		dev_pm_clear_wake_irq(dev);
+	return error;
 }
 EXPORT_SYMBOL_GPL(spi_hid_core_probe);
 
@@ -1376,15 +1470,21 @@ void spi_hid_core_remove(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	int error;
 
-	disable_irq(spi->irq);
+	if (shid->is_panel_follower)
+		drm_panel_remove_follower(&shid->panel_follower);
+	else
+		disable_irq(spi->irq);
+
 	cancel_work_sync(&shid->reset_work);
 
 	spi_hid_stop_hid(shid);
 
-	shid->ops->assert_reset(shid->ops);
-	error = shid->ops->power_down(shid->ops);
-	if (error)
-		dev_err(dev, "failed to disable regulator\n");
+	if (shid->power_state != HIDSPI_OFF) {
+		shid->ops->assert_reset(shid->ops);
+		error = shid->ops->power_down(shid->ops);
+		if (error)
+			dev_err(dev, "failed to disable regulator\n");
+	}
 
 	if (device_may_wakeup(dev))
 		dev_pm_clear_wake_irq(dev);
@@ -1395,6 +1495,9 @@ static int spi_hid_core_pm_suspend(struct device *dev)
 {
 	struct spi_hid *shid = dev_get_drvdata(dev);
 
+	if (shid->is_panel_follower)
+		return 0;
+
 	return spi_hid_suspend(shid);
 }
 
@@ -1402,6 +1505,9 @@ static int spi_hid_core_pm_resume(struct device *dev)
 {
 	struct spi_hid *shid = dev_get_drvdata(dev);
 
+	if (shid->is_panel_follower)
+		return 0;
+
 	return spi_hid_resume(shid);
 }
 
diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
index 293e2cfcfbf7..261b2fd7f332 100644
--- a/drivers/hid/spi-hid/spi-hid-core.h
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -10,6 +10,8 @@
 #include <linux/hid-over-spi.h>
 #include <linux/spi/spi.h>
 
+#include <drm/drm_panel.h>
+
 /* Protocol message size constants */
 #define SPI_HID_READ_APPROVAL_LEN		5
 #define SPI_HID_OUTPUT_HEADER_LEN		8
@@ -56,6 +58,10 @@ struct spi_hid {
 	struct spi_hid_input_buf *input;	/* Input buffer. */
 	struct spi_hid_input_buf *response;	/* Response buffer. */
 
+	struct drm_panel_follower panel_follower;
+	bool	is_panel_follower;
+	bool	panel_follower_work_finished;
+
 	u16 response_length;
 	u16 bufsize;
 
@@ -66,6 +72,7 @@ struct spi_hid {
 	unsigned long flags;	/* device flags. */
 
 	struct work_struct reset_work;
+	struct work_struct panel_follower_work;
 
 	/* Control lock to ensure complete output transaction. */
 	struct mutex output_lock;

-- 
2.54.0.1064.gd145956f57-goog

Re: [PATCH v4 09/11] dt-bindings: input: Document hid-over-spi DT schema

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `read-opcode` and `write-opcode` properties are defined as `uint8`, which will cause the driver to fail probing with `-EOVERFLOW` if using u32 reads.
- [Low] Missing 'reg' property in the 'required' list for an SPI peripheral.
--

commit 455b6048d6e6dd86252693c3c8edc0762c5dbba4
Author: Jingyuan Liang <jingyliang@chromium.org>

dt-bindings: input: Document hid-over-spi DT schema

Documentation describes the required and optional properties for
implementing Device Tree for a Microsoft G6 Touch Digitizer that
supports HID over SPI Protocol 1.0 specification.

> diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> new file mode 100644
> index 0000000000000..27cf311e0aab0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml

[ ... ]

> +  read-opcode:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      Value to be used in Read Approval packets. 1 byte.
> +
> +  write-opcode:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      Value to be used in Write Approval packets. 1 byte.

[Severity: High]
Will defining these as uint8 cause the driver probe to fail?

In the driver (drivers/hid/spi-hid/spi-hid-of.c), spi_hid_of_populate_config()
reads these using device_property_read_u32().

If a conforming device tree provides 1-byte properties,
device_property_read_u32() will return -EOVERFLOW because the property length
is less than 4 bytes, causing the probe to abort.

Should the driver be updated to use device_property_read_u8(), or should
these schema properties be uint32?

> +
> +required:
> +  - compatible
> +  - interrupts
> +  - reset-gpios
> +  - vdd-supply

[Severity: Low]
Should 'reg' be included in this required list?

Although spi-controller.yaml enforces 'reg' for child nodes matching the
@[0-9a-f]+$ pattern, a malformed node without a unit address would bypass
this check and silently pass schema validation against this binding.

> +  - input-report-header-address
> +  - input-report-body-address
> +  - output-report-address
> +  - read-opcode
> +  - write-opcode

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=9

Re: [PATCH v4 08/11] HID: spi_hid: add device tree support for SPI over HID

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Heap buffer overflow via attacker-controlled `report_length`.
- [Medium] API misuse: Calling `gpiod_set_value` instead of `gpiod_set_value_cansleep`.
- [Medium] Hardware backpowering risk due to improper GPIO initialization.
- [Low] Log spam during boot due to unhandled -EPROBE_DEFER.
--

commit 0c5bcceb42b14d25affd5a3652cde4d4a22e95c2
Author: Jarrett Schultz <jaschultz@microsoft.com>

HID: spi_hid: add device tree support for SPI over HID

This commit adds Open Firmware (Device Tree) transport support for HID
over SPI devices. It introduces the spi_hid_of module to configure and
manage power and reset lines for these devices based on device tree
properties.

> diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
> new file mode 100644
> index 0000000000000..ba7d5338f5d89
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-of.c
> @@ -0,0 +1,246 @@

[ ... ]

> +static int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> +				      struct device *dev)
> +{
> +	int error;
> +	u32 val;

[ ... ]

> +	conf->supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(conf->supply)) {
> +		if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get regulator: %ld.",
> +				PTR_ERR(conf->supply));
> +		return PTR_ERR(conf->supply);
> +	}
> +	conf->supply_enabled = false;
> +
> +	conf->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

[Severity: Medium]
Could initializing the reset GPIO with GPIOD_OUT_LOW cause hardware
backpowering?

If the GPIO is specified as active-low in the Device Tree, a logical 0
drives the physical pin high. Because this initialization happens during
probe before the device regulator is turned on in spi_hid_of_power_up(),
a high voltage might be driven into an unpowered peripheral. This could
forward-bias ESD diodes and backpower the device.

> +	if (IS_ERR(conf->reset_gpio)) {
> +		dev_err(dev, "%s: error getting reset GPIO.", __func__);

[Severity: Low]
Does this code unnecessarily log errors during normal boot sequencing?

If devm_gpiod_get() returns -EPROBE_DEFER, the error is unconditionally
logged by dev_err() before being returned.

> +		return PTR_ERR(conf->reset_gpio);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int spi_hid_of_assert_reset(struct spihid_ops *ops)
> +{
> +	struct spi_hid_of_config *conf = container_of(ops,
> +						      struct spi_hid_of_config,
> +						      ops);
> +
> +	gpiod_set_value(conf->reset_gpio, 1);

[Severity: Medium]
Should this use gpiod_set_value_cansleep() instead of gpiod_set_value()?

The assert_reset callback is invoked from sleepable contexts like probe
and workqueues. If the reset GPIO is attached to a sleepable controller
such as an I2C or SPI expander, calling the non-sleeping gpiod_set_value()
will trigger a WARN_ON in the GPIO core.

> +	return 0;
> +}
> +
> +static int spi_hid_of_deassert_reset(struct spihid_ops *ops)
> +{
> +	struct spi_hid_of_config *conf = container_of(ops,
> +						      struct spi_hid_of_config,
> +						      ops);
> +
> +	gpiod_set_value(conf->reset_gpio, 0);

[Severity: Medium]
Similarly, should this use gpiod_set_value_cansleep() since deassert_reset
can also be called from sleepable contexts?

> +	return 0;
> +}

[ ... ]

> +static int spi_hid_of_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct spi_hid_of_config *config;
> +	int error;

[ ... ]

> +	error = spi_hid_of_populate_config(config, dev);
> +	if (error) {
> +		dev_err(dev, "%s: unable to populate config data.", __func__);

[Severity: Low]
Does this code unnecessarily log a second error on probe deferral?

If spi_hid_of_populate_config() returns -EPROBE_DEFER, dev_err()
will unconditionally log it here as well.

> +		return error;
> +	}
> +
> +	return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
> +}

[ ... ]

In relation to the core code enabled by this transport driver:

[Severity: High]
Does the core input handling allow a heap buffer overflow via an
attacker-controlled report_length?

In drivers/hid/spi-hid/spi-hid-core.c:spi_hid_dev_irq(), the input report
header is read, and header.report_length is checked:

	if (shid->desc.max_input_length != 0 &&
	    header->report_length > shid->desc.max_input_length) {

The max_input_length is completely controlled by the SPI device and can
be set to 0 to bypass the check entirely, or set to a large value.

The driver then initiates a DMA read:

		error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
					   false);

Because the actual allocated buffer size (shid->bufsize) is determined
independently by the HID report descriptors, a malicious device could
send small report descriptors but huge report_length values, causing a
DMA read directly past the bounds of the shid->input->body heap buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=8

Re: [PATCH v4 04/11] HID: spi-hid: add spi-hid driver HID layer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing bounds check against allocated buffer size leads to a kernel heap buffer overflow.
- [High] Out-of-bounds write by 1 byte when copying the `HID_REQ_GET_REPORT` response in `spi_hid_ll_raw_request`.
- [High] Unsynchronized access to the shared `shid->response` buffer in `spi_hid_ll_raw_request`.
- [High] Timeout or interruption in `spi_hid_sync_request` leaves the device state and completion structure out-of-sync.
- [High] Device probe fails for standard HID devices with small reports because `spi_hid_ll_start` returns `-EINVAL`.
--

commit 475a21113f187310ac25983762798463f8e5115e
Author: Jingyuan Liang <jingyliang@chromium.org>

    HID: spi-hid: add spi-hid driver HID layer
    
    Add HID low level driver callbacks to register SPI as a HID driver, and
    an external touch device as a HID device.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 02a7608c4b882..72c2e1ce3e8d9 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c

[ ... ]

> @@ -84,11 +185,455 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state

[ ... ]

> +static int spi_hid_sync_request(struct spi_hid *shid,
> +				struct spi_hid_output_report *report)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int error;
> +
> +	guard(mutex)(&shid->output_lock);
> +
> +	reinit_completion(&shid->output_done);
> +
> +	error = __spi_hid_send_output_report(shid, report);
> +	if (error)
> +		return error;
> +
> +	error = wait_for_completion_interruptible_timeout(&shid->output_done,
> +							  msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
> +	if (error == 0) {
> +		dev_err(dev, "Response timed out\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
If this times out or is interrupted, the function returns and releases
shid->output_lock, but does the hardware stop processing the request?

If the hardware is still running and sends a late response, could it
prematurely complete shid->output_done for a subsequent request, permanently
desynchronizing the driver and the device?

[ ... ]

> +static int spi_hid_alloc_buffers(struct spi_hid *shid, size_t report_size)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int inbufsize = round_up(sizeof(shid->input->header) +
> +				 sizeof(shid->input->body) + report_size, 4);
> +	int outbufsize = round_up(sizeof(shid->output->header) + report_size, 4);
> +	void *tmp;
> +
> +	tmp = devm_krealloc(dev, shid->output, outbufsize, GFP_KERNEL | __GFP_ZERO);
> +	if (!tmp)
> +		return -ENOMEM;
> +	shid->output = tmp;
> +
> +	tmp = devm_krealloc(dev, shid->input, inbufsize, GFP_KERNEL | __GFP_ZERO);
> +	if (!tmp)
> +		return -ENOMEM;
> +	shid->input = tmp;
> +
> +	tmp = devm_krealloc(dev, shid->response, inbufsize, GFP_KERNEL | __GFP_ZERO);

[Severity: Critical]
When allocating these buffers based on report_size, is there a corresponding
bounds check against the allocated size when reading from the SPI device?

In spi_hid_dev_irq() and spi_hid_input_sync(), the driver appears to read
header.report_length bytes into shid->input->body. Furthermore, in
spi_hid_response_handler(), memcpy copies body.content_len bytes into
shid->response->content.

Since these lengths are provided by the device, could a malicious or
malfunctioning device send a larger length and overflow the allocated heap
buffers?

[ ... ]

> +static int spi_hid_ll_start(struct hid_device *hid)
> +{
> +	struct spi_device *spi = hid->driver_data;
> +	struct spi_hid *shid = spi_get_drvdata(spi);
> +	int error = 0;
> +	u16 bufsize = 0;
> +
> +	spi_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
> +	spi_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
> +	spi_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
> +
> +	if (bufsize < HID_MIN_BUFFER_SIZE) {
> +		dev_err(&spi->dev,
> +			"HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
> +			bufsize);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this check unconditionally reject standard HID devices (like mice or
keyboards) that legitimately have maximum report sizes smaller than
HID_MIN_BUFFER_SIZE (64 bytes)?

Should the driver enforce a minimum allocation size (for example, by padding
bufsize) instead of refusing to probe the device?

[ ... ]

> +static int spi_hid_ll_raw_request(struct hid_device *hid,
> +				  unsigned char reportnum, __u8 *buf,
> +				  size_t len, unsigned char rtype, int reqtype)
> +{
> +	struct spi_device *spi = hid->driver_data;
> +	struct spi_hid *shid = spi_get_drvdata(spi);
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	switch (reqtype) {

[ ... ]

> +	case HID_REQ_GET_REPORT:
> +		ret = spi_hid_get_request(shid, reportnum);

[Severity: High]
After spi_hid_get_request() returns, shid->output_lock is no longer held.
Could another thread concurrently call spi_hid_ll_raw_request() or
spi_hid_ll_output_report(), acquire the lock, and overwrite shid->response
before the current thread copies it?

> +		if (ret) {
> +			dev_err(dev, "failed to get report\n");
> +			return ret;
> +		}
> +
> +		ret = min_t(size_t, len,
> +			    (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
> +		buf[0] = shid->response->body[3];
> +		memcpy(&buf[1], &shid->response->content, ret);

[Severity: High]
Does this memcpy() cause an out-of-bounds write of 1 byte?

The code copies 'ret' bytes starting at '&buf[1]', which means it writes a
total of 'ret + 1' bytes into 'buf'. If 'ret' equals the allocated buffer
size 'len', it will write 'len + 1' bytes. Should the memcpy length
parameter be 'ret - 1'?

> +		break;
> +	default:
> +		dev_err(dev, "invalid request type\n");
> +		return -EIO;
> +	}
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=4

Re: [PATCH v4 06/11] HID: spi_hid: add spi_hid traces

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The `____cacheline_aligned` attribute was dropped from the DMA-safe transfer buffers `read_approval_header` and `read_approval_body` when the `spi_hid` struct was moved to a header file. This can lead to DMA cache coherency issues and memory corruption on architectures with non-coherent caches.
- [Medium] The `trace_spi_hid_input_sync` tracepoint logs the contents of the receive buffer (`rx_buf`) *before* the SPI transfer completes, leaking uninitialized heap memory or stale data from previous transfers.

Pre-existing issues:
- [Critical] A missing bounds check on `header.report_length` against the dynamically allocated buffer size allows a massive out-of-bounds DMA write, leading to a heap buffer overflow.
- [High] Concurrent reallocation of the shared `shid->input` buffer in `spi_hid_ll_parse` while the interrupt handler accesses it leads to a Use-After-Free.
- [High] Handling `HID_REQ_GET_REPORT` in `spi_hid_ll_raw_request` causes an out-of-bounds write to the caller's buffer by exactly 1 byte.
- [High] Concurrent overwriting of `shid->response` by the IRQ handler while a waiting thread is actively processing it leads to data corruption.
--

commit 2915829fcdae73aed3b7b4c76739fccf90310d7d
Author: Jingyuan Liang <jingyliang@chromium.org>

    HID: spi_hid: add spi_hid traces
    
    Add traces for purposed of debugging spi_hid driver.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index f6ea2d4365a70..ef527999d6dc6 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -295,6 +225,11 @@ static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
>  	spi_message_init_with_transfers(&shid->input_message,
>  					shid->input_transfer, 2);
>  
> +	trace_spi_hid_input_sync(shid,	shid->input_transfer[0].tx_buf,
> +				 shid->input_transfer[0].len,
> +				 shid->input_transfer[1].rx_buf,
> +				 shid->input_transfer[1].len, 0);
> +
>  	error = spi_sync(shid->spi, &shid->input_message);

[Severity: Medium]
Does this tracepoint log the contents of the receive buffer before the SPI
transfer actually completes? Since spi_sync() hasn't executed yet, it appears
this will blindly copy uninitialized heap memory or stale data from previous
transfers into the trace output.

[ ... ]
> @@ -528,6 +468,8 @@ static int spi_hid_input_report_handler(struct spi_hid *shid,
>  static void spi_hid_response_handler(struct spi_hid *shid,
>  				     struct input_report_body_header *body)
>  {
> +	trace_spi_hid_response_handler(shid);
> +
>  	shid->response_length = body->content_len;
>  	/* completion_done returns 0 if there are waiters, otherwise 1 */
>  	if (completion_done(&shid->output_done)) {

[Severity: High]
This is a pre-existing issue, but is the completion_done() check sufficient
to protect against data corruption?

When a waiting thread wakes up from wait_for_completion_interruptible_timeout()
in spi_hid_sync_request(), the completion's internal done count is decremented
to 0. A newly arriving interrupt will see completion_done() == 0, and will
overwrite shid->response while the awakened thread is concurrently reading it
outside of the output_lock.

[ ... ]
> @@ -867,6 +815,9 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
>  	struct spi_hid_input_header header;
>  	int error = 0;
>  
> +	trace_spi_hid_dev_irq(shid, irq);
> +	trace_spi_hid_header_transfer(shid);
> +
>  	scoped_guard(mutex, &shid->io_lock) {

[Severity: High]
This is a pre-existing issue, but does concurrent buffer reallocation in
spi_hid_ll_parse() lead to a use-after-free here?

spi_hid_ll_parse() calls spi_hid_alloc_buffers() (which uses devm_krealloc())
without disabling the IRQ or holding the io_lock. If devm_krealloc() frees
the old buffer while the IRQ handler is concurrently configuring DMA transfers
into shid->input, will the IRQ handler write into freed memory?

[ ... ]
> @@ -916,6 +874,12 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
>  			goto out;
>  		}
>  
> +		trace_spi_hid_input_body_complete(shid, shid->input_transfer[0].tx_buf,
> +						  shid->input_transfer[0].len,
> +						  shid->input_transfer[1].rx_buf,
> +						  shid->input_transfer[1].len,
> +						  shid->input_message.status);
> +
>  		if (shid->input_message.status < 0) {

[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check that could
lead to an out-of-bounds DMA write?

In spi_hid_bus_validate_header(), the validation only checks if
header.report_length > shid->desc.max_input_length. If a malicious device
advertises a max_input_length of 0, the check is bypassed entirely. The driver
then unconditionally performs a DMA read of header.report_length bytes into
shid->input->body, which could overflow the dynamically allocated bufsize.

[Severity: High]
This is a pre-existing issue elsewhere in this file, but does handling
HID_REQ_GET_REPORT in spi_hid_ll_raw_request() cause an out-of-bounds write?

In spi_hid_ll_raw_request():

    ret = min_t(size_t, len,
                (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
    buf[0] = shid->response->body[3];
    memcpy(&buf[1], &shid->response->content, ret);

The driver writes the 1-byte ID to buf[0] and then copies ret bytes into
&buf[1], meaning it writes a total of ret + 1 bytes. If ret equals the
caller's buffer size (len), doesn't this overflow the destination buffer by
exactly 1 byte?

> diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
> new file mode 100644
> index 0000000000000..293e2cfcfbf74
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.h
[ ... ]
> @@ -0,0 +1,91 @@
> +/* Driver context */
> +struct spi_hid {
[ ... ]
> +	struct completion output_done;
> +
> +	u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN];
> +	u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
> +
> +	u32 report_descriptor_crc32;	/* HID report descriptor crc32 checksum. */

[Severity: High]
When moving struct spi_hid to this new header, the ____cacheline_aligned
attribute was dropped from the read_approval_header and read_approval_body
buffers. 

Could this lead to DMA cache coherency issues and memory corruption on
architectures with non-coherent caches, since these arrays might now share
cachelines with adjacent fields?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=6

Re: [PATCH v4 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Embedded arrays `read_approval_header` and `read_approval_body` are used for SPI DMA transfers without cacheline alignment, risking memory corruption.
- [Medium] Unchecked return values of reset operations in `spi_hid_dev_init` and `spi_hid_resume`
--

commit ea35a3a23739acaca367a8d250ef25c96d22df36
Author: Angela Czubak <acz@semihalf.com>

HID: spi-hid: add transport driver skeleton for HID over SPI bus

Create spi-hid folder and add Kconfig and Makefile for spi-hid driver.
Add basic device structure, definitions, and probe/remove functions.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> new file mode 100644
> index 0000000000000..02a7608c4b882
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -0,0 +1,213 @@

[ ... ]

> +/* Driver context */
> +struct spi_hid {
> +	struct spi_device	*spi;	/* spi device. */
> +	struct hid_device	*hid;	/* pointer to corresponding HID dev. */
> +
> +	struct spihid_ops	*ops;
> +	struct spi_hid_conf	*conf;
> +
> +	enum hidspi_power_state power_state;
> +
> +	u32 regulator_error_count;
> +	int regulator_last_error;
> +	u32 bus_error_count;
> +	int bus_last_error;
> +	u32 dir_count;	/* device initiated reset count. */
> +};

[Severity: High]
Does embedding the read_approval_header and read_approval_body arrays directly
in this struct for SPI DMA transfers risk memory corruption?

When these buffers are passed as tx_buf to spi_sync() (e.g. in
spi_hid_input_sync()), they lack the ____cacheline_aligned directive. On
architectures with non-coherent DMA, cache maintenance operations could
encompass adjacent struct members.

Concurrent CPU writes to those adjacent members during the DMA mapping window
will be destroyed, leading to memory corruption.

Should these buffers be cacheline aligned, or dynamically allocated?

[ ... ]

> +int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
> +		       struct spi_hid_conf *conf)
> +{

[ ... ]

> +	/*
> +	 * At the end of probe we initialize the device:
> +	 *   0) assert reset, bias the interrupt line
> +	 *   1) sleep minimal reset delay
> +	 *   2) request IRQ
> +	 *   3) power up the device
> +	 *   4) deassert reset (high)
> +	 * After this we expect an IRQ with a reset response.
> +	 */
> +
> +	shid->ops->assert_reset(shid->ops);
> +
> +	shid->ops->sleep_minimal_reset_delay(shid->ops);

[ ... ]

> +	error = shid->ops->power_up(shid->ops);
> +	if (error) {
> +		dev_err(dev, "%s: could not power up\n", __func__);
> +		return error;
> +	}
> +
> +	shid->ops->deassert_reset(shid->ops);

[Severity: Medium]
Can hardware reset failures be masked here?

The assert_reset and deassert_reset callbacks return integer error codes.
However, their return values are ignored in spi_hid_core_probe() (as well as
resume paths).

If deassert_reset() fails, the device may remain in reset, causing the driver
to hang waiting for a reset response IRQ that will never arrive.

Should these return values be checked and handled?

> +
> +	dev_dbg(dev, "%s: d3 -> %s\n", __func__,
> +		spi_hid_power_mode_string(shid->power_state));
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=3

Re: [PATCH v4 11/11] HID: spi-hid: add panel follower support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Leak of wake IRQ structure on probe failure or driver unbind for panel followers.
- [High] Unbalanced regulator power_down during remove() for unbound panel followers.
- [High] Missing power_state update in spi_hid_dev_init() breaks device and leaks regulator after suspend.

Pre-existing issues:
- [High] Preexisting Use-After-Free due to race between cancel_work_sync() and hid_destroy_device() timeouts.
--

commit 6b1c4605757961c511d63ab9f2108f26f48fd916
Author: Jingyuan Liang <jingyliang@chromium.org>

HID: spi-hid: add panel follower support

Add support to spi-hid to be a panel follower.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 517f069134775..27f25d95ed280 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -1281,6 +1281,106 @@ const struct attribute_group *spi_hid_groups[] = {

[ ... ]

> +static int spi_hid_dev_init(struct spi_hid *shid)
> +{
> +	struct spi_device *spi = shid->spi;
> +	struct device *dev = &spi->dev;
> +	int error;
> +
> +	shid->ops->assert_reset(shid->ops);
> +
> +	shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> +	error = shid->ops->power_up(shid->ops);
> +	if (error) {
> +		dev_err(dev, "%s: could not power up\n", __func__);
> +		shid->regulator_error_count++;
> +		shid->regulator_last_error = error;
> +		return error;
> +	}
> +
> +	shid->ops->deassert_reset(shid->ops);
> +
> +	enable_irq(spi->irq);
> +
> +	return 0;
> +}

[Severity: High]
If a panel follower suspends before it successfully receives its hid_version,
spi_hid_suspend() will legitimately power down the device and set power_state =
HIDSPI_OFF.

Upon resume, spi_hid_panel_follower_work() retries initialization by calling
spi_hid_dev_init(). Since spi_hid_dev_init() does not update power_state to
HIDSPI_ON, does the state remain HIDSPI_OFF?

If so, when the reset response interrupt arrives, would spi_hid_dev_irq() drop
it because it believes the device is off, leaving the device broken?
Furthermore, would any subsequent suspend return early because power_state ==
HIDSPI_OFF, failing to power down the regulator and causing a power leak?

[ ... ]

> +static int spi_hid_register_panel_follower(struct spi_hid *shid)
> +{
> +	struct device *dev = &shid->spi->dev;
> +
> +	shid->panel_follower.funcs = &spi_hid_panel_follower_prepare_funcs;
> +
> +	/*
> +	 * If we're not in control of our own power up/power down then we can't
> +	 * do the logic to manage wakeups. Give a warning if a user thought
> +	 * that was possible then force the capability off.
> +	 */
> +	if (device_can_wakeup(dev)) {
> +		dev_warn(dev, "Can't wakeup if following panel\n");
> +		device_set_wakeup_capable(dev, false);
> +	}

[Severity: High]
Does explicitly executing device_set_wakeup_capable(dev, false) mutate the
dev->power.can_wakeup flag to false, which would then break the symmetry of the
wake IRQ cleanup conditions?

In spi_hid_core_probe(), the wake IRQ is successfully registered if
device_may_wakeup(dev) is true. During error cleanup (err_wake_irq) or driver
removal (spi_hid_core_remove()), the code conditionally calls
dev_pm_clear_wake_irq(dev) based on if (device_may_wakeup(dev)).

Since the capability was previously cleared here, will this check evaluate to
false and cause the driver to skip the cleanup, leaking the wake IRQ
registration?

[ ... ]

> @@ -1300,6 +1400,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
>  	shid->ops = ops;
>  	shid->conf = conf;
>  	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +	shid->is_panel_follower = drm_is_panel_follower(&spi->dev);

[Severity: High]
During spi_hid_core_probe(), shid->power_state is unconditionally initialized
to HIDSPI_ON. For a panel follower, the actual hardware initialization and
regulator power_up() are deferred to spi_hid_panel_follower_work().

If the driver is unbound (or probe fails gracefully after device registration)
before the panel prepares, power_up() is never called. However,
spi_hid_core_remove() checks if (shid->power_state != HIDSPI_OFF) and calls
power_down().

Will this trigger a power_down() on an already disabled regulator, resulting in
an unbalanced regulator use-count?

[ ... ]

> @@ -1376,15 +1470,21 @@ void spi_hid_core_remove(struct spi_device *spi)
>  	struct device *dev = &spi->dev;
>  	int error;
>  
> -	disable_irq(spi->irq);
> +	if (shid->is_panel_follower)
> +		drm_panel_remove_follower(&shid->panel_follower);
> +	else
> +		disable_irq(spi->irq);
> +
>  	cancel_work_sync(&shid->reset_work);
>  
>  	spi_hid_stop_hid(shid);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk due to a race
between cancel_work_sync() and hid_destroy_device() timeouts?

In spi_hid_core_remove(), cancel_work_sync(&shid->reset_work) is called prior
to spi_hid_stop_hid(shid), which destroys the HID device. Because
disable_irq(spi->irq) is called beforehand, any concurrent HID request (like a
userspace GET_REPORT ioctl) waiting in
wait_for_completion_interruptible_timeout() will inevitably time out.

Upon timing out, the sync request handler sets the error flag and executes
schedule_work(&shid->reset_work). Since this timeout can occur after
cancel_work_sync() has already finished but before remove() concludes, the
work is freshly queued. The remove() function then frees shid, resulting in
the reset_work executing on freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=11

Re: [PATCH v4 05/11] HID: spi-hid: add HID SPI protocol implementation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:

New issues:
- [Critical] Missing validation of `report_length` against the allocated DMA buffer size allows an out-of-bounds write of up to ~64KB during SPI input read.
- [Critical] Use-After-Free of `shid->input` DMA buffers via concurrent reallocation and IRQ handling.
- [Critical] Out-of-bounds write in `spi_hid_response_handler` using device-supplied length.
- [High] Incorrect sizing of arrays in `struct spi_hid_input_buf` drops the Report ID and causes an out-of-bounds read when reporting to the HID core.
- [High] Returning `IRQ_HANDLED` when the device is off without servicing the interrupt causes an interrupt storm.
- [High] Missing power state update in `spi_hid_error_handler` abort path leads to an infinite interrupt storm.
- [High] Use-After-Free (UAF) on driver unbind due to missing synchronization with active HID raw requests.
- [High] Holding `io_lock` over `hid_input_report` creates an ABBA lock inversion and potential self-deadlock with `output_lock`.
- [High] Missing validation of response type in `spi_hid_response_handler` leaks stale data to userspace.

Pre-existing issues:
- [High] Handling of `HID_REQ_GET_REPORT` in `spi_hid_ll_raw_request` copies too many bytes into the output buffer, overflowing the caller's allocation by 1 byte and reading out of bounds.
- [Medium] Concurrent calls to `HID_REQ_GET_REPORT` cause a data race on the shared `shid->response` buffer, corrupting data returned to userspace.
--

commit 49a8cad2bdbcc64aa9d451335312691879579a95
Author: Jingyuan Liang <jingyliang@chromium.org>

HID: spi-hid: add HID SPI protocol implementation

This driver follows HID Over SPI Protocol Specification 1.0 available at
https://www.microsoft.com/en-us/download/details.aspx?id=103325. The
initial version of the driver does not support: 1) multi-fragment input
reports, 2) sending GET_INPUT and COMMAND output report types and
processing their respective acknowledge input reports, and 3) device
sleep power state.

diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 72c2e1ce3e8d9..f6ea2d4365a70 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
@@ -187,15 +336,64 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
> +static void spi_hid_error_handler(struct spi_hid *shid)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int error;
> +
> +	guard(mutex)(&shid->power_lock);
> +	if (shid->power_state == HIDSPI_OFF)
> +		return;
> +
> +	guard(disable_irq)(&shid->spi->irq);
> +
> +	if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> +		dev_err(dev, "unresponsive device, aborting\n");
> +		spi_hid_stop_hid(shid);
> +		shid->ops->assert_reset(shid->ops);
> +		error = shid->ops->power_down(shid->ops);
> +		if (error) {
> +			dev_err(dev, "failed to disable regulator\n");
> +			shid->regulator_error_count++;
> +			shid->regulator_last_error = error;
> +		}
> +		return;
> +	}

[Severity: High]
Does this error path need to update the power state?

When reset_attempts exceeds the maximum, this function powers down the
device and returns immediately without setting shid->power_state to
HIDSPI_OFF.

Because this function uses guard(disable_irq), returning automatically
re-enables the interrupt. Since the device is unpowered, it will assert
its IRQ line and trigger the handler again. Since power_state is still
HIDSPI_ON, can this cause an infinite interrupt storm?

[ ... ]
@@ -263,6 +462,88 @@ static int spi_hid_sync_request(struct spi_hid *shid,
> +static int spi_hid_input_report_handler(struct spi_hid *shid,
> +					struct spi_hid_input_buf *buf)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	struct spi_hid_input_report r;
> +	int error = 0;
> +
> +	guard(mutex)(&shid->io_lock);

[Severity: High]
Can this cause an ABBA lock inversion with output_lock?

This function holds io_lock before calling into the generic HID subsystem.
If a BPF program attached to the HID core synchronously triggers
hid_hw_raw_request during this call, it routes back to the driver
via spi_hid_ll_raw_request(), which calls spi_hid_sync_request()
(acquiring output_lock) and then __spi_hid_send_output_report(), which
attempts to acquire io_lock. Could this self-deadlock the thread?

> +
> +	if (!test_bit(SPI_HID_READY, &shid->flags) ||
> +	    test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
> +		dev_err(dev, "HID not ready\n");
> +		return 0;
> +	}
> +
> +	spi_hid_input_report_prepare(buf, &r);
> +
> +	error = hid_input_report(shid->hid, HID_INPUT_REPORT,
> +				 r.content - 1, r.content_length + 1, 1);

[Severity: High]
Does passing r.content - 1 drop the Report ID?

Looking at struct spi_hid_input_buf, the body array is sized to
SPI_HID_READ_APPROVAL_LEN (which is 5 bytes). However, the actual protocol
body header is only 4 bytes. This means the first byte of the content lands
in buf->body[4], while the Report ID is in buf->body[3].

Since this passes r.content - 1 (which points to buf->body[4]) to
hid_input_report, it completely skips the Report ID. Because it also passes
r.content_length + 1 as the size, does this read one byte past the end of
the received SPI transfer into uninitialized heap memory?

[ ... ]
> +static void spi_hid_response_handler(struct spi_hid *shid,
> +				     struct input_report_body_header *body)
> +{
> +	shid->response_length = body->content_len;
> +	/* completion_done returns 0 if there are waiters, otherwise 1 */
> +	if (completion_done(&shid->output_done)) {
> +		dev_err(&shid->spi->dev, "Unexpected response report\n");
> +	} else {
> +		if (body->input_report_type == REPORT_DESCRIPTOR_RESPONSE ||
> +		    body->input_report_type == GET_FEATURE_RESPONSE) {
> +			memcpy(shid->response->body, shid->input->body,
> +			       sizeof(shid->input->body));
> +			memcpy(shid->response->content, shid->input->content,
> +			       body->content_len);

[Severity: Critical]
Can this memcpy overflow the response buffer?

The memcpy uses body->content_len to copy data into
shid->response->content. While body->content_len is validated against
header.report_length elsewhere, that header length is only bounded by
shid->desc.max_input_length (up to 65535).

Since shid->response is only allocated to shid->bufsize (typically 2048),
can a device sending a large content length cause a heap buffer overflow?

> +		}
> +		complete(&shid->output_done);

[Severity: High]
Is it safe to unconditionally trigger completion here?

If an unexpected response (such as OUTPUT_REPORT_RESPONSE) arrives,
this completes shid->output_done without updating shid->response->content.
The waiting thread will then wake up and return success. Could this leak
stale kernel heap memory to userspace?

> +	}
> +}
[ ... ]
> +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> +				       struct spi_hid_input_header *header)
> +{
> +	struct device *dev = &shid->spi->dev;
> +
> +	if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> +		dev_err(dev, "Unknown input report version (v 0x%x)\n",
> +			header->version);
> +		return -EINVAL;
> +	}
> +
> +	if (shid->desc.max_input_length != 0 &&
> +	    header->report_length > shid->desc.max_input_length) {
> +		dev_err(dev, "Input report body size %u > max expected of %u\n",
> +			header->report_length, shid->desc.max_input_length);
> +		return -EMSGSIZE;
> +	}

[Severity: Critical]
Does this header validation also need to check the allocated buffer size?

This validates header->report_length against shid->desc.max_input_length,
which can be up to 65535, but never validates it against the actually
allocated buffer size (shid->bufsize).

Later in spi_hid_dev_irq(), spi_hid_input_sync() will use this unvalidated
length for a DMA transfer directly into shid->input->body. Could a
malfunctioning device cause a massive kernel heap buffer overflow?

[ ... ]
@@ -371,9 +860,83 @@ static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
> +static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> +{
> +	struct spi_hid *shid = _shid;
> +	struct device *dev = &shid->spi->dev;
> +	struct spi_hid_input_header header;
> +	int error = 0;
> +
> +	scoped_guard(mutex, &shid->io_lock) {
> +		error = spi_hid_input_sync(shid, shid->input->header,
> +					   sizeof(shid->input->header), true);
> +		if (error) {
> +			dev_err(dev, "Failed to transfer header: %d\n", error);
> +			goto err;
> +		}
> +
> +		if (shid->power_state == HIDSPI_OFF) {
> +			dev_warn(dev, "Device is off after header was received\n");
> +			goto out;
> +		}

[Severity: High]
Can returning IRQ_HANDLED here cause an interrupt storm?

If the device is unpowered, it may float its IRQ line, which is often
level-triggered. Returning IRQ_HANDLED without communicating with the
device to clear the interrupt prevents the spurious IRQ detector from
disabling it. Will this lead to an infinite re-triggering of the interrupt
and a hard lockup?

[ ... ]
> +		if (shid->input_message.status < 0) {
> +			dev_warn(dev, "Error reading body: %d\n",
> +				 shid->input_message.status);
> +			shid->bus_error_count++;
> +			shid->bus_last_error = shid->input_message.status;
> +			goto err;
> +		}
> +	}
> +
> +	error = spi_hid_process_input_report(shid, shid->input);

[Severity: Critical]
Is it safe to dereference shid->input outside of the io_lock?

This passes shid->input to spi_hid_process_input_report() entirely outside
of the io_lock scoped guard.

If a concurrent call to spi_hid_alloc_buffers() happens:

    tmp = devm_krealloc(dev, shid->input, inbufsize,
                        GFP_KERNEL | __GFP_ZERO);

This reallocates and frees the buffer while the device IRQ is fully enabled.
Could this lead to a use-after-free if an interrupt fires during or
immediately after the reallocation?

> +	if (error) {
> +		dev_err(dev, "Failed to process input report: %d\n", error);
> +		goto err;
> +	}
> +
> +out:
> +	return IRQ_HANDLED;

[ ... ]
@@ -762,6 +1335,9 @@ void spi_hid_core_remove(struct spi_device *spi)
>  	struct device *dev = &spi->dev;
>  	int error;
>  
> +	disable_irq(spi->irq);
> +	cancel_work_sync(&shid->reset_work);
> +
>  	spi_hid_stop_hid(shid);
>  
>  	shid->ops->assert_reset(shid->ops);

[Severity: High]
Does driver unbind safely synchronize with active HID raw requests?

This remove function disables the IRQ and destroys the HID device, returning
to allow devres to free the shid struct. However, hid_destroy_device() does
not serialize against active userspace ioctls in hidraw.

If a thread is blocked inside spi_hid_sync_request() holding
shid->output_lock and waiting on shid->output_done:

    error = wait_for_completion_interruptible_timeout(&shid->output_done, ...);

Can it wake up after the timeout and access the freed shid memory, causing
a use-after-free?


[Severity: High]
This is a pre-existing issue, but does the handling of HID_REQ_GET_REPORT
overflow the caller's allocation?

Looking at spi_hid_ll_raw_request() from drivers/hid/spi-hid/spi-hid-core.c:

    case HID_REQ_GET_REPORT:
        ret = spi_hid_get_request(shid, reportnum);
        ...
        ret = min_t(size_t, len,
                    (shid->response->body[1] |
                     (shid->response->body[2] << 8)) + 1);
        buf[0] = shid->response->body[3];
        memcpy(&buf[1], &shid->response->content, ret);
        break;

If ret == len, the memcpy writes len bytes starting at &buf[1], consuming
len + 1 bytes in total and overflowing the caller's buf by 1 byte.

It also reads ret bytes from shid->response->content, which only contains
ret - 1 valid payload bytes, causing a 1-byte out-of-bounds read.


[Severity: Medium]
This is a pre-existing issue, but does reading the shared response buffer
require synchronization?

In that same HID_REQ_GET_REPORT path in spi_hid_ll_raw_request(), the code
accesses shid->response->body and shid->response->content without holding
any locks after spi_hid_get_request() drops output_lock.

If multiple threads concurrently issue ioctls, could a second thread
overwrite shid->response via the IRQ handler while the first thread is
executing its memcpy, corrupting data returned to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=5

Re: [PATCH v4 10/11] HID: spi-hid: add power management implementation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] `spi_hid_resume()` calls `hid_driver_reset_resume()` synchronously before the device signals readiness.
- [High] Error path in `spi_hid_resume()` creates unbalanced IRQ disable depth for wakeup-capable devices.
- [High] Missing synchronization between system suspend and `reset_work` causes a TOCTOU race on `power_state`.
- [High] System suspend cuts device power without synchronizing against ongoing SPI HID requests.
- [High] Unbalanced `enable_irq()` in `spi_hid_resume()` triggers a kernel warning if the device was already `HIDSPI_OFF`.
- [Medium] Conditional cleanup of the wake IRQ leads to a memory leak if userspace disables wakeup.

Pre-existing issues:
- [Critical] Returning `IRQ_HANDLED` for an unpowered device causes an infinite IRQ storm.
--

commit 57585bfd26214992cdd5246d2df2a4622693a915
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add power management implementation

This commit implements HID over SPI driver power management callbacks, adding
suspend and resume functions for system power state transitions.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 698e72102c11e..517f069134775 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c

[ ... ]

> @@ -245,6 +247,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
>  	}
>  }
>  
> +static int spi_hid_suspend(struct spi_hid *shid)
> +{
> +	int error;
> +	struct device *dev = &shid->spi->dev;
> +
> +	guard(mutex)(&shid->power_lock);

[Severity: High]
Does this suspend routine need to synchronize against ongoing SPI I/O?

HID I/O routines like spi_hid_ll_output_report() and spi_hid_ll_raw_request()
acquire output_lock and io_lock to serialize transfers, but they do not check
or lock power_state. Because spi_hid_suspend() acquires power_lock to disable
the device's regulator but does not acquire output_lock or io_lock, nor does
it clear SPI_HID_READY, could it cut hardware power concurrently while a kernel
thread is actively executing spi_write() on another CPU, leading to hardware
bus timeouts?

[Severity: High]
Does this function also need to flush or cancel the asynchronous reset_work
before cutting device power?

If it doesn't synchronize with the workqueue, could there be a race where
spi_hid_reset_response() locklessly reads power_state as HIDSPI_ON, gets
preempted, and allows spi_hid_suspend() to power down the device? If the
workqueue then resumes and calls spi_hid_sync_request(), it would initiate
SPI transmissions to an unpowered device.

> +	if (shid->power_state == HIDSPI_OFF)
> +		return 0;

[Severity: High]
If this early return is taken, will the subsequent resume cause an unbalanced
IRQ enable warning?

In spi_hid_suspend(), this early return completely bypasses the call to
disable_irq(shid->spi->irq). However, upon system resume, spi_hid_resume()
unconditionally calls enable_irq(shid->spi->irq). If the IRQ disable depth
was not incremented during suspend, could this unconditional enable drop the
depth below zero and trigger a WARN(1, "Unbalanced enable for IRQ %d\n")?

> +
> +	if (shid->hid) {
> +		error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
> +		if (error) {
> +			dev_err(dev, "%s failed to suspend hid driver: %d\n",
> +				__func__, error);
> +			return error;
> +		}
> +	}
> +
> +	disable_irq(shid->spi->irq);
> +
> +	if (!device_may_wakeup(dev)) {
> +		set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> +		shid->ops->assert_reset(shid->ops);
> +
> +		error = shid->ops->power_down(shid->ops);
> +		if (error) {
> +			dev_err(dev, "%s: could not power down\n", __func__);
> +			shid->regulator_error_count++;
> +			shid->regulator_last_error = error;
> +			/* Undo partial suspend before returning error */
> +			shid->ops->deassert_reset(shid->ops);
> +			clear_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +			enable_irq(shid->spi->irq);
> +			if (shid->hid)
> +				hid_driver_reset_resume(shid->hid);
> +			return error;
> +		}
> +
> +		shid->power_state = HIDSPI_OFF;
> +	}
> +	return 0;
> +}
> +
> +static int spi_hid_resume(struct spi_hid *shid)
> +{
> +	int error;
> +	struct device *dev = &shid->spi->dev;
> +
> +	guard(mutex)(&shid->power_lock);
> +
> +	if (!device_may_wakeup(dev)) {
> +		if (shid->power_state == HIDSPI_OFF) {
> +			shid->ops->assert_reset(shid->ops);
> +
> +			shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> +			error = shid->ops->power_up(shid->ops);
> +			if (error) {
> +				dev_err(dev, "%s: could not power up\n", __func__);
> +				shid->regulator_error_count++;
> +				shid->regulator_last_error = error;
> +				return error;
> +			}
> +			shid->power_state = HIDSPI_ON;
> +			shid->ops->deassert_reset(shid->ops);
> +		}
> +	}
> +
> +	enable_irq(shid->spi->irq);
> +
> +	if (shid->hid) {
> +		error = hid_driver_reset_resume(shid->hid);

[Severity: High]
Does this code need to wait for the device's asynchronous RESET_RESPONSE
before sending further commands?

During a full resume cycle, the device is powered up, its reset line is
deasserted, and enable_irq() is called. The hardware then begins booting
and will signal readiness via an asynchronous RESET_RESPONSE interrupt.
However, spi_hid_resume() does not wait for this interrupt and synchronously
executes hid_driver_reset_resume() immediately. Because the SPI_HID_READY
flag is left set during suspend, any HID initialization commands generated
by the upper layer's resume callback will be immediately transmitted over
the SPI bus to a device that is still booting. Could this result in bus
timeouts and failed resume cycles?

> +		if (error) {
> +			dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
> +				__func__, error);
> +			/* Undo partial resume before returning error */
> +			disable_irq(shid->spi->irq);
> +			if (!device_may_wakeup(dev)) {
> +				set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +				shid->ops->assert_reset(shid->ops);
> +				shid->ops->power_down(shid->ops);
> +				shid->power_state = HIDSPI_OFF;
> +			}

[Severity: High]
Can this error path create an unbalanced IRQ disable depth for
wakeup-capable devices?

If a device is wakeup-capable, spi_hid_suspend() skips powering down the
device (leaving power_state at HIDSPI_ON) but still calls disable_irq().
In spi_hid_resume(), enable_irq() is called, bringing the IRQ depth to 0.

If hid_driver_reset_resume() subsequently fails, this error recovery path
calls disable_irq() to undo the resume, but skips setting HIDSPI_OFF due
to the if (!device_may_wakeup(dev)) condition. Because the PM core ignores
resume errors and continues, the device remains at HIDSPI_ON.

On the next system suspend cycle, spi_hid_suspend() will see HIDSPI_ON and
execute disable_irq() a second time. A later successful resume will only call
enable_irq() once, leaving the device's interrupt permanently disabled.

> +			return error;
> +		}
> +	}
> +	return 0;
> +}

[ ... ]

> @@ -795,6 +887,11 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
>  	trace_spi_hid_header_transfer(shid);
>  
>  	scoped_guard(mutex, &shid->io_lock) {
> +		if (shid->power_state == HIDSPI_OFF) {
> +			dev_warn(dev, "Device is off, ignoring interrupt\n");
> +			goto out;
> +		}

[Severity: Critical]
This isn't a bug introduced by this patch, but should this path return IRQ_NONE
instead of IRQ_HANDLED?

In spi_hid_error_handler(), if the device exceeds maximum reset attempts,
it is aborted and powered off (HIDSPI_OFF). The function then returns,
releasing guard(disable_irq)(&shid->spi->irq) and explicitly re-enabling
the IRQ line.

If the active-low interrupt line floats low due to power loss, it immediately
triggers. This threaded handler detects HIDSPI_OFF, logs a warning, and
returns IRQ_HANDLED via goto out.

Returning IRQ_HANDLED circumvents the kernel's spurious interrupt detector.
Because the physical line remains asserted and the handler claims success
without clearing the hardware source, the genirq subsystem immediately
re-fires the interrupt. Can this lead to a 100% CPU lockup?

[ ... ]

> @@ -1251,10 +1343,19 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
>  		dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
>  		return error;
>  	}
> +	if (device_may_wakeup(dev)) {
> +		error = dev_pm_set_wake_irq(dev, spi->irq);
> +		if (error) {
> +			dev_err(dev, "%s: failed to set wake IRQ\n", __func__);
> +			return error;
> +		}
> +	}
>  
>  	error = shid->ops->power_up(shid->ops);
>  	if (error) {
>  		dev_err(dev, "%s: could not power up\n", __func__);
> +		if (device_may_wakeup(dev))
> +			dev_pm_clear_wake_irq(dev);

[Severity: Medium]
Will this conditional cleanup leak memory if userspace alters the wakeup
settings?

In spi_hid_core_probe(), dev_pm_set_wake_irq() is executed if
device_may_wakeup(dev) is true. The cleanup path conditionally calls
dev_pm_clear_wake_irq(dev) based on the same check.

However, device_may_wakeup() evaluates dev->power.should_wakeup, which is
mutable state controllable by userspace via sysfs
(/sys/devices/.../power/wakeup). If userspace disables wakeup after the
device is probed, the check fails, bypassing the cleanup and permanently
leaking the allocated wake_irq memory while leaving a dangling pointer in
dev->power.wakeirq. Should dev_pm_clear_wake_irq() be called unconditionally
since it handles missing wake IRQs gracefully?

[ ... ]

> @@ -1284,9 +1385,31 @@ void spi_hid_core_remove(struct spi_device *spi)
>  	error = shid->ops->power_down(shid->ops);
>  	if (error)
>  		dev_err(dev, "failed to disable regulator\n");
> +
> +	if (device_may_wakeup(dev))
> +		dev_pm_clear_wake_irq(dev);

[Severity: Medium]
As noted above, should dev_pm_clear_wake_irq() be called unconditionally here
to avoid leaking the wake_irq structure if userspace disables device wakeup?

>  }
>  EXPORT_SYMBOL_GPL(spi_hid_core_remove);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=10

Re: [PATCH v4 07/11] HID: spi_hid: add ACPI support for SPI over HID

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi_hid_acpi_deassert_reset() silently fails to execute the ACPI _RST method, breaking device initialization.
- [High] Use-After-Free due to flawed cleanup ordering: the reset workqueue is cancelled before the HID device is destroyed.
- [High] The shared buffer `shid->response` is read without holding `output_lock`, leading to data corruption if concurrent HID requests occur.
--

commit 8f475e9d4e137ed8f5267ef4406acd64e1baca61
Author: Angela Czubak <acz@semihalf.com>
Subject: HID: spi_hid: add ACPI support for SPI over HID

This commit detects and adds ACPI support for SPI HID devices.

> diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
> new file mode 100644
> index 0000000000000..298e3ba44d8a0
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-acpi.c
> @@ -0,0 +1,253 @@
[ ... ]
> +static int spi_hid_acpi_deassert_reset(struct spihid_ops *ops)
> +{
> +	struct spi_hid_acpi_config *conf = container_of(ops,
> +							struct spi_hid_acpi_config,
> +							ops);
> +
> +	return device_reset(&conf->adev->dev);

[Severity: High]
Does device_reset() work correctly here?

Since conf->adev->dev is the internal ACPI device structure without its fwnode
set, ACPI_HANDLE() will return null, causing __device_reset() to skip the ACPI
reset.

Also, looking at spi_hid_dev_init(), the return value of deassert_reset()
is ignored:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_dev_init() {
    ...
	shid->ops->deassert_reset(shid->ops);
    ...
}

Should it evaluate _RST directly or use &spi->dev instead, and should the
return value be checked to prevent hardware reset from silently failing?

> +}

[Severity: High]
Can the cleanup ordering lead to a use-after-free regression during removal?

In spi_hid_core_remove(), cancel_work_sync() is called before
spi_hid_stop_hid():

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_core_remove() {
    ...
	cancel_work_sync(&shid->reset_work);

	spi_hid_stop_hid(shid);
    ...
}

Since spi_hid_stop_hid() calls hid_destroy_device(), the HID device is still
accessible to userspace between the work cancellation and device destruction.

If a userspace process concurrently issues an ioctl during this window,
spi_hid_sync_request() will timeout, and the error handler in
spi_hid_get_request() could incorrectly schedule reset_work. Once removal
finishes and devm frees the shid structure, could the mistakenly scheduled
work execute and dereference the freed shid pointer?

Should spi_hid_stop_hid() be called before cancel_work_sync() to prevent this?

[Severity: High]
Can the shared buffer shid->response be read while corrupted if concurrent
HID requests occur?

In spi_hid_sync_request(), output_lock is acquired via scoped guard and is
implicitly released upon function return:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_sync_request() {
    ...
	guard(mutex)(&shid->output_lock);
    ...
}

Functions like spi_hid_ll_raw_request() call into spi_hid_sync_request()
and then read from shid->response after the lock has been released:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_ll_raw_request() {
    ...
	ret = spi_hid_sync_request(shid, ...);
    ...
	ret = min_t(size_t, len,
		    (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
	buf[0] = shid->response->body[3];
	memcpy(&buf[1], &shid->response->content, ret);
    ...
}

If multiple threads concurrently issue ioctls, could the second thread acquire
output_lock and overwrite shid->response with its own data before the first
thread has finished reading it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=7