Re: [PATCH v13 6/7] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality

On Wed, Jul 01, 2026 at 07:21:48AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Unprotected and temporary reassignment of a live device's `of_node` creates a data race, and missing restoration on the error path causes reference count leaks and a double-free.
> - [High] `regmap_read_poll_timeout` is configured with a 10ms sleep interval that vastly exceeds the actual total timeout, artificially delaying every transaction and causing spurious timeouts.
> - [High] Failure to allocate memory for an I2C adapter leaves previously registered adapters active but with freed backing memory, leading to a Use-After-Free.
> - [Medium] Loop counters `msg_count` and `nxfers_i` are `u8`, causing infinite loops if the transfer count `num` is >= 256.
> - [Medium] Race condition between the I3C hub driver and other MFD child drivers over the shared `P3H2X4X_DEV_REG_PROTECTION_CODE` register.
> --
>
> commit 37198d19f613736872b6f1fd4f9a75597b1f0ab1
> Author: Aman Kumar Pandey <[email protected]>
>
> i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
>
> This patch adds I3C hub functionality for the NXP P3H2x4x family of multiport
> hubs. It enables I3C/I2C communication between host and hub, downstream device
> support, and target port configuration.
>
> > diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> > --- a/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> [ ... ]
> > +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> > +{
> > +	struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
> > +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node;
> > +	int ret, i;
> [ ... ]
> > +	/* Unlock access to protected registers */
> > +	ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> > +			   P3H2X4X_REGISTERS_UNLOCK_CODE);
>
> [Severity: Medium]
> Could this global unlock race with other MFD child drivers (such as the
> on-die regulator)?
>
> Because the protected configuration registers are unlocked at the start of
> p3h2x4x_i3c_hub_probe() and left unlocked during the entire hardware
> configuration and device discovery window (which includes sleeping in
> i3c_master_do_daa()), a concurrent MFD child driver could access its own
> registers and re-lock the protection code. If that happens, subsequent
> hardware configuration writes in this function might fail silently.
>
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to unlock HUB's protected registers\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, p3h2x4x_lock_registers_action,
> > +				       p3h2x4x_i3c_hub);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = p3h2x4x_configure_hw(dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to configure the HUB\n");
> [ ... ]
>
> > diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> > --- a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> [ ... ]
> > +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> > +{
> > +	struct i3c_master_controller *parent = i3c_dev_get_master(p3h2x4x_hub->i3cdev->desc);
> > +	struct device_node *orig_of_node;
> > +	u8 tp, ntwk_mask = 0;
> > +	int ret;
> [ ... ]
> > +	for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> > +		if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> > +		    p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> > +			continue;
> > +
> > +		/*
> > +		 * i3c_master_register() derives bus population from the parent device's
> > +		 * OF node. Temporarily switch the hub device to the target-port node for
> > +		 * registration, then restore the original node immediately.
> > +		 */

i3c_master_register() use dev of node. if add API i3c_master_register_fwnode(),
pass down a children fwnode, things will be simple, and no below AI' report
issue

> > +		orig_of_node = p3h2x4x_hub->dev->of_node;
> > +		p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
> > +
> > +		struct i3c_hub_controller *hub_controller =
> > +				&p3h2x4x_hub->tp_bus[tp].hub_controller;
> > +		struct i3c_master_controller *controller = &hub_controller->controller;
> > +
> > +		hub_controller->parent = parent;
> > +		hub_controller->hub = p3h2x4x_hub->hub;
> > +
> > +		dev_set_drvdata(&controller->dev, hub_controller);
> > +
> > +		ret = i3c_hub_reserve_parent_addrslots_from_dt(hub_controller,
> > +							       p3h2x4x_hub->tp_bus[tp].of_node);
> > +		if (ret)
> > +			return ret;
>
> [Severity: High]
> Will this leak the original parent node reference and cause a double-free on
> the target node if i3c_hub_reserve_parent_addrslots_from_dt() returns an
> error?
>
> If the early return is taken, dev->of_node is never restored to
> orig_of_node. When the probe unwinds, the devres cleanup function
> p3h2x4x_put_target_port_of_nodes() drops the target node's reference.
> Later, when the device core destroys the device, it will call
> of_node_put(dev->of_node) which now incorrectly points to the target node
> again, causing a double-free.
>
> Concurrently, modifying dev->of_node without synchronization creates a data
> race for concurrent sysfs or uevent reads.
>
> > +
> > +		ret = i3c_master_register(controller,
> > +					  p3h2x4x_hub->dev,
> > +					  i3c_hub_master_ops(),
> > +					  false);
> > +
> > +		p3h2x4x_hub->dev->of_node = orig_of_node;
> [ ... ]
>
> > diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> > --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> [ ... ]
> > +static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
> > +						 u8 target_port_status,
> > +						 u8 data_length)
> > +{
> > +	u32 status_read;
> > +	u8 status;
> > +	int ret;
> > +
> > +	ret = regmap_read_poll_timeout(hub->regmap, target_port_status,
> > +				       status_read,
> > +				       status_read & P3H2X4X_SMBUS_TRANSACTION_FINISH_FLAG,
> > +				       P3H2X4X_POLLING_ROLL_PERIOD_MS * 1000,
> > +				       P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
>
> [Severity: High]
> Does this polling configuration cause spurious timeouts or severe performance
> penalties?
>
> The sleep_us parameter is set to P3H2X4X_POLLING_ROLL_PERIOD_MS * 1000,
> which is 10,000us (10ms). However, the total timeout_us parameter provided
> by P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length) is significantly smaller
> (e.g., between 100us and 2,000us depending on data length).
>
> If the transaction is not complete on the very first read, the thread will
> sleep for a full 10ms, drastically overshooting the intended timeout and
> potentially returning a spurious -ETIMEDOUT depending on the state when it
> wakes up.

Make sense, pleas double check it

>
> > +	if (ret)
> > +		return ret;
> [ ... ]
> > +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> > +{
> > +	int ret_sum = 0, ret;
> > +	u8 msg_count, rw;
> > +
> > +	struct tp_bus *bus = i2c_get_adapdata(adap);
> > +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
> > +
> > +	guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
> > +	guard(mutex)(&bus->port_mutex);
> > +
> > +	for (msg_count = 0; msg_count < num; msg_count++) {
>
> [Severity: Medium]
> Can this loop run infinitely if the transfer count (num) is greater than
> or equal to 256?
>
> The parameter num is a signed 32-bit integer, but msg_count is declared as
> u8. If an in-kernel driver submits an I2C transfer with 256 or more
> messages, msg_count will wrap around to 0 before ever reaching num, making
> the loop condition perpetually true and causing a CPU lockup.

make sense, please check

>
> > +		if (msgs[msg_count].len > P3H2X4X_SMBUS_PAYLOAD_SIZE) {
> > +			dev_err(p3h2x4x_i3c_hub->dev,
> > +				"Message nr. %d not sent - length over %d bytes.\n",
> > +				msg_count, P3H2X4X_SMBUS_PAYLOAD_SIZE);
> > +			return -EINVAL;
> > +		}
> [ ... ]
> > +int p3h2x4x_tp_smbus_algo(struct p3h2x4x_i3c_hub_dev *hub)
> > +{
> > +	int ret;
> > +	u8 tp;
> > +
> > +	for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> > +		if (!hub->tp_bus[tp].of_node ||
> > +		    hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_SMBUS)
> > +			continue;
> > +
> > +		/* Allocate adapter */
> > +		struct i2c_adapter *smbus_adapter =
> > +			devm_kzalloc(hub->dev, sizeof(*smbus_adapter), GFP_KERNEL);
> > +		if (!smbus_adapter)
> > +			return -ENOMEM;
>
> [Severity: High]
> Will this early return on allocation failure cause a use-after-free for
> previously registered I2C adapters?
>
> In this loop, if devm_kzalloc() fails for a given port, the function returns
> -ENOMEM immediately without calling p3h2x4x_unregister_smbus_adapters().

make sense, please check it.

Frank
>
> Any adapters successfully registered in earlier loop iterations will remain
> active in the I2C subsystem. Because the devres unregister action
> (p3h2x4x_unregister_smbus_adapters_action) is only added at the end of the
> probe() function, it will not execute. However, devres will automatically
> free the memory allocated by devm_kzalloc(), leaving the I2C subsystem to
> dereference freed memory when it interacts with those remaining dangling
> adapters.
>
> > +
> > +		/* Initialize adapter */
> > +		smbus_adapter->owner = THIS_MODULE;
> > +		smbus_adapter->class = I2C_CLASS_HWMON;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c