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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Sanman Pradhan <psanman@xxxxxxxxxxx>

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;
>  }
>