[PATCH v14 0/2] Firmware Support for USB-HID Devices and CP2112

[PATCH v14 0/2] Firmware Support for USB-HID Devices and CP2112

[Danny Kaehn]

This patchset allows USB-HID devices to have Firmware bindings through sharing
the USB fwnode with the HID driver, and adds such a binding and driver
implementation for the CP2112 USB to SMBus Bridge (which necessitated the
USB-HID change). This change allows a CP2112 permanently attached in hardware to
be described in DT and ACPI and interoperate with other drivers.

Changes in v14:
- Drop DT binding patch (merged upstream) (remove DT mailing lists and
  maintainers from email chain)
- Refactor logic in cp2112_probe() to reduce extra duplicate
  dev_fwnode() calls by retrieving and storing the CP2112's fwnode once
- Update comment on cp2112_child_acpi_cell_addrs enum to explain why
  values are explicitly defined
- Edit commit message for 1/2 per Andy's comments (lowercase letters in
  description; _ADR "equal to" Zero, etc..
- Edit commit message for 2/2 to use lowercase letters
- Link to v13: https://lore.kernel.org/r/20260127-cp2112-dt-v13-0-6448ddd4bf22@plexus.com

Changes in v13:
- dt-binding changes:
  - drop scl-gpios and sda-gpios from binding, since they are included
      from the i2c-controller schena.
  - Set indentation to 4 spaces consistently for the DTS example
  - Fix alignment for gpio-line-names in the example
- Use dev_fwnode in hid-cp2112.c instead of directly accessing fwnode
- Immediately release the fwnode_handle from
    device_get_named_child_node() in hid-cp2112.c
- Link to v12: https://lore.kernel.org/r/20251126-cp2112-dt-v12-0-2cdba6481db3@plexus.com

Changes in v12:
- dt-binding changes:
  - Drop "on the host controller" from top-level description based on
      comment from Rob H.
  - Correct "Properties must precede subnodes" dt_binding_check error by
      moving gpio_chip-related properties above the i2c subnode in the
      binding and in the example.
  - Include `interrupt-controller` property in the example
- Modify hid-cp2112.c to support separate schemas for DT vs. ACPI - DT
  combines gpio subnode with the CP2112's node, but will have an I2C
  subnode; while ACPI will maintain separate child nodes for the GPIO
  I2C devices

Changes in v11:
- Eliminate 'gpio' subnode for DT and ACPI for the CP2112 per comment
    from Rob H.
- Edit hid-cp2112.c to match for ACPI index and fall back to matching by
    name (instead of the other way around)
- Separate CP2112 I2C bus speed configuration into a separate patch

Changes in v10:
- Define an enumeration and mapping for CP2112 ACPI _ADRs and devicetree
    child node names, and use these in the scanning of child nodes
- Address other miscellaneous

Changes in v9:
- Add _ADR-based ACPI binding of child nodes (I2C is _ADR Zero, GPIO is _ADR One)
- Use a loop-based approach for assigning child nodes within probe().
    As a consequence, hid-cp2112.c no longer maintains references to the
    child fwnodes during the lifetime of the device. (plese correct if this
    is actually needed for this use-case)

Changes in v8:
- Apply Review tags retroactively to patches previously reviewed

Changes in v7:
- Use dev_fwnode when calling fwnod_handle_put in i2c_adapter in hid-cp2112.c
- Capitalize I2C and GPIO in commit message for patch 0003

Changes in v6:
- Fix fwnode_handle reference leaks in hid-cp21112.c
- Simplify hog node pattern in silabs,cp2112.yaml

Changes in v5:
 - Use fwnode API instead of of_node api in hid-core.c and hid-cp2112.c
 - Include sda-gpios and scl-gpios in silabs,cp2112.yaml
 - Additional fixups to silabs,cp2112.yaml to address comments
   - Remove ngpios property
   - Constrain the hog pattern to a single naming scheme
   - Remove unneeded properties from the gpio hog which are provided by
       the parent schema
 - Submit threaded interrupt bugfix separately from this patchset, as requested

Changes in v4:
 - Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/i2c

Changes in v3:
 - Additional fixups to silabs,cp2112.yaml to address comments

Changes in v2:
 - Added more detail to silabs,cp2112.yaml dt-binding
 - Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/input
 - Added support for setting smbus clock-frequency from DT in hid-cp2112.c
 - Added freeing of of_nodes on error paths of _probe in hid-cp2112.c

Danny Kaehn (3):
  dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
  HID: usbhid: Share USB device firmware node with child HID device
  HID: cp2112: Fwnode Support

 .../bindings/i2c/silabs,cp2112.yaml           | 113 ++++++++++++++++++
 drivers/hid/hid-cp2112.c                      |  50 ++++++++
 drivers/hid/usbhid/hid-core.c                 |   2 +
 3 files changed, 165 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

