Re: [PATCH v12 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] The SMBus transaction wait logic uses a fixed sleep instead of polling the hardware completion flag, leading to premature reads of incomplete transactions and data corruption.
- [High] Overwriting the parent device's `of_node` pointer iteratively causes a memory leak of the original parent node, a double-free of the target port nodes, and state corruption.
- [Medium] Potential `of_node` reference leak in `p3h2x4x_get_target_port_dt_conf()` if multiple child nodes specify the same target port index.
- [Medium] The hub's protected registers remain permanently unlocked if the hardware configuration or bus registration fails during probe.
- [Low] Comment describes a read followed by a write, but the code implements a write followed by a read.
--

commit 1c982ecf71c6fb3b765caf255191a821a5e37cb5
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality

Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
These devices support downstream target ports that can be configured
as I3C, I2C, or SMBus.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> new file mode 100644
> index 0000000000000..05e19bff20d5c
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
[ ... ]
> +static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
> +					    const struct device_node *node)
> +{
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> +	u64 tp_port;
> +
> +	for_each_available_child_of_node_scoped(node, dev_node) {
> +		if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
> +			continue;
> +
> +		if (tp_port < P3H2X4X_TP_MAX_COUNT) {
> +			p3h2x4x_i3c_hub->tp_bus[tp_port].of_node = of_node_get(dev_node);

[Severity: Medium]
Could this leak the of_node reference if a malformed device tree specifies
multiple child nodes with the same target port index?

If multiple child nodes specify the same reg value, the pointer is
overwritten with a new of_node_get() result, which abandons the previous
reference.

> +			p3h2x4x_i3c_hub->tp_bus[tp_port].tp_mask = P3H2X4X_SET_BIT(tp_port);
> +			p3h2x4x_i3c_hub->tp_bus[tp_port].p3h2x4x_i3c_hub = p3h2x4x_i3c_hub;
> +			p3h2x4x_i3c_hub->tp_bus[tp_port].tp_port = tp_port;
> +		}
> +	}
> +}
[ ... ]
> +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	/* Unlock access to protected registers */
> +	ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			   P3H2X4X_REGISTERS_UNLOCK_CODE);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to unlock HUB's protected registers\n");
> +
> +	ret = p3h2x4x_configure_hw(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to configure the HUB\n");

[Severity: Medium]
Are the hub's protected registers left permanently unlocked if probe fails
during p3h2x4x_configure_hw() or bus registration?

The driver unlocks the hardware registers at the start of probe and locks
them at the very end. However, if any initialization step fails and returns
an error, the function exits immediately without locking the registers again.

Could this be mitigated by using a devm_add_action_or_reset() callback or
explicit error handling labels to ensure the registers are safely relocked?

> +
> +	/* Register virtual I3C master controllers for I3C target ports */
> +	if (p3h2x4x->i3cdev) {
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> new file mode 100644
> index 0000000000000..8adb235b992c0
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> +{
[ ... ]
> +	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;
> +
> +		/* Assign DT node for this TP */
> +		p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;

[Severity: High]
Does overwriting the parent device's of_node pointer iteratively cause a
memory leak of the original parent node and a double-free of the target port
nodes?

The loop over target ports directly mutates the shared pdev->dev.of_node
pointer which was populated in probe using device_set_of_node_from_dev().
This leaks the reference to the original parent node, and leaves
dev->of_node pointing to the last target port node.

When the driver is unbound, the driver core will drop a reference on
dev->of_node, but p3h2x4x_put_target_port_of_nodes() also drops a reference
on the same node, which can cause a use-after-free or double-free refcount
underflow. Mutating dev->of_node also creates a race with sysfs/uevent.

Should the driver assign the node to controller->dev.of_node directly
instead of mutating the parent's pointer?

> +
> +		struct i3c_hub_controller *hub_controller =
> +				&p3h2x4x_hub->tp_bus[tp].hub_controller;
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> new file mode 100644
> index 0000000000000..cf664425af071
> --- /dev/null
> +++ 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;
> +
> +	fsleep(P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
> +
> +	ret = regmap_read(hub->regmap, target_port_status, &status_read);

[Severity: High]
Does using a hardcoded sleep based on bytes transferred instead of polling a
hardware completion status bit lead to premature reads of incomplete
transactions?

If the downstream device performs clock stretching or is slow to respond,
the transfer will take longer than the hardcoded timeout. The driver then
reads target_port_status and shifts to check the status bits.

Since an ongoing transaction leaves the status bits at 0
(P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK), the driver will incorrectly assume the
transaction has finished successfully. It will then proceed to read stale or
uninitialized memory buffers and return garbage data.

Could this be updated to use a polling loop checking the finish flag?

> +	if (ret)
> +		return ret;
> +
> +	status = (u8)status_read;
[ ... ]
> +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
[ ... ]
> +		rw = (msgs[msg_count].flags & I2C_M_RD) ? 1 : 0;
> +		if (!rw) {
> +			/* If a read message is immediately followed by a write message to
> +			 * the same address,  consider combining them into a single transaction.
> +			 */

[Severity: Low]
This isn't a bug, but the comment describes a read followed by a write while
the code implements a write followed by a read.

The actual code block checks !rw for the first message and flags & I2C_M_RD
for the second message, which accurately handles a write message immediately
followed by a read message.

> +			if (msg_count + 1 < num &&
> +			    msgs[msg_count].addr == msgs[msg_count + 1].addr &&
> +			    (msgs[msg_count + 1].flags & I2C_M_RD)) {
> +				if (msgs[msg_count].len + msgs[msg_count + 1].len >
> +				    P3H2X4X_SMBUS_PAYLOAD_SIZE)
> +					return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=6

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