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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] i3c: hub: TOCTOU race and disconnect during I3C master registration
- [Medium] i3c: hub: inconsistent DT child parsing allows port usurpation
--

commit 0eedb3548ed97f556aa49d9f3afacf2b02004acb
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

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

This commit adds I3C hub functionality for the NXP P3H2x4x family. It
enables I3C/I2C communication between host and hub, transparent communication
with downstream devices, and target port configuration.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> --- /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);
> +	struct p3h2x4x *p3h2x4x = dev_get_drvdata(dev->parent);
> +	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;

[Severity: Medium]
Does this loop potentially capture unrelated nodes with a "reg" property?

Unlike p3h2x4x_parse_tp_dt_settings() below which checks if the node name is
"i3c" or "smbus", this loop iterates over all children of the MFD node without
checking the node name. If an unrelated child has a "reg" property, it might
usurp a valid target port slot and incorrectly default to P3H2X4X_TP_MODE_I3C.

[ ... ]
> +static int p3h2x4x_parse_tp_dt_settings(struct device *dev,
> +					const struct device_node *node,
> +					struct tp_configuration tp_config[])
> +{
> +	struct p3h2x4x *p3h2x4x = dev_get_drvdata(dev->parent);
> +	u64 id;
> +	int ret;
> +
> +	for_each_available_child_of_node_scoped(node, tp_node) {
> +		enum p3h2x4x_tp_mode mode;
> +
> +		/*
> +		 * Only "i3c" and "smbus" children describe target ports. Skip any
> +		 * other child (for example the MFD "regulators" container), which
> +		 * has no "reg" property.
> +		 */
> +		if (of_node_name_eq(tp_node, "i3c"))
> +			mode = P3H2X4X_TP_MODE_I3C;
> +		else if (of_node_name_eq(tp_node, "smbus"))
> +			mode = P3H2X4X_TP_MODE_SMBUS;
> +		else
> +			continue;

[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> +{
[ ... ]
> +		ret = i3c_master_register_fwnode(controller,
> +						 p3h2x4x_hub->dev,
> +						 of_fwnode_handle(p3h2x4x_hub->tp_bus[tp].of_node),
> +						 &i3c_hub_master_ops,
> +						 false);

[Severity: High]
Could this lead to a physical port disconnection and subsequent transfer
failures?

When i3c_master_register_fwnode() is called here, it triggers DAA. At this
time, the always_enable flag is false, meaning p3h2x4x_hub_enable_port() and
p3h2x4x_hub_disable_port() will actively manipulate P3H2X4X_TP_NET_CON_CONF.
After DAA completes, the port is left physically disconnected.

> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(p3h2x4x_hub->dev,
> +					       p3h2x4x_unregister_i3c_master,
> +					       controller);
> +		if (ret)
> +			return ret;
> +
> +		ntwk_mask |= p3h2x4x_hub->tp_bus[tp].tp_mask;
> +		p3h2x4x_hub->tp_bus[tp].is_registered = true;
> +		p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> +	}
> +	return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);

[Severity: High]
Because always_enable is set to true in the loop, subsequent port enable and
disable callbacks become no-ops. However, the hardware register
P3H2X4X_TP_NET_CON_CONF isn't actually written until the very end of the loop.

Does this leave a window where the port is exposed but physically
disconnected? If an I3C transfer is initiated concurrently before the loop
finishes, won't it fail due to the software assuming the port is always
enabled while the hardware hasn't been updated yet?

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