Re: [PATCH v2 1/6] hwmon/misc: amd-sbi: Move core sbtsi support from hwmon to misc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Auxiliary device ID collision prevents probe of multiple SB-TSI sensors
- [Medium] Kconfig dependency change causes silent loss of the hwmon driver on kernel upgrades
--

commit fe75a41fdaa2ec32654699f9128ef6e13b7c7140
Author: Prathima <Prathima.Lk@amd.com>

hwmon/misc: amd-sbi: Move core sbtsi support from hwmon to misc

This commit moves the core Side-Band Temperature Sensor Interface (SBTSI)
logic into drivers/misc/amd-sbi/tsi.c. It registers an auxiliary device per
target so that the hwmon driver can bind to it while allowing other consumers
to reuse the core functionality.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 14e4cea48acc4..6fa51e6ef6ff4 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1939,7 +1939,7 @@ config SENSORS_SL28CPLD
>  
>  config SENSORS_SBTSI
>  	tristate "Emulated SB-TSI temperature sensor"
> -	depends on I2C
> +	depends on AMD_SBTSI

Is there a risk that this dependency change could silently disable the
hwmon driver on kernel upgrades?

Because CONFIG_AMD_SBTSI has no default value, an unattended configuration
update like make olddefconfig might drop CONFIG_SENSORS_SBTSI entirely due
to unmet dependencies.

Could CONFIG_AMD_SBTSI be selected by CONFIG_SENSORS_SBTSI instead, or use
a default strategy to ensure existing user configurations are preserved?

[ ... ]

> diff --git a/drivers/misc/amd-sbi/tsi.c b/drivers/misc/amd-sbi/tsi.c
> new file mode 100644
> index 0000000000000..ee22167855505
> --- /dev/null
> +++ b/drivers/misc/amd-sbi/tsi.c

[ ... ]

> +static int sbtsi_create_hwmon_adev(struct device *dev, u8 dev_addr)
> +{
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	adev = kzalloc_obj(*adev);
> +	if (!adev)
> +		return -ENOMEM;
> +
> +	adev->name = AMD_SBTSI_AUX_HWMON;
> +	adev->id = dev_addr;

Could this assignment to adev->id cause a collision on multi-socket AMD
systems?

It looks like auxiliary device names are constructed globally as
modname.name.id by __auxiliary_device_add().

If multi-socket systems have identical sensors at the same I2C address
(for example, 0x4c) on different I2C adapters, probing the second sensor
might result in a duplicate sysfs name and cause device_add() to fail with
-EEXIST.

Should the driver allocate a globally unique instance ID for each
auxiliary device using an IDA, rather than relying solely on the I2C
address?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515134506.397649-1-Akshay.Gupta@amd.com?part=1