Re: [PATCH v14 8/8] i3c: hub: p3h2x4x: Add SMBus slave mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprotected shared state modification after exposing the I2C adapter creates a race condition that can overwrite slave registration state and drop IBI data
- [Medium] Mixing goto-based error handling with scope-based guard() cleanup
--

--- Patch [8]: [PATCH v14 8/8] i3c: hub: p3h2x4x: Add SMBus slave mode support ---
commit 8afab6d0ec8653574d5df430584d53ab5ca6d8cb
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
Subject: i3c: hub: p3h2x4x: Add SMBus slave mode support

This commit adds SMBus slave mode support for the P3H2x4x hub SMBus target
ports. It sets up the receive path via IBI and forwards payloads to the
registered I2C slave client.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> index b991c46f3ed59..89e94f551b31f 100644
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> +			 const struct i3c_ibi_payload *payload)
> +{
[ ... ]
> +	for (i = 0; i < P3H2X4X_TP_MAX_COUNT; ++i) {
> +		if (!((payload_byte_two >> i) & 0x01))
> +			continue;
> +
> +		if (!p3h2x4x_i3c_hub->tp_bus[i].is_registered) {
> +			dev_dbg(&i3cdev->dev, "IBI for unregistered SMBus port %u\n", i);
> +			regmap_write(p3h2x4x_i3c_hub->regmap,
> +				     P3H2X4X_TP0_SMBUS_AGNT_STS + i,
> +				     BUF_RECEIVED_FLAG_TF_MASK);
> +			continue;
> +		}

[Severity: High]
Could there be a race condition with synchronous slave binding here?

When the I2C adapter is exposed to the system in p3h2x4x_tp_smbus_algo(),
a slave driver can bind synchronously and invoke p3h2x4x_tp_i2c_reg_slave().
This would enable IBI in hardware and set ibi_en to true.

However, right after i2c_add_adapter() returns, p3h2x4x_tp_smbus_algo()
overwrites ibi_en to false and sets is_registered to true without any locks:

drivers/i3c/hub/p3h2840_i3c_hub_smbus.c:p3h2x4x_tp_smbus_algo() {
    ...
    ret = i2c_add_adapter(smbus_adapter);
    if (ret) {
        p3h2x4x_unregister_smbus_adapters(hub);
        return ret;
    }

    hub->tp_bus[tp].is_registered = true;
    hub->hub_config.tp_config[tp].ibi_en = false;
    ...
}

If an IBI arrives during this tiny window, wouldn't the handler drop the data
because is_registered is still false? Also, won't the blind overwrite of
ibi_en to false corrupt the newly registered slave's state?

[ ... ]
> +static int p3h2x4x_tp_i2c_reg_slave(struct i2c_client *slave)
> +{
[ ... ]
> +	guard(mutex)(&hub->etx_mutex);
> +
> +	if (!hub->i3cdev || !hub->ibi_ready)
> +		return -EOPNOTSUPP;
> +
> +	if (bus->tp_smbus_client)
> +		return -EBUSY;
> +
> +	mutex_lock(&p3h2x4x->protected_reg_lock);
> +
> +	/* Unlock access to protected registers */
> +	ret = regmap_write(hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			   P3H2X4X_REGISTERS_UNLOCK_CODE);
> +	if (ret)
> +		goto out_unlock_mutex;
[ ... ]
> +out_unlock_mutex:
> +	mutex_unlock(&p3h2x4x->protected_reg_lock);
> +
> +	if (!ret) {
> +		bus->tp_smbus_client = slave;
> +		hub->hub_config.tp_config[bus->tp_port].ibi_en = true;
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
The cleanup subsystem guidelines suggest avoiding mixing goto-based error
handling with scope-based guard() cleanup in the same function. Could this
mix create confusing ownership semantics here?

> +
> +static int p3h2x4x_tp_i2c_unreg_slave(struct i2c_client *slave)
> +{
[ ... ]
> +	guard(mutex)(&hub->etx_mutex);
> +
> +	if (bus->tp_smbus_client != slave)
> +		return -EINVAL;
> +
> +	bus->tp_smbus_client = NULL;
> +	hub->hub_config.tp_config[bus->tp_port].ibi_en = false;
> +
> +	mutex_lock(&p3h2x4x->protected_reg_lock);
> +
> +	/* Unlock access to protected registers */
> +	ret = regmap_write(hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> +			   P3H2X4X_REGISTERS_UNLOCK_CODE);
> +	if (ret)
> +		goto out_unlock_mutex;
[ ... ]
> +out_unlock_mutex:
> +	mutex_unlock(&p3h2x4x->protected_reg_lock);
> +	return ret;
> +}

[Severity: Medium]
Similar to the registration function above, does the use of both guard(mutex)
and goto out_unlock_mutex in this function violate the cleanup guidelines?

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