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

On Fri, Jun 12, 2026 at 11:39:48AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
> - [High] Regulator devres cleanup actions are erroneously attached to the parent device instead of the child device being probed.
> - [High] SMBus I2C adapters are not unregistered on probe error paths, causing a Use-After-Free.
> - [High] In-Band Interrupt (IBI) remains requested and enabled on probe failure, causing a Use-After-Free.
> - [High] Unsafe reliance on non-deterministic delays without polling the SMBus finish flag causes premature data reads.
> - [High] The driver dynamically overwrites the shared platform device's `of_node` pointer, corrupting the core device tree state.
> - [High] `regmap_update_bits` uses the configuration value as the bitmask, failing to clear stale bits.
> - [High] DAA is performed on the upstream bus before the downstream target ports are physically enabled.
> - [Medium] Target port device tree node references acquired during probe are leaked on error paths.
> - [Medium] Default configuration structures are populated with enum values instead of Ohms, causing misconfiguration.
> - [Low] Dead code due to incorrect `IS_ERR` check on `devm_kzalloc` return value.
> --
>
> commit 8d18a29a52d0297c45635edaf397ce828e709ef9
> Author: Aman Kumar Pandey <[email protected]>
>
> 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..fedcb978f07ee
> > --- /dev/null
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
>
> [ ... ]
>
> > +static int p3h2x4x_configure_ldo(struct device *dev)
> > +{
> > +	static const char * const supplies[] = {
> > +		"vcc1",
> > +		"vcc2",
> > +		"vcc3",
> > +		"vcc4"
> > +	};
> > +	int ret, i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(supplies); i++) {
> > +		ret = devm_regulator_get_enable_optional(dev->parent, supplies[i]);
>
> [Severity: High]
> Could this leak devres memory on the parent device? By passing dev->parent
> (the MFD parent device) instead of dev (the child hub device), the regulator
> cleanup actions are attached to the parent. If the hub driver fails probe
> or is repeatedly bound/unbound, it looks like these regulators won't be
> disabled and the references will be leaked.

look like make sense, please check it.

>
> [ ... ]
>
> > +static int p3h2x4x_configure_tp(struct device *dev)
> > +{
> > +	struct p3h2x4x_i3c_hub_dev *hub = dev_get_drvdata(dev);
> > +	u8 mode = 0, smbus = 0, pullup = 0, target_port = 0;
> > +	int tp, ret;
> > +
> > +	for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> > +		pullup |= hub->hub_config.tp_config[tp].pullup_en ? P3H2X4X_SET_BIT(tp) : 0;
>
> [ ... ]
>
> > +	ret = regmap_update_bits(hub->regmap, P3H2X4X_TP_PULLUP_EN, pullup, pullup);
>
> [Severity: High]
> Does this fail to clear stale bits? The third argument to regmap_update_bits()
> is the mask. By using 'pullup' as the mask, it seems this only applies
> operations to bits that are 1. Any bit that is 0 in 'pullup' is ignored,
> which might leave stale hardware state (like pullups enabled by the
> bootloader) unmodified. Should this use a proper mask covering all relevant
> bits?

Make sense, please check it.

>
> [ ... ]
>
> > +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]
> Is there a resource leak here on probe error paths? This manually increments
> the reference count of the child nodes using of_node_get(). While
> p3h2x4x_i3c_hub_remove() correctly drops these references, if
> p3h2x4x_i3c_hub_probe() fails and returns an error, the remove callback
> isn't executed. Without a devm cleanup action, could these OF nodes leak
> memory?
>
> [ ... ]
>
> > +static void p3h2x4x_default_configuration(struct device *dev)
> > +{
> > +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> > +	int tp_count;
> > +
> > +	p3h2x4x_i3c_hub->hub_config.tp0145_pullup = P3H2X4X_TP_PULLUP_500R;
>
> [Severity: Medium]
> Are these default configuration values being set correctly? These fields
> appear to be designed to store physical Ohm measurements, but they are
> initialized to macros like P3H2X4X_TP_PULLUP_500R which evaluate to enum
> indices (e.g., 1). When the default value 1 is later processed by
> p3h2x4x_pullup_dt_to_reg() using find_closest(), it would resolve to the
> closest table entry 250 instead of the intended 500. Should these initialize
> with real-world units?
>
> [ ... ]
>
> > +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> > +{
>
> [ ... ]
>
> > +	/* Register virtual I2C adapters for SMBus target ports */
> > +	ret = p3h2x4x_tp_smbus_algo(p3h2x4x_i3c_hub);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to add i2c adapter\n");
> > +
> > +	/* Lock access to protected registers */
> > +	ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> > +			   P3H2X4X_REGISTERS_LOCK_CODE);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to lock HUB's protected registers\n");
>
> [Severity: High]
> Are the SMBus I2C adapters cleaned up if this final regmap_write() fails?
> p3h2x4x_tp_smbus_algo() registers adapters using i2c_add_adapter(), which
> requires an explicit i2c_del_adapter(). If the probe fails here, the adapters
> are never unregistered, which leaves the I2C core with active references to
> devm-freed memory. Could this cause a use-after-free?
>
> > +
> > +	return 0;
> > +}
>
> > 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..38505dda0e81b
> > --- /dev/null
> > +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
>
> [ ... ]
>
> > +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> > +{
>
> [ ... ]
>
> > +	p3h2x4x_hub->hub = devm_kzalloc(p3h2x4x_hub->dev,
> > +					sizeof(*p3h2x4x_hub->hub),
> > +					GFP_KERNEL);
> > +
> > +	if (!p3h2x4x_hub->hub)
> > +		return -ENOMEM;
> > +
> > +	i3c_hub_init(p3h2x4x_hub->hub,
> > +		     &p3h2x4x_hub_ops,
> > +		     p3h2x4x_hub->i3cdev);
> > +
> > +	if (IS_ERR(p3h2x4x_hub->hub))
> > +		return PTR_ERR(p3h2x4x_hub->hub);
>
> [Severity: Low]
> Is this IS_ERR() check necessary? devm_kzalloc() returns either a valid
> pointer or NULL on failure, never an error pointer. It seems this block of
> code might be dead.

Make sense, please fix it

Frank
>
> [ ... ]
>
> > +	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]
> Can dynamically overwriting the platform device's of_node pointer corrupt the
> core device tree state? By changing p3h2x4x_hub->dev->of_node in a loop
> without saving and restoring the original, it looks like this permanently
> changes the node the device points to. Could this break device tree links,
> sysfs topologies, or devres lookups that rely on the original parent hub
> of_node?
>
> [ ... ]
>
> > +		/* Perform DAA */
> > +		ret = i3c_master_do_daa(parent);
> > +		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;
> > +	}
>
> [ ... ]
>
> > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > +	ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);
>
> [Severity: High]
> If a subsequent step in p3h2x4x_i3c_hub_probe() fails, are the IBIs safely
> cleaned up? The devm framework would free the p3h2x4x_i3c_hub structure,
> but the IBI might remain active on the master. If an interrupt fires later
> and accesses the freed structure via i3cdev_get_drvdata(i3cdev), could this
> trigger a use-after-free?
>
> > +	if (ret) {
> > +		i3c_device_free_ibi(p3h2x4x_hub->i3cdev);
> > +		return ret;
> > +	}
> > +#endif
> > +	ret = regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
>
> [Severity: High]
> Does performing DAA here actually reach the downstream targets? The DAA is
> executed earlier on the upstream bus inside the target port loop, but the
> downstream target ports don't appear to be logically connected to the upstream
> bus until ntwk_mask is written to P3H2X4X_TP_NET_CON_CONF at the end of the
> function. Will downstream targets receive the ENTDAA command if DAA is done
> before the physical ports are enabled?
>
> > 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..edb75f790e92e
> > --- /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]
> Could relying on a fixed timeout instead of polling the hardware finish flag
> cause the driver to read stale data? Because the top 4 bits of the status
> register are initialized to 0 before the transaction begins, they remain 0
> while the hardware is busy. If the transaction exceeds the timeout (e.g., due
> to clock stretching), status evaluates to 0
> (P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK). Should this code poll the
> P3H2X4X_SMBUS_TRANSACTION_FINISH_FLAG instead to ensure completion?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

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