[PATCH v2 1/2] hwmon: (max6621) fix temperature clamp range

[PATCH v2 1/2] hwmon: (max6621) fix temperature clamp range

[Cong Nguyen]

MAX6621_TEMP_INPUT_MIN and MAX6621_TEMP_INPUT_MAX are used to clamp the
writable offset and critical thresholds. They are defined as -127000 and
128000.

The driver decodes the temperature through an s8 and its own comment in
max6621_read() documents an 8-bit two's complement value, whose range is
-128 to +127 degrees C. The current limits therefore reject the valid
-128 degrees C and accept +128 degrees C, which does not fit the 8-bit
range.

Correct the limits to -128000 and 127000.

Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
Changes in v2:
 - Drop the temperature input change; it was not a bug (temp_input already
   sign-extends correctly via an s8 intermediate).
 - Drop the incorrect changelog reasoning (no "+128 degC", no PECI/16-bit).
 - Split into two patches per review: this one fixes the clamp range; 2/2
   fixes the negative offset/crit reads.
 - No 1/64 degC precision change (not documented in the datasheet).

 drivers/hwmon/max6621.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
index a7066f3a0bb4..ee5898fbe110 100644
--- a/drivers/hwmon/max6621.c
+++ b/drivers/hwmon/max6621.c
@@ -17,8 +17,8 @@
 
 #define MAX6621_DRV_NAME		"max6621"
 #define MAX6621_TEMP_INPUT_REG_NUM	9
-#define MAX6621_TEMP_INPUT_MIN		-127000
-#define MAX6621_TEMP_INPUT_MAX		128000
+#define MAX6621_TEMP_INPUT_MIN		-128000
+#define MAX6621_TEMP_INPUT_MAX		127000
 #define MAX6621_TEMP_ALERT_CHAN_SHIFT	1
 
 #define MAX6621_TEMP_S0D0_REG		0x00
-- 
2.25.1

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

[Cong Nguyen]

max6621_read() reads the CONFIG2 offset and the critical alert threshold
registers into a u32 and scales them without sign extension:

	/* offset */ *val = (regval >> MAX6621_REG_TEMP_SHIFT) * 1000L;
	/* crit   */ *val = regval * 1000L;

Both attributes are writable and their write paths clamp to a negative
minimum and encode negative values, so a value written as negative is read
back as a large positive number. For example, writing a -10 degrees C
offset stores max6621_temp_mc2reg(-10000) = (-10 << 6) = 0xfd80; the read
then computes 0xfd80 >> 6 = 1014 -> 1014000 instead of -10000.

Cast the register value to s16 before scaling so the read preserves the
sign the write path encodes. The temperature input path already uses an s8
intermediate and is left unchanged.

Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
 drivers/hwmon/max6621.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
index ee5898fbe110..3690f2ffe5fc 100644
--- a/drivers/hwmon/max6621.c
+++ b/drivers/hwmon/max6621.c
@@ -239,7 +239,7 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 			if (ret)
 				return ret;
 
-			*val = (regval >> MAX6621_REG_TEMP_SHIFT) *
+			*val = ((s16)regval >> MAX6621_REG_TEMP_SHIFT) *
 			       1000L;
 
 			break;
@@ -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:
-- 
2.25.1

Re: [PATCH v2 1/2] hwmon: (max6621) fix temperature clamp range

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Reading a channel-specific alarm attribute returns the global alert status and silently clears alerts for all channels.
--

--- Patch [1]: [PATCH v2 1/2] hwmon: (max6621) fix temperature clamp range ---
commit 6a78b24a06173620d98274b9f9d004f988e65833
Author: Cong Nguyen <congnt264@gmail.com>

hwmon: (max6621) fix temperature clamp range

MAX6621_TEMP_INPUT_MIN and MAX6621_TEMP_INPUT_MAX are used to clamp the
writable offset and critical thresholds. They are defined as -127000 and
128000. Correct the limits to -128000 and 127000.

Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>

> diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
> index e86ec6d237ca6..16a3a61d25326 100644
> --- a/drivers/hwmon/max6621.c
> +++ b/drivers/hwmon/max6621.c
> @@ -17,8 +17,8 @@
>  
>  #define MAX6621_DRV_NAME		"max6621"
>  #define MAX6621_TEMP_INPUT_REG_NUM	9
> -#define MAX6621_TEMP_INPUT_MIN		-127000
> -#define MAX6621_TEMP_INPUT_MAX		128000
> +#define MAX6621_TEMP_INPUT_MIN		-128000
> +#define MAX6621_TEMP_INPUT_MAX		127000

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
max6621_read() when handling hwmon_temp_crit_alarm:

drivers/hwmon/max6621.c:max6621_read() {
    ...
		case hwmon_temp_crit_alarm:
			...
			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;
    ...
}

Because MAX6621_TEMP_ALERT_CAUSE_REG is global, does reading a single
channel's alarm attribute incorrectly return the global alert status
regardless of the requested channel?

Furthermore, could the subsequent write to MAX6621_CLEAR_ALERT_REG silently
clear the alert state for all other channels?

Since unprivileged users can typically read these alarm attributes, would
this allow a local user to suppress system-wide thermal alarms by
continuously reading a single channel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/9d3a4f1895a47794bb359a2a32fb1ccd6a15812c.1786334038.git.congnt264@gmail.com?part=1

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

[sashiko-bot]

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 <congnt264@gmail.com>

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/9d3a4f1895a47794bb359a2a32fb1ccd6a15812c.1786334038.git.congnt264@gmail.com?part=2

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

[Guenter Roeck]

On 8/9/26 22:16, sashiko-bot@kernel.org 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 <congnt264@gmail.com>
> 
> 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

Re: [PATCH v2 1/2] hwmon: (max6621) fix temperature clamp range

[Guenter Roeck]

On Mon, Aug 10, 2026 at 11:27:54AM +0700, Cong Nguyen wrote:
> MAX6621_TEMP_INPUT_MIN and MAX6621_TEMP_INPUT_MAX are used to clamp the
> writable offset and critical thresholds. They are defined as -127000 and
> 128000.
> 
> The driver decodes the temperature through an s8 and its own comment in
> max6621_read() documents an 8-bit two's complement value, whose range is
> -128 to +127 degrees C. The current limits therefore reject the valid
> -128 degrees C and accept +128 degrees C, which does not fit the 8-bit
> range.
> 
> Correct the limits to -128000 and 127000.
> 
> Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4
> Signed-off-by: Cong Nguyen <congnt264@gmail.com>

Applied.

Thanks,
Guenter

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

[Guenter Roeck]

On Mon, Aug 10, 2026 at 11:28:39AM +0700, Cong Nguyen wrote:
> max6621_read() reads the CONFIG2 offset and the critical alert threshold
> registers into a u32 and scales them without sign extension:
> 
> 	/* offset */ *val = (regval >> MAX6621_REG_TEMP_SHIFT) * 1000L;
> 	/* crit   */ *val = regval * 1000L;
> 
> Both attributes are writable and their write paths clamp to a negative
> minimum and encode negative values, so a value written as negative is read
> back as a large positive number. For example, writing a -10 degrees C
> offset stores max6621_temp_mc2reg(-10000) = (-10 << 6) = 0xfd80; the read
> then computes 0xfd80 >> 6 = 1014 -> 1014000 instead of -10000.
> 
> Cast the register value to s16 before scaling so the read preserves the
> sign the write path encodes. The temperature input path already uses an s8
> intermediate and is left unchanged.
> 
> Fixes: 92b64580f14b ("hwmon: (max6621) Add support for Maxim MAX6621 temperature sensor")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4
> Signed-off-by: Cong Nguyen <congnt264@gmail.com>

Applied.

Thanks,
Guenter