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

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

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

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@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
 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);
 		break;
 	default:
 		ret = -ENODATA;
-- 
2.34.1

[PATCH v3 2/2] hwmon: (pmbus/isl68137) Add mutex protection for AVS enable sysfs attributes

[Pradhan, Sanman]

From: Sanman Pradhan <psanman@juniper.net>

The custom avs0_enable and avs1_enable sysfs attributes access PMBus
registers through the exported API helpers (pmbus_read_byte_data,
pmbus_read_word_data, pmbus_write_word_data, pmbus_update_byte_data)
without holding the PMBus update_lock mutex. These exported helpers do
not acquire the mutex internally, unlike the core's internal callers
which hold the lock before invoking them.

The store callback is especially vulnerable: it performs a multi-step
read-modify-write sequence (read VOUT_COMMAND, write VOUT_COMMAND, then
update OPERATION) where concurrent access from another thread could
interleave and corrupt the register state.

Add pmbus_lock_interruptible()/pmbus_unlock() around both the show and
store callbacks to serialize PMBus register access with the rest of the
driver.

Fixes: 038a9c3d1e424 ("hwmon: (pmbus/isl68137) Add driver for Intersil ISL68137 PWM Controller")
Cc: stable@vger.kernel.org
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
 drivers/hwmon/pmbus/isl68137.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
index f42b13fe9fc18..48059ac4a08be 100644
--- a/drivers/hwmon/pmbus/isl68137.c
+++ b/drivers/hwmon/pmbus/isl68137.c
@@ -94,7 +94,15 @@ static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
 					     int page,
 					     char *buf)
 {
-	int val = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	int val;
+
+	val = pmbus_lock_interruptible(client);
+	if (val)
+		return val;
+
+	val = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+
+	pmbus_unlock(client);
 
 	if (val < 0)
 		return val;
@@ -116,6 +124,10 @@ static ssize_t isl68137_avs_enable_store_page(struct i2c_client *client,
 
 	op_val = result ? ISL68137_VOUT_AVS : 0;
 
+	rc = pmbus_lock_interruptible(client);
+	if (rc)
+		return rc;
+
 	/*
 	 * Writes to VOUT setpoint over AVSBus will persist after the VRM is
 	 * switched to PMBus control. Switching back to AVSBus control
@@ -127,17 +139,20 @@ static ssize_t isl68137_avs_enable_store_page(struct i2c_client *client,
 		rc = pmbus_read_word_data(client, page, 0xff,
 					  PMBUS_VOUT_COMMAND);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 
 		rc = pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
 					   rc);
 		if (rc < 0)
-			return rc;
+			goto unlock;
 	}
 
 	rc = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
 				    ISL68137_VOUT_AVS, op_val);
 
+unlock:
+	pmbus_unlock(client);
+
 	return (rc < 0) ? rc : count;
 }
 
-- 
2.34.1

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

[Guenter Roeck]

On Thu, Mar 19, 2026 at 05:31:19PM +0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> 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@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
>  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).

Thanks,
Guenter

>  		break;
>  	default:
>  		ret = -ENODATA;

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@juniper.net>
> > 
> > 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@vger.kernel.org
> > Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> > ---
> >  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

Re: [PATCH v3 2/2] hwmon: (pmbus/isl68137) Add mutex protection for AVS enable sysfs attributes

[Guenter Roeck]

On Thu, Mar 19, 2026 at 05:31:29PM +0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> The custom avs0_enable and avs1_enable sysfs attributes access PMBus
> registers through the exported API helpers (pmbus_read_byte_data,
> pmbus_read_word_data, pmbus_write_word_data, pmbus_update_byte_data)
> without holding the PMBus update_lock mutex. These exported helpers do
> not acquire the mutex internally, unlike the core's internal callers
> which hold the lock before invoking them.
> 
> The store callback is especially vulnerable: it performs a multi-step
> read-modify-write sequence (read VOUT_COMMAND, write VOUT_COMMAND, then
> update OPERATION) where concurrent access from another thread could
> interleave and corrupt the register state.
> 
> Add pmbus_lock_interruptible()/pmbus_unlock() around both the show and
> store callbacks to serialize PMBus register access with the rest of the
> driver.
> 
> Fixes: 038a9c3d1e424 ("hwmon: (pmbus/isl68137) Add driver for Intersil ISL68137 PWM Controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/isl68137.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
> index f42b13fe9fc18..48059ac4a08be 100644
> --- a/drivers/hwmon/pmbus/isl68137.c
> +++ b/drivers/hwmon/pmbus/isl68137.c
> @@ -94,7 +94,15 @@ static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
>  					     int page,
>  					     char *buf)
>  {
> -	int val = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +	int val;
> +
> +	val = pmbus_lock_interruptible(client);
> +	if (val)
> +		return val;
> +
> +	val = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +
> +	pmbus_unlock(client);
>  
>  	if (val < 0)
>  		return val;
> @@ -116,6 +124,10 @@ static ssize_t isl68137_avs_enable_store_page(struct i2c_client *client,
>  
>  	op_val = result ? ISL68137_VOUT_AVS : 0;
>  
> +	rc = pmbus_lock_interruptible(client);
> +	if (rc)
> +		return rc;
> +
>  	/*
>  	 * Writes to VOUT setpoint over AVSBus will persist after the VRM is
>  	 * switched to PMBus control. Switching back to AVSBus control
> @@ -127,17 +139,20 @@ static ssize_t isl68137_avs_enable_store_page(struct i2c_client *client,
>  		rc = pmbus_read_word_data(client, page, 0xff,
>  					  PMBUS_VOUT_COMMAND);
>  		if (rc < 0)
> -			return rc;
> +			goto unlock;
>  
>  		rc = pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
>  					   rc);
>  		if (rc < 0)
> -			return rc;
> +			goto unlock;
>  	}
>  
>  	rc = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
>  				    ISL68137_VOUT_AVS, op_val);
>  
> +unlock:
> +	pmbus_unlock(client);
> +
>  	return (rc < 0) ? rc : count;
>  }
>  

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

[Pradhan, Sanman]

Thank you for catching that. That's right, clamping to 0xFFFF 
is the correct approach. Appreciate the fix-up and 
applying the series.

Thank you.

Regards,
Sanman Pradhan