--
2.25.1

---
Danny Kaehn (2):
      HID: cp2112: Add fwnode support
      HID: cp2112: Configure I2C bus speed from firmware

 drivers/hid/hid-cp2112.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
---
base-commit: 70eda68668d1476b459b64e69b8f36659fa9dfa8
change-id: 20240605-cp2112-dt-7cdc95448e8a

Best regards,
-- 
Danny Kaehn <danny.kaehn@plexus.com>

[PATCH v14 1/2] HID: cp2112: Add fwnode support

[Danny Kaehn]

Support describing the CP2112's I2C and GPIO interfaces in firmware.

Bindings between the firmware nodes and the functions of the device
are distinct between ACPI and DeviceTree.

For ACPI, the i2c_adapter will use the child with _ADR equal to Zero
and the gpio_chip will use the child with _ADR equal to One.

For DeviceTree, the i2c_adapter will use the child with name "i2c",
but the gpio_chip will share a firmware node with the CP2112.

Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---
 drivers/hid/hid-cp2112.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 803b883ae875..e960fc988058 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -29,6 +29,18 @@
 #include <linux/usb/ch9.h>
 #include "hid-ids.h"
 
+/**
+ * enum cp2112_child_acpi_cell_addrs - Child ACPI addresses for CP2112 sub-functions
+ * Note that the enum values are explicitly defined, as this defines the interface
+ * between ACPI and Linux
+ * @CP2112_I2C_ADR: Address for I2C node
+ * @CP2112_GPIO_ADR: Address for GPIO node
+ */
+enum cp2112_child_acpi_cell_addrs {
+	CP2112_I2C_ADR = 0,
+	CP2112_GPIO_ADR = 1,
+};
+
 #define CP2112_REPORT_MAX_LENGTH		64
 #define CP2112_GPIO_CONFIG_LENGTH		5
 #define CP2112_GPIO_GET_LENGTH			2
@@ -1208,7 +1220,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct cp2112_device *dev;
 	u8 buf[3];
 	struct cp2112_smbus_config_report config;
+	struct fwnode_handle *cp2112_fwnode;
+	struct fwnode_handle *child;
 	struct gpio_irq_chip *girq;
+	u32 addr;
 	int ret;
 
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1226,6 +1241,28 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	cp2112_fwnode = dev_fwnode(&hdev->dev);
+	if (is_acpi_device_node(cp2112_fwnode)) {
+		fwnode_for_each_child_node(cp2112_fwnode, child) {
+			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
+			if (ret)
+				continue;
+
+			switch (addr) {
+			case CP2112_I2C_ADR:
+				device_set_node(&dev->adap.dev, child);
+				break;
+			case CP2112_GPIO_ADR:
+				dev->gc.fwnode = child;
+				break;
+			}
+		}
+	} else if (is_of_node(cp2112_fwnode)) {
+		child = fwnode_get_named_child_node(cp2112_fwnode, "i2c");
+		device_set_node(&dev->adap.dev, child);
+		fwnode_handle_put(child);
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");

-- 
2.25.1

[PATCH v14 2/2] HID: cp2112: Configure I2C bus speed from firmware

[Danny Kaehn]

Now that the I2C adapter on the CP2112 can have an associated firmware
node, set the bus speed based on firmware configuration

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---
 drivers/hid/hid-cp2112.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index e960fc988058..04379db93571 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1223,6 +1223,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct fwnode_handle *cp2112_fwnode;
 	struct fwnode_handle *child;
 	struct gpio_irq_chip *girq;
+	struct i2c_timings timings;
 	u32 addr;
 	int ret;
 
@@ -1308,6 +1309,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_power_normal;
 	}
 
+	i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
+
+	config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
 	config.retry_time = cpu_to_be16(1);
 
 	ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),

-- 
2.25.1

Re: [PATCH v14 1/2] HID: cp2112: Add fwnode support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Premature release of firmware node references results in use-after-free vulnerabilities when assigning the I2C adapter and GPIO chip firmware nodes.
--

commit 3dfaf368e855835bf23009205c4b93a2a070aae1
Author: Danny Kaehn <danny.kaehn@plexus.com>

HID: cp2112: Add fwnode support

