[PATCH v13 6/7] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Wed Jul 1 00:21:48 PDT 2026
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 at 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
More information about the linux-i3c
mailing list