hwmon: (pmbus) Follow-up fixes for isl68137 and ina233

[PATCH v2 2/2] hwmon: (pmbus/ina233) Handle sign extension for negative shunt voltage

Posted by Pradhan, Sanman 2 weeks, 4 days ago

From: Sanman Pradhan <psanman@juniper.net>

ina233_read_word_data() reads MFR_READ_VSHUNT, which is a 16-bit
two's complement value. Because pmbus_read_word_data() returns an
integer, negative voltages (values > 32767) are currently treated as
large positive values, leading to incorrect scaling in DIV_ROUND_CLOSEST().

Add a cast to (s16) to ensure negative shunt voltages are correctly
sign-extended before the scaling calculation is performed.

Fixes: b64b6cb163f16 ("hwmon: Add driver for TI INA233 Current and Power Monitor")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v2:
  - Added (s16) cast to fix sign-extension for negative shunt voltages,
    complementing the error check fix applied in v1
---
 drivers/hwmon/pmbus/ina233.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
index dde1e16783943..819f4e8aeab61 100644
--- a/drivers/hwmon/pmbus/ina233.c
+++ b/drivers/hwmon/pmbus/ina233.c
@@ -70,7 +70,7 @@ static int ina233_read_word_data(struct i2c_client *client, int page,
 
 		/* Adjust returned value to match VIN coefficients */
 		/* VIN: 1.25 mV VSHUNT: 2.5 uV LSB */
-		ret = DIV_ROUND_CLOSEST(ret * 25, 12500);
+		ret = DIV_ROUND_CLOSEST((s16)ret * 25, 12500);
 		break;
 	default:
 		ret = -ENODATA;
-- 
2.34.1

Re: [PATCH v2 2/2] hwmon: (pmbus/ina233) Handle sign extension for negative shunt voltage

Posted by Guenter Roeck 2 weeks, 4 days ago

On 3/18/26 12:40, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> ina233_read_word_data() reads MFR_READ_VSHUNT, which is a 16-bit
> two's complement value. Because pmbus_read_word_data() returns an
> integer, negative voltages (values > 32767) are currently treated as
> large positive values, leading to incorrect scaling in DIV_ROUND_CLOSEST().
> 
> Add a cast to (s16) to ensure negative shunt voltages are correctly
> sign-extended before the scaling calculation is performed.
> 
> Fixes: b64b6cb163f16 ("hwmon: Add driver for TI INA233 Current and Power Monitor")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
> v2:
>    - Added (s16) cast to fix sign-extension for negative shunt voltages,
>      complementing the error check fix applied in v1
> ---
>   drivers/hwmon/pmbus/ina233.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
> index dde1e16783943..819f4e8aeab61 100644
> --- a/drivers/hwmon/pmbus/ina233.c
> +++ b/drivers/hwmon/pmbus/ina233.c
> @@ -70,7 +70,7 @@ static int ina233_read_word_data(struct i2c_client *client, int page,
>   
>   		/* Adjust returned value to match VIN coefficients */
>   		/* VIN: 1.25 mV VSHUNT: 2.5 uV LSB */
> -		ret = DIV_ROUND_CLOSEST(ret * 25, 12500);
> +		ret = DIV_ROUND_CLOSEST((s16)ret * 25, 12500);

This may end up reporting a negative error value to the caller.
Should the result be masked against 0xffff ?

Thanks,
Guenter

Re: [PATCH v2 2/2] hwmon: (pmbus/ina233) Handle sign extension for negative shunt voltage

Posted by Guenter Roeck 2 weeks, 4 days ago

On 3/18/26 13:20, Guenter Roeck wrote:
> On 3/18/26 12:40, Pradhan, Sanman wrote:
>> From: Sanman Pradhan <psanman@juniper.net>
>>
>> ina233_read_word_data() reads MFR_READ_VSHUNT, which is a 16-bit
>> two's complement value. Because pmbus_read_word_data() returns an
>> integer, negative voltages (values > 32767) are currently treated as
>> large positive values, leading to incorrect scaling in DIV_ROUND_CLOSEST().
>>
>> Add a cast to (s16) to ensure negative shunt voltages are correctly
>> sign-extended before the scaling calculation is performed.
>>
>> Fixes: b64b6cb163f16 ("hwmon: Add driver for TI INA233 Current and Power Monitor")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
>> ---
>> v2:
>>    - Added (s16) cast to fix sign-extension for negative shunt voltages,
>>      complementing the error check fix applied in v1
>> ---
>>   drivers/hwmon/pmbus/ina233.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
>> index dde1e16783943..819f4e8aeab61 100644
>> --- a/drivers/hwmon/pmbus/ina233.c
>> +++ b/drivers/hwmon/pmbus/ina233.c
>> @@ -70,7 +70,7 @@ static int ina233_read_word_data(struct i2c_client *client, int page,
>>           /* Adjust returned value to match VIN coefficients */
>>           /* VIN: 1.25 mV VSHUNT: 2.5 uV LSB */
>> -        ret = DIV_ROUND_CLOSEST(ret * 25, 12500);
>> +        ret = DIV_ROUND_CLOSEST((s16)ret * 25, 12500);
> 
> This may end up reporting a negative error value to the caller.
> Should the result be masked against 0xffff ?
> 

Also, as Sashiko reports, "ret" can be a negative error code which needs
to be checked and handled first. And the mask would be wrong - I think it
needs a clamp_val().

Thanks,
Guenter