Re: [PATCH v2 2/2] hwmon: (max6621) fix negative temperature offset and crit readings

On 8/9/26 22:16, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Medium] The `hwmon_temp_crit_alarm` read path ignores the `channel` parameter and unconditionally clears global alerts, causing alarms to be misattributed and lost.
> --
> 
> --- Patch [2]: [PATCH v2 2/2] hwmon: (max6621) fix negative temperature offset and crit readings ---
> commit 0116e27b3257c1efaee364113dfc90d35d8fb869
> Author: Cong Nguyen <[email protected]>
> 
> hwmon: (max6621) fix negative temperature offset and crit readings
> 
> This commit fixes a bug where negative temperature offsets and critical alerts
> were read back as large positive numbers due to missing sign extension. By
> casting the register value to s16 before scaling, the read path now preserves
> the sign encoded during writes.
> 
>> diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
>> index 16a3a61d25326..1a644d7871431 100644
>> --- a/drivers/hwmon/max6621.c
>> +++ b/drivers/hwmon/max6621.c
> 
> [ ... ]
> 
>> @@ -254,7 +254,7 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>   			if (ret)
>>   				return ret;
>>   
>> -			*val = regval * 1000L;
>> +			*val = (s16)regval * 1000L;
>>   
>>   			break;
>>   		case hwmon_temp_crit_alarm:
>>   			/*
>>   			 * Set val to zero to recover the case, when reading
>>   			 * MAX6621_TEMP_ALERT_CAUSE_REG results in for example
>>   			 * MAX6621_ALERT_DIS. Reading will return with error,
>>   			 * but in such case alarm should be returned as 0.
>>   			 */
>>   			*val = 0;
>>   			ret = regmap_read(data->regmap,
>>   					  MAX6621_TEMP_ALERT_CAUSE_REG,
>>   					  &regval);
> 
> [ ... ]
> 
>>   			if (regval) {
>>   				ret = i2c_smbus_write_byte(data->client,
>>   						MAX6621_CLEAR_ALERT_REG);
>>   				if (ret)
>>   					return ret;
>>   			}
>>
>>   			*val = !!regval;
>>
>>   			break;
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this hwmon_temp_crit_alarm read path
> in max6621_read() ignore the channel parameter?
> 
> When userspace reads the tempX_crit_alarm sysfs attribute for any channel,
> this code reads the global MAX6621_TEMP_ALERT_CAUSE_REG and unconditionally
> clears it via MAX6621_CLEAR_ALERT_REG if an alert is active.
> 
> Could this result in misattributed alarms where a read on one channel
> incorrectly reports an alarm belonging to a different channel, and causes
> subsequent reads on other channels to miss their alarms because the global
> alert state was already cleared?
> 

Valid point, but the datasheet says:

The result is a 16-bit word (low byte transmitted first,
high byte second) that contains the register that
caused ALERT to assert. An error (8103h) is returned
when there is no active ALERT.

Since we don't know what "contains the register that caused ALERT to assert"
actually means (in other words, what registers are returned), we can not fix
the problem without access to an evaluation board or a board using this chip.

Guenter