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

[sashiko-bot@kernel.org]

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

[ ... ]

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

[ ... ]

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

[ ... ]

> +	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/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=8