Re: [PATCH v14 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 5 potential issue(s) to consider:
- [High] Child driver illegaly overwrites the parent MFD device's private data, causing type confusion and kernel panics.
- [High] The SMBus timeout formula underestimates minimum physical wire time, causing large block transfers to inevitably fail with -ETIMEDOUT.
- [High] Target ports configured as "i2c" are left completely disconnected and functionally dead.
- [High] Use-After-Free of device structure embedded in devm-allocated memory during driver unbind.
- [Medium] Missing I2C adapter quirks causes undocumented rigid transfer size limits.
--

--- Patch [7]: [PATCH v14 7/8] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality ---
commit cfe07c5ef0a917e7757bc31fa34e458df22a78ed
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.h b/drivers/i3c/hub/p3h2840_i3c_hub.h
> index 0000000000000..e7ab243bbfff6
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub.h
[ ... ]
> +/* Hub SMBus transaction time */
> +#define P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(x)		((20 * (x)) + 80)

[Severity: High]
Is this timeout formula sufficient for large SMBus block transfers?

At 400kHz, each 9-bit I2C byte takes 22.5 us. For a 32-byte payload plus a
1-byte address, the minimum physical wire time is 33 * 22.5 = 742.5 us.
However, the formula 20 * x + 80 allocates only 720 us for a 32-byte payload.

If a large payload message is sent, the timeout will trigger in
p3h2x4x_read_smbus_transaction_status() before the transfer can complete on
the wire, causing it to fail with a timeout.

[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> index 0000000000000..2fd9723da60d9
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
[ ... ]
> +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> +{
> +	struct p3h2x4x *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;
> +
> +	p3h2x4x_i3c_hub = devm_kzalloc(dev, sizeof(*p3h2x4x_i3c_hub), GFP_KERNEL);

[Severity: High]
Could this lead to a use-after-free during driver unbind?

The p3h2x4x_i3c_hub structure is allocated via devm_kzalloc() and contains
struct i3c_master_controller instances, which in turn embed struct device.

When the devm action calls i3c_master_unregister() on driver unbind, it
invokes device_unregister(). Because device_unregister() does not block to
wait for all sysfs references to drop, userspace might still hold references.
When devm subsequently frees the containing p3h2x4x_i3c_hub_dev structure,
accessing those remaining references will cause a use-after-free.

> +	if (!p3h2x4x_i3c_hub)
> +		return -ENOMEM;
[ ... ]
> +	/* Register virtual I3C master controllers for I3C target ports */
> +	if (p3h2x4x->i3cdev) {
> +		p3h2x4x_i3c_hub->i3cdev = p3h2x4x->i3cdev;
> +		i3cdev_set_drvdata(p3h2x4x->i3cdev, p3h2x4x_i3c_hub);

[Severity: High]
Does this code safely manage the parent device's private data?

By calling i3cdev_set_drvdata() on p3h2x4x->i3cdev here in
p3h2x4x_i3c_hub_probe(), it replaces the MFD parent's drvdata with the child
driver's context. If any parent callbacks try to retrieve their data, they
will incorrectly cast struct p3h2x4x_i3c_hub_dev to struct p3h2x4x, leading
to type confusion and data corruption.

> +		ret = p3h2x4x_tp_i3c_algo(p3h2x4x_i3c_hub);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to register i3c bus\n");
> +	}
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> index 0000000000000..0e5bf7eb3cf7f
> --- /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;

[Severity: High]
Will target ports configured as i2c in the device tree be registered?

The p3h2x4x_tp_i3c_algo() function explicitly skips ports unless they are in
I3C mode. Similarly, p3h2x4x_tp_smbus_algo() skips ports unless they are in
SMBus mode. As a result, ports configured as i2c are ignored entirely and
never registered or enabled in the hardware's network configuration mask.

> +
> +		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
> index 0000000000000..b991c46f3ed59
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
[ ... ]
> +	for (msg_count = 0; msg_count < num; msg_count++) {
> +		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;
> +		}

[Severity: Medium]
Would it be better to expose this size limitation to the I2C subsystem?

By enforcing a hard boundary here dynamically in p3h2x4x_tp_i2c_xfer()
without populating the i2c_adapter_quirks structure, the I2C subsystem
cannot automatically split up oversized reads and writes for the client.
This will result in unexpected failures for standard transactions that exceed
84 bytes.

> +
> +		rw = (msgs[msg_count].flags & I2C_M_RD) ? 1 : 0;
[ ... ]

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