Re: [PATCH v2] hwmon: Add support for currX_emergency and inX_[l]emergency attributes

On 8/13/26 18:20, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Internal ABI breakage in the habanalabs driver caused by inserting values into hwmon enums.
> --
> 
> --- Patch [1]: [PATCH v2] hwmon: Add support for currX_emergency and inX_[l]emergency attributes ---
> commit 618b70e656b029d918a4d90557dfadbd253324dd
> Author: Guenter Roeck <[email protected]>
> 
> hwmon: Add support for currX_emergency and inX_[l]emergency attributes
> 
> This commit adds new emergency limit attributes for current and voltage to
> the hardware monitoring subsystem. It updates the hwmon core code, the
> enumerations, and the sysfs ABI documentation to support the new alarms.
> 
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index dd713e193d0c3..a3a7d27f3b5ff 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -134,6 +134,8 @@ enum hwmon_in_attributes {
>>   	hwmon_in_max,
>>   	hwmon_in_lcrit,
>>   	hwmon_in_crit,
>> +	hwmon_in_lemergency,
>> +	hwmon_in_emergency,
> 
> [Severity: High]
> Does inserting these elements here break downstream drivers that rely on the
> historical integer mapping?
> 
> For example, in drivers/accel/habanalabs/common/hwmon.c:adjust_hwmon_flags(),
> the driver performs a hardcoded bitwise shift assuming the Linux hwmon enum
> values perfectly match the firmware API shifted by 1:
> 
>      flags = (cpucp_flags << 1) | HWMON_I_ENABLE;
> 
> By inserting elements into the middle of this enum, all subsequent values
> (like hwmon_in_average) shift by 2. This causes the habanalabs driver to
> incorrectly map firmware sensors to Linux hwmon attributes. For instance,
> inX_highest will map to inX_average, and reading from these incorrectly
> mapped attributes via sysfs will fail with -EINVAL.
> 
> Can these new enum values be added to the end of the enum instead, or does
> habanalabs need to be updated to decouple it from the hwmon enum ordering?
> 

There is no guarantee of a specific ordering of those flags. They can and will change
position and/or ordering as new flags are added. The hwmon subsystem must not be held
hostage by some driver making up their own rules. FWIW, I don't even understand
what the habanalabs driver is doing and what assumptions it is making about the position
and ordering of the flags.

Guenter