This commit adds firmware node support to describe the CP2112's I2C and
GPIO interfaces. It differentiates between ACPI and DeviceTree bindings,
mapping the child nodes to the respective adapter and gpio_chip devices.

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 803b883ae8750..e960fc988058f 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
[ ... ]
> @@ -1226,6 +1241,28 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> +	cp2112_fwnode = dev_fwnode(&hdev->dev);
> +	if (is_acpi_device_node(cp2112_fwnode)) {
> +		fwnode_for_each_child_node(cp2112_fwnode, child) {
> +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> +			if (ret)
> +				continue;
> +
> +			switch (addr) {
> +			case CP2112_I2C_ADR:
> +				device_set_node(&dev->adap.dev, child);
> +				break;
> +			case CP2112_GPIO_ADR:
> +				dev->gc.fwnode = child;
> +				break;
> +			}

[Severity: High]
Does this code leave dangling pointers in the device structures?

The fwnode_for_each_child_node() macro automatically drops the reference to
the child node when advancing to the next node or exiting the loop.

Because device_set_node() and direct pointer assignments do not increment
the reference count, the nodes assigned here could be prematurely freed
resulting in a use-after-free.

> +		}
> +	} else if (is_of_node(cp2112_fwnode)) {
> +		child = fwnode_get_named_child_node(cp2112_fwnode, "i2c");
> +		device_set_node(&dev->adap.dev, child);
> +		fwnode_handle_put(child);

[Severity: High]
Are we releasing the firmware node prematurely here?

fwnode_get_named_child_node() returns a node with an incremented reference
count. However, since device_set_node() does not take its own reference on
the child node, dropping the reference immediately after could leave
dev->adap.dev with a dangling pointer if the node is later removed.

> +	}
> +
>  	ret = hid_parse(hdev);
>  	if (ret) {
>  		hid_err(hdev, "parse failed\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-cp2112-dt-v14-0-b1b4b6734b6f@plexus.com?part=1

Re: [PATCH v14 2/2] HID: cp2112: Configure I2C bus speed from firmware

[sashiko-bot]

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

New issues:
- [Medium] The driver silently overwrites custom hardware I2C clock speeds with a 100kHz default for devices without firmware nodes.

Pre-existing issues:
- [High] Firmware nodes are attached to device structures without holding a reference, leading to a dangling pointer and use-after-free.
- [High] The delayed work `gpio_poll_worker` is not canceled in the probe error path, leading to a use-after-free if the probe fails.
--

commit 07ff467e4d8df706a7a27476591cd82570fb8287
Author: Danny Kaehn <danny.kaehn@plexus.com>

    HID: cp2112: Configure I2C bus speed from firmware
    
    Now that the I2C adapter on the CP2112 can have an associated firmware
    node, set the bus speed based on firmware configuration.

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index e960fc988058f..04379db93571b 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
[ ... ]
> @@ -1308,6 +1309,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err_power_normal;
>  	}
>  
> +	i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this new call dereference
a dangling pointer?

Earlier in cp2112_probe(), the reference to the child firmware node is
dropped either explicitly via fwnode_handle_put(child) in the OF path, or
implicitly when the fwnode_for_each_child_node loop iterates or completes
in the ACPI path:

	} else if (is_of_node(cp2112_fwnode)) {
		child = fwnode_get_named_child_node(cp2112_fwnode, "i2c");
		device_set_node(&dev->adap.dev, child);
		fwnode_handle_put(child);
	}

device_set_node(&dev->adap.dev, child) assigns the pointer without taking
its own reference, leaving dev->adap.dev.fwnode as a dangling pointer.

This newly added i2c_parse_fw_timings() call dereferences this released
fwnode via device_property_read_u32().

Could this lead to a use-after-free?

> +	config.clock_speed = cpu_to_be32(timings.bus_freq_hz);

[Severity: Medium]
Since use_defaults is set to true, does this overwrite custom hardware I2C
clock speeds with a 100kHz default for devices without firmware nodes?

Before this patch, it looks like cp2112_probe() read the current SMBus
configuration from the device, only modified retry_time, and wrote it back.
This preserved the device's original configured clock speed (which might be
set to 400kHz in the EEPROM).

By calling i2c_parse_fw_timings() with an uninitialized local timings
struct and use_defaults = true, timings.bus_freq_hz will be
unconditionally set to 100kHz if the device lacks a firmware property.

Is it intended to silently downgrade the speed and override user-configured
EEPROM settings?

[Severity: High]
This is a pre-existing issue, but is there a missing cancellation of the
delayed work in the probe error path?

If sysfs_create_group() fails during probe, the code jumps to the
err_gpiochip_remove label, which calls gpiochip_remove(&dev->gc) and
returns:

	ret = sysfs_create_group(&hdev->dev.kobj, &cp2112_attr_group);
	if (ret < 0) {
		...
		goto err_gpiochip_remove;
	}
	...
err_gpiochip_remove:
	gpiochip_remove(&dev->gc);
err_free_i2c:

Because gpiochip_add_data() was already called, an IRQ consumer could have
concurrently requested a GPIO line, triggering cp2112_gpio_irq_startup()
and queuing dev->gpio_poll_worker.

Unlike cp2112_remove(), the error path lacks a call to
cancel_delayed_work_sync(&dev->gpio_poll_worker).

Could this mean the queued work will execute on freed memory?

>  	config.retry_time = cpu_to_be16(1);
>  
>  	ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-cp2112-dt-v14-0-b1b4b6734b6f@plexus.com?part=2