From: Sanman Pradhan <psanman@juniper.net>

The regulator voltage operations pmbus_regulator_get_voltage(),
pmbus_regulator_set_voltage(), and pmbus_regulator_list_voltage()
call PMBus access helpers without holding update_lock. These helpers
perform multi-step PMBus transactions involving shared core state such
as page selection and transaction timing. Without serialization, a
concurrent PMBus access can interleave with those operations and cause
reads from or writes to the wrong rail.

For set_voltage(), this is particularly dangerous because the
VOUT_COMMAND write could be directed to the wrong regulator output.

Add mutex_lock/unlock around the affected regulator voltage paths,
following the pattern already used by other PMBus regulator operations
such as _pmbus_regulator_on_off() and pmbus_regulator_get_status().

Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
v5:
- New patch in the series. Adds the missing update_lock mutex to the
  three regulator voltage ops that were missing serialization.
---
 drivers/hwmon/pmbus/pmbus_core.c | 46 ++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 4d7634ee61484..3dad455448d05 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3181,7 +3181,9 @@ static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
 		.convert = true,
 	};
 
+	mutex_lock(&data->update_lock);
 	s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_READ_VOUT);
+	mutex_unlock(&data->update_lock);
 	if (s.data < 0)
 		return s.data;
 
@@ -3202,16 +3204,23 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
 	};
 	int val = DIV_ROUND_CLOSEST(min_uv, 1000); /* convert to mV */
 	int low, high;
+	int ret;
 
 	*selector = 0;
 
+	mutex_lock(&data->update_lock);
+
 	low = pmbus_regulator_get_low_margin(client, s.page);
-	if (low < 0)
-		return low;
+	if (low < 0) {
+		ret = low;
+		goto unlock;
+	}
 
 	high = pmbus_regulator_get_high_margin(client, s.page);
-	if (high < 0)
-		return high;
+	if (high < 0) {
+		ret = high;
+		goto unlock;
+	}
 
 	/* Make sure we are within margins */
 	if (low > val)
@@ -3221,7 +3230,11 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
 
 	val = pmbus_data2reg(data, &s, val);
 
-	return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
+	ret = _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
+
+unlock:
+	mutex_unlock(&data->update_lock);
+	return ret;
 }
 
 static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
@@ -3231,6 +3244,7 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
 	struct i2c_client *client = to_i2c_client(dev->parent);
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	int val, low, high;
+	int ret;
 
 	if (data->flags & PMBUS_VOUT_PROTECTED)
 		return 0;
@@ -3243,18 +3257,28 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
 	val = DIV_ROUND_CLOSEST(rdev->desc->min_uV +
 				(rdev->desc->uV_step * selector), 1000); /* convert to mV */
 
+	mutex_lock(&data->update_lock);
+
 	low = pmbus_regulator_get_low_margin(client, rdev_get_id(rdev));
-	if (low < 0)
-		return low;
+	if (low < 0) {
+		ret = low;
+		goto unlock;
+	}
 
 	high = pmbus_regulator_get_high_margin(client, rdev_get_id(rdev));
-	if (high < 0)
-		return high;
+	if (high < 0) {
+		ret = high;
+		goto unlock;
+	}
 
 	if (val >= low && val <= high)
-		return val * 1000; /* unit is uV */
+		ret = val * 1000; /* unit is uV */
+	else
+		ret = 0;
 
-	return 0;
+unlock:
+	mutex_unlock(&data->update_lock);
+	return ret;
 }
 
 const struct regulator_ops pmbus_regulator_ops = {
-- 
2.34.1

On 3/20/26 17:52, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> The regulator voltage operations pmbus_regulator_get_voltage(),
> pmbus_regulator_set_voltage(), and pmbus_regulator_list_voltage()
> call PMBus access helpers without holding update_lock. These helpers
> perform multi-step PMBus transactions involving shared core state such
> as page selection and transaction timing. Without serialization, a
> concurrent PMBus access can interleave with those operations and cause
> reads from or writes to the wrong rail.
> 
> For set_voltage(), this is particularly dangerous because the
> VOUT_COMMAND write could be directed to the wrong regulator output.
> 
> Add mutex_lock/unlock around the affected regulator voltage paths,
> following the pattern already used by other PMBus regulator operations
> such as _pmbus_regulator_on_off() and pmbus_regulator_get_status().
> 
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
> v5:
> - New patch in the series. Adds the missing update_lock mutex to the
>    three regulator voltage ops that were missing serialization.
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 46 ++++++++++++++++++++++++--------
>   1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 4d7634ee61484..3dad455448d05 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -3181,7 +3181,9 @@ static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
>   		.convert = true,
>   	};
>   
> +	mutex_lock(&data->update_lock);
>   	s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_READ_VOUT);
> +	mutex_unlock(&data->update_lock);

I guess that would have been too easy. Here is the AI review feedback:

https://sashiko.dev/#/patchset/20260321005206.6350-1-sanman.pradhan%40hpe.com

Apparently this causes a lock recursion. We'll have to make sure
that regulator_notifier_call_chain() is called outside the pmbus lock.

The only way I can imagine this to work would be with a worker.
Instead of calling pmbus_regulator_notify() directly from
pmbus_fault_handler(), we could store pending events in struct
pmbus_data (one event per page) and trigger a worker. The worker
would then walk through the event list and call pmbus_regulator_notify()
outside the lock for each pending event.

Of course it isn't that simple because the list of events can be updated
while the worker is running, but that should be solvable.

Either case, I am not sure if we want  / need to handle this with the
current patch series. The other patches look ok and should be ready to apply.
WDYT ?

Thanks,
Guenter

From: Sanman Pradhan <psanman@juniper.net>

Thanks for the review, that makes sense.

Agreed, Patch 1 should be dropped from this series for now rather than
trying to solve the notifier/locking interaction here.

I'll resend the remaining patches as a v6 3-patch series:
- export pmbus_wait() and pmbus_update_ts()
- max31785: use access_delay for PMBus-mediated accesses
- max31785: check for partial i2c_transfer() in read_long_data()

Thank you.

Regards,
Sanman Pradhan