[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

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 1/2] HID: cp2112: Add fwnode support

[Benjamin Tissoires]

On May 20 2026, sashiko-bot@kernel.org wrote:
> 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.

Sashiko is overly cautious here. ACPI handles are not refcounted, so we
are fine.

That being said, we could unify the OF and ACPI behaviour by calling the
noops fwnode_handle_get(child) for ACPI as well and clean them up in
cp2112_remove(). This would also shut up the next warning from Sashiko
below.

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

OF children are tied to the lifespan of the OF device, so we are fine,
but technically incorrect, as mentioned by Sashiko.

If you regroup the 2 behaviours and clean them in cp2112_remove(), this
would be cleaner IMO.

Cheers,
Benjamin

> 
> > +	}
> > +
> >  	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 1/2] HID: cp2112: Add fwnode support

[Danny Kaehn]

On Tue, Jun 02, 2026 at 09:55:18AM +0200, Benjamin Tissoires wrote:
> On May 20 2026, sashiko-bot@kernel.org wrote:
> > 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.
> 
> Sashiko is overly cautious here. ACPI handles are not refcounted, so we
> are fine.
> 
> That being said, we could unify the OF and ACPI behaviour by calling the
> noops fwnode_handle_get(child) for ACPI as well and clean them up in
> cp2112_remove(). This would also shut up the next warning from Sashiko
> below.
> 
> > 
> > > +		}
> > > +	} 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.
> 
> OF children are tied to the lifespan of the OF device, so we are fine,
> but technically incorrect, as mentioned by Sashiko.
> 
> If you regroup the 2 behaviours and clean them in cp2112_remove(), this
> would be cleaner IMO.
>

Ack, this is something I've gone back-and-forth on in the revisions of
this patch. Will update the driver to hold the fwnode reference for the
I2C subnode.

However, for the GPIO chip, DT uses the parent node (which shouldn't
need an increment to the refcount), while ACPI uses a child node. Given
this discussion, it sounds like I ought to still increment the refcount
for the GPIO child node to comply to the interface, even though ACPI
handles aren't refcounted. This will require another bit of DT/ACPI
diverging behavior in _remove(), which is fine by me, just announcing
that intent in case it doesn't match your expectation / might appear as
a code smell in the follow-up patch.




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

Thanks,

Danny Kaehn

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

[Benjamin Tissoires]

On Jun 04 2026, Danny Kaehn wrote:
> 
> 
> On Tue, Jun 02, 2026 at 09:55:18AM +0200, Benjamin Tissoires wrote:
> > On May 20 2026, sashiko-bot@kernel.org wrote:
> > > 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.
> > 
> > Sashiko is overly cautious here. ACPI handles are not refcounted, so we
> > are fine.
> > 
> > That being said, we could unify the OF and ACPI behaviour by calling the
> > noops fwnode_handle_get(child) for ACPI as well and clean them up in
> > cp2112_remove(). This would also shut up the next warning from Sashiko
> > below.
> > 
> > > 
> > > > +		}
> > > > +	} 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.
> > 
> > OF children are tied to the lifespan of the OF device, so we are fine,
> > but technically incorrect, as mentioned by Sashiko.
> > 
> > If you regroup the 2 behaviours and clean them in cp2112_remove(), this
> > would be cleaner IMO.
> >
> 
> Ack, this is something I've gone back-and-forth on in the revisions of
> this patch. Will update the driver to hold the fwnode reference for the
> I2C subnode.
> 
> However, for the GPIO chip, DT uses the parent node (which shouldn't
> need an increment to the refcount), while ACPI uses a child node. Given

Oh, if they are different, then maybe following that advice is not such
a good idea.

> this discussion, it sounds like I ought to still increment the refcount
> for the GPIO child node to comply to the interface, even though ACPI
> handles aren't refcounted.

Yeah, sashiko says that this is harmless and would provide symetrical
behavior (but artificial)

> This will require another bit of DT/ACPI
> diverging behavior in _remove(), which is fine by me, just announcing
> that intent in case it doesn't match your expectation / might appear as
> a code smell in the follow-up patch.

I thought you would just deref the stored node, so it doesn't matter
whether the original node is a parent or a child. Especially because it
would be a noop for ACPI.

But if there are no way of having a single fwnode_handle_put(node) (per
ref, of course), then maybe we can stick to the current code.

We should probably fix the other speed downgrade from 2/2.

Cheers,
Benjamin

> 
> 
> 
> 
> > Cheers,
> > Benjamin
> > 
> > > -- 
> > > Sashiko AI review � https://sashiko.dev/#/patchset/20260520-cp2112-dt-v14-0-b1b4b6734b6f@plexus.com?part=1
> 
> Thanks,
> 
> Danny Kaehn
> 
> 

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

[Andy Shevchenko]

On Wed, May 20, 2026 at 11:13:06AM -0500, Danny Kaehn wrote:
> 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.

As we seems settled on the approaches in both cases, I think we are fine
to go
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Jacky Huang]

Hi Danny,

Thank you for all your work bringing this through so many revisions, and
I am glad to help by testing it.

I tested this on an AST2600-based OpenBMC platform (NVIDIA Vera Rubin)
that has several CP2112s. With this patch, a CP2112 whose device-tree
"i2c" subnode declares downstream I2C devices (PCA9555 expanders) has
them auto-instantiated by i2c-core: they appeared on the adapter's I2C
bus with their gpio-line-names. CP2112s with an empty "i2c" node
registered their adapters but instantiated no children, as expected.

