Re: [PATCH v3 1/2] hwmon: (pmbus/ina233) Fix error handling and sign extension in shunt voltage read

[Guenter Roeck]

On Sun, Mar 22, 2026 at 07:20:43AM -0700, Guenter Roeck wrote:
> On Thu, Mar 19, 2026 at 05:31:19PM +0000, Pradhan, Sanman wrote:
> > From: Sanman Pradhan <psanman@xxxxxxxxxxx>
> > 
> > ina233_read_word_data() reads MFR_READ_VSHUNT via pmbus_read_word_data()
> > but has two issues:
> > 
> > 1. The return value is not checked for errors before being used in
> >    arithmetic. A negative error code from a failed I2C transaction is
> >    passed directly to DIV_ROUND_CLOSEST(), producing garbage data.
> > 
> > 2. MFR_READ_VSHUNT is a 16-bit two's complement value. Negative shunt
> >    voltages (values with bit 15 set) are treated as large positive
> >    values since pmbus_read_word_data() returns them zero-extended in an
> >    int. This leads to incorrect scaling in the VIN coefficient
> >    conversion.
> > 
> > Fix both issues by adding an error check, casting to s16 for proper
> > sign extension, and clamping the result to a valid non-negative range.
> > The clamp is necessary because read_word_data callbacks must return
> > non-negative values on success (negative values indicate errors to the
> > pmbus core).
> > 
> > Fixes: b64b6cb163f16 ("hwmon: Add driver for TI INA233 Current and Power Monitor")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Sanman Pradhan <psanman@xxxxxxxxxxx>
> > ---
> >  drivers/hwmon/pmbus/ina233.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
> > index dde1e16783943..1f7170372f243 100644
> > --- a/drivers/hwmon/pmbus/ina233.c
> > +++ b/drivers/hwmon/pmbus/ina233.c
> > @@ -67,10 +67,13 @@ static int ina233_read_word_data(struct i2c_client *client, int page,
> >  	switch (reg) {
> >  	case PMBUS_VIRT_READ_VMON:
> >  		ret = pmbus_read_word_data(client, 0, 0xff, MFR_READ_VSHUNT);
> > +		if (ret < 0)
> > +			return ret;
> >  
> >  		/* Adjust returned value to match VIN coefficients */
> >  		/* VIN: 1.25 mV VSHUNT: 2.5 uV LSB */
> > -		ret = DIV_ROUND_CLOSEST(ret * 25, 12500);
> > +		ret = clamp_val(DIV_ROUND_CLOSEST((s16)ret * 25, 12500),
> > +				0, 0x7FFF);
> 
> The clamp should be to 0xffff, not 0x7fff. That is still a positive return
> value, but does not drop the sign bit (bit 15).
> 
Never mind though, I'll fix that up myself.

Applied.

Thanks,
Guenter