Re: [PATCH v4 4/4] regulator: qcom-rpmh: Add debounce delay before disabling regulator

[Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>]

On 10/1/2026 6:35 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] regulator: qcom-rpmh: Use-After-Free due to devres LIFO ordering
> - [High] regulator: qcom-rpmh: Data race in worker thread bypassing regulator_lock
> - [High] regulator: qcom-rpmh: NULL pointer dereference if delayed disable fires before init completes
> - [Medium] regulator: qcom-rpmh: State desynchronization on driver removal drops pending disable
> --
> 
> commit b0211424f1ad79c3af4f67d5c03d4546a1bd442e
> Author: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> 
> regulator: qcom-rpmh: Add debounce delay before disabling regulator
> 
> This commit adds a debounce delay to the Qualcomm RPMh regulator driver to
> prevent rapid disable/enable cycles. It introduces a delayed work item that
> defers sending disable requests to the hardware. A devres action is also
> added to cancel the delayed work upon driver removal.
> 
>> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
>> --- a/drivers/regulator/qcom-rpmh-regulator.c
>> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> 
> [ ... ]
> 
>> @@ -347 +347,54 @@ static int rpmh_regulator_set_enable_state(struct regulator_dev *rdev,
>>  	return ret;
>>  }
>>  
>> +static void rpmh_delayed_off_work(struct work_struct *work)
>> +{
>> +	struct rpmh_vreg *vreg = container_of(work,
>> +					struct rpmh_vreg, delayed_off_work.work);
>> +
>> +	if (!_rpmh_regulator_set_enable_state(vreg, vreg->rdev, false))
> 
> [Severity: High]
> Does this worker thread modify the hardware state without holding the
> regulator core lock?
> 
> The core consumer APIs execute under the regulator_lock. Running
> _rpmh_regulator_set_enable_state() here asynchronously might cause a data
> race on shared structures like vreg->enabled and vreg->voltage_selector if a
> consumer calls regulator_set_voltage() concurrently.

I checked the driver again and noticed that the check for
(vreg->enabled == -EINVAL && vreg->voltage_selector != -ENOTRECOVERABLE)
in rpmh_regulator_set_enable_state() is only needed to cover the
case where there was a voltage vote before the first enable vote
coming after the regulator's probe. After this first enable vote,
the value of vreg->enabled would always be 0 or 1 and this check
would never pass, so it can be ignored for concurrency concerns
with rpmh_regulator_vrm_set_voltage_sel() which checks
vreg->enabled.

The other comments are valid and I will address them in v5.

Thanks,
Jishnu

> 
>> +		vreg->last_off = ktime_get_boottime();
>> +}
>