Re: [PATCH v3 12/13] hwmon: spd5118: Add I3C support

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] NVMEM reads over I3C return corrupted data due to an unhandled 64-byte block rollover.
- [High] Stack-allocated buffers are passed to `regmap_bulk_read()`, which is unsafe for DMA-capable I3C controllers and will cause kernel panics or memory corruption.
- [Low] The driver can be selected when `CONFIG_I2C` is disabled, leading to a Kconfig unmet direct dependency warning and a compilation failure due to undefined I2C APIs.
- [Low] Building the driver when `CONFIG_I3C` is disabled results in a linker error for `__devm_regmap_init_i3c`.
--

commit 4b70b869fa4332fdf133591e152cc997f3522e25
Author: Akhil R <akhilrajeev@nvidia.com>

hwmon: spd5118: Add I3C support
    
Add a regmap config and a probe function to support I3C-based
communication with SPD5118 devices.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 14e4cea48acc47..a7f60f8868eb6b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2309,12 +2309,15 @@ config SENSORS_INA3221
>  
>  config SENSORS_SPD5118
>  	tristate "SPD5118 Compliant Temperature Sensors"
> -	depends on I2C
> +	depends on I3C_OR_I2C
>  	select REGMAP_I2C

If CONFIG_I2C is disabled and CONFIG_I3C is enabled as a module, could this
cause Kconfig unmet direct dependency warnings?

Because select REGMAP_I2C strictly depends on I2C, it will result in warnings.
Compilation might also fail when module_i3c_i2c_driver() unconditionally
references i2c_add_driver(), which is undefined when CONFIG_I2C is not set.

> +	select REGMAP_I3C if I3C

[ ... ]

> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 8fdde35e68cd3f..db66028d2f590e 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -464,6 +465,27 @@ static const struct regmap_config spd5118_regmap8_config = {
>  	.num_ranges = ARRAY_SIZE(spd5118_i2c_regmap_range_cfg),
>  };
>  
> +/*
> + * SPD5118 2-byte register address format (JESD300-5, Tables 7 & 20):
> + *   Byte 1 (on wire first):  MemReg | BlkAddr[0] | Address[5:0]
> + *   Byte 2 (on wire second): 0000   | BlkAddr[4:1]
> + *
> + * The address byte (with MemReg and lower address bits) must be sent first,
> + * followed by the upper block address byte. With regmap 16-bit register
> + * format, this maps to little-endian: the low byte of the 16-bit value is
> + * transmitted first. No range config is needed since I3C does not use MR11
> + * page switching.
> + */
> +static const struct regmap_config spd5118_regmap_i3c_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = 0x7ff,

Since the 2-byte address format specifies only a 6-bit Address field, does the
internal address counter roll over at the 64-byte boundary during I3C mode?

If spd5118_nvmem_read_page() executes a 128-byte regmap_bulk_read(), it
appears the device will silently wrap around to the beginning of the same
64-byte block, returning repeated and incorrect data instead of advancing to
the next block.

> +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.writeable_reg = spd5118_writeable_reg,
> +	.volatile_reg = spd5118_volatile_reg,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +

[ ... ]

> @@ -701,7 +723,39 @@ static struct i2c_driver spd5118_i2c_driver = {
>  	.address_list	= IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? normal_i2c : NULL,
>  };
>  
> -module_i2c_driver(spd5118_i2c_driver);
> +/* I3C */
> +
> +static int spd5118_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	struct device *dev = i3cdev_to_dev(i3cdev);
> +	struct regmap *regmap;
> +	u8 regval[2];
> +	int err;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &spd5118_regmap_i3c_config);

If CONFIG_I3C is disabled, will this unconditionally call
devm_regmap_init_i3c() and result in a linker error?

When CONFIG_I3C is disabled, REGMAP_I3C is not selected, and
__devm_regmap_init_i3c is only exported by regmap-i3c.c when REGMAP_I3C is
enabled.

> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
> +
> +	err = regmap_bulk_read(regmap, SPD5118_REG_TYPE, regval, 2);

Is it safe to pass the stack-allocated buffer regval to regmap_bulk_read()?

Because val_bits = 8 uses regmap_parse_inplace_noop, regmap_bulk_read() avoids
bounce buffers and passes the user buffer directly down to regmap_i3c_read()
and i3c_device_do_xfers().

Passing stack memory to DMA-capable APIs can lead to memory corruption or
kernel panics.

> +	if (err)
> +		return err;
> +
> +	if (regval[0] != 0x51 || regval[1] != 0x18)
> +		return -ENODEV;
> +
> +	return spd5118_common_probe(dev, regmap);
> +}

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