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

[sashiko-bot@kernel.org]

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 <aman.kumarpandey@nxp.com>

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

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

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

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/20260701065755.2067793-1-lakshay.piplani@nxp.com?part=6

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c