Re: [PATCH v5 1/4] hwmon: (pmbus) add missing mutex_lock in regulator ops

[Guenter Roeck]

On 3/20/26 17:52, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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