Tested-by: Jacky Huang <jackyhuang@nvidia.com>

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

[Bartosz Golaszewski]

On Wed, 20 May 2026 18:13:06 +0200, Danny Kaehn <danny.kaehn@plexus.com> said:
> 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>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

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

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

[Benjamin Tissoires]

On May 20 2026, sashiko-bot@kernel.org wrote:
> 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?

Cleaning up the refcounting in the previous patch would solve this
(false positive, admitedly).

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

I think we should fix this:
i2c_parse_fw_timings(&dev->adap.dev, &timings, false);
if (timings.bus_freq_hz)
    config.clock_speed = cpu_to_be32(timings.bus_freq_hz);

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

This one would be nice to fix in a follow up patch.

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

Cheers,
Benjamin

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

[Danny Kaehn]

Hi Benjamin,

Thanks for the review; will fix the issue pointed out here.

On Tue, Jun 02, 2026 at 09:59:23AM +0200, Benjamin Tissoires wrote:
> On May 20 2026, sashiko-bot@kernel.org wrote:
> > 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)
...
> > > +	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?
> 
> I think we should fix this:
> i2c_parse_fw_timings(&dev->adap.dev, &timings, false);
> if (timings.bus_freq_hz)
>     config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
>

Yeah, agreed, a good find. The CP2112's default is 100kHz, but
technically possible the bootloader or something has changed it from the
default. Will fix.

> > 
> > [Severity: High]
> > This is a pre-existing issue, but is there a missing cancellation of the
> > delayed work in the probe error path?
> 
> This one would be nice to fix in a follow up patch.
>

(Not planning to include with this patchset for sake of churn, but could
perhaps be fixed separately in the future... I do have a handfull of
other CP2112 patches to send on once these merge).

> > >  
> > >  	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
> > 
> 
> Cheers,
> Benjamin

Thanks,

Danny Kaehn

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

[Jiri Kosina]

On Wed, 20 May 2026, Danny Kaehn wrote:

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

Applied to hid.git#for-7.2/cp2112, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: Firmware Support for USB-HID Devices and CP2112

[Andy Shevchenko]

In lieu of this discussion [1] I just realised that CP2112 misses the
documentation piece under Documentation/firmware-guide/acpi/ on how
it's supposed to be enumerated. Danny, can you add a such, please?

[1]: 20261003095924.1370238-1-thierry.reding@kernel.org

-- 
With Best Regards,
Andy Shevchenko

Re: Firmware Support for USB-HID Devices and CP2112

[Danny Kaehn]

On Wed, Nov 04, 2026 at 07:19:46PM +0200, Andy Shevchenko wrote:
> In lieu of this discussion [1] I just realised that CP2112 misses the
> documentation piece under Documentation/firmware-guide/acpi/ on how
> it's supposed to be enumerated. Danny, can you add a such, please?
> 
> [1]: 20261003095924.1370238-1-thierry.reding@kernel.org
>

Hi Andy,

Apologies about the delay here -- getting an environment set up to send
kernel email always takes some time :)

I will target getting something sent out for this by the end of the
week.

This can wait for the review of course, but since it seems like
this is a relatively unblazed trail (ACPI "binding" documents for
specific drivers), do you have opinions on whether a directory tree
should be started for driver documents like this? (i.e. an i2c
subdirectory?), or would you prefer it just be added to the top-level
directory? It seems like there also isn't yet a naming convention for
these -- presumably the "silabs,cp2112.rst" name would be too
"DeviceTree-ish"... maybe "i2c-silabs-cp2112.rst" if kept at the
top-level...

Thanks,

Danny Kaehn

Re: Firmware Support for USB-HID Devices and CP2112

[Andy Shevchenko]

On Tue, Nov 10, 2026 at 12:56:03PM -0600, Danny Kaehn wrote:
> On Wed, Nov 04, 2026 at 07:19:46PM +0200, Andy Shevchenko wrote:
> > In lieu of this discussion [1] I just realised that CP2112 misses the
> > documentation piece under Documentation/firmware-guide/acpi/ on how
> > it's supposed to be enumerated. Danny, can you add a such, please?
> > 
> > [1]: 20261003095924.1370238-1-thierry.reding@kernel.org

> Apologies about the delay here -- getting an environment set up to send
> kernel email always takes some time :)
> 
> I will target getting something sent out for this by the end of the
> week.
> 
> This can wait for the review of course, but since it seems like
> this is a relatively unblazed trail (ACPI "binding" documents for
> specific drivers), do you have opinions on whether a directory tree
> should be started for driver documents like this? (i.e. an i2c
> subdirectory?), or would you prefer it just be added to the top-level
> directory? It seems like there also isn't yet a naming convention for
> these -- presumably the "silabs,cp2112.rst" name would be too
> "DeviceTree-ish"... maybe "i2c-silabs-cp2112.rst" if kept at the
> top-level...

Hmm... Since you rose this topic, I think we can move i2c-mux ACPI
documentation to i2c folder and create there the support for that specific
hardware in DTish naming schema.

-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

On Wed, May 20, 2026 at 11:13:05AM -0500, Danny Kaehn wrote:
> 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.
> 
 
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>