Re: [PATCH v3 04/13] i3c: master: Support ACPI enumeration of child devices

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] `i3c_acpi_get_i2c_resource()` incorrectly overwrites device configuration and accumulates flags when multiple `I2cSerialBus` resources are present.
- [High] Auxiliary non-I2C/I3C ACPI child nodes cause the entire I3C master bus registration to fail.
- [High] The ACPI I2C enumeration path leaves `boardinfo->base.type` uninitialized, resulting in empty device names.
--

commit 32dd6acd0427ec4e092205396a3b4f2782c52903
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: Support ACPI enumeration of child devices

This commit adds support for describing I3C and I2C devices using ACPI.
It extracts device configuration such as the static address and LVR
from ACPI resources to enable enumeration on the I3C bus.

Link: https://www.mipi.org/mipi-disco-for-i3c-download

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2409,6 +2410,53 @@ EXPORT_SYMBOL_GPL(i3c_master_add_i3c_dev_locked);
>  
>  #define OF_I3C_REG1_IS_I2C_DEV			BIT(31)
>  
> +#ifdef CONFIG_ACPI
> +static int i3c_acpi_get_i2c_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct i2c_dev_boardinfo *boardinfo = data;
> +	struct acpi_resource_i2c_serialbus *sb;
> +
> +	if (!i2c_acpi_get_i2c_resource(ares, &sb))
> +		return 1;
> +
> +	boardinfo->base.addr = sb->slave_address;
> +	if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> +		boardinfo->base.flags |= I2C_CLIENT_TEN;
> +
> +	boardinfo->lvr = sb->lvr;
> +
> +	return 0;
> +}

Does this code overwrite the device configuration if multiple I2cSerialBus
resources are present? By returning 0, it tells the ACPI core to continue
walking resources. The primary address seems to be overwritten by the last
resource, and flags like I2C_CLIENT_TEN from earlier resources will
accumulate into the final configuration.

[ ... ]

> +static int i3c_acpi_add_i2c_boardinfo(struct i2c_dev_boardinfo *boardinfo,
> +				      struct fwnode_handle *fwnode)
> +{
> +	struct acpi_device *adev = to_acpi_device_node(fwnode);
> +	LIST_HEAD(resources);
> +	int ret;
> +
> +	boardinfo->base.fwnode = acpi_fwnode_handle(adev);
> +
> +	ret = acpi_dev_get_resources(adev, &resources,
> +				     i3c_acpi_get_i2c_resource, boardinfo);

Is boardinfo->base.type left uninitialized here? Because the structure is
allocated with devm_kzalloc() earlier, the type field remains an empty
string.

Standard I2C APIs rely on this field to populate the client name, which might
break sysfs node creation and legacy string-based driver matching.

[ ... ]

>  static int fwnode_populate_i3c_bus(struct i3c_master_controller *master)
>  {
>  	struct device *dev = &master->dev;
> @@ -2532,7 +2645,13 @@ static int fwnode_populate_i3c_bus(struct i3c_master_controller *master)
>  		return 0;
>  
>  	fwnode_for_each_available_child_node_scoped(fwnode, child) {
> -		ret = i3c_master_add_dev(master, child);
> +		if (is_of_node(child))
> +			ret = i3c_master_add_of_dev(master, child);
> +		else if (is_acpi_device_node(child))
> +			ret = i3c_master_add_acpi_dev(master, child);
> +		else
> +			continue;
> +
>  		if (ret)
>  			return ret;
>  	}

If an auxiliary ACPI child node lacks an _ADR method and lacks I2cSerialBus
resources, i3c_master_add_acpi_dev() calls i3c_master_add_i2c_boardinfo(),
which will eventually return -ENODEV.

Will this cause the entire I3C master bus registration to fail because the
loop aborts on the first error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423085718.70762-1-akhilrajeev@nvidia.com?part=4