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

[sashiko-bot@kernel.org]

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.

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

[ ... ]

>  static int rpmh_regulator_disable(struct regulator_dev *rdev)
>  {
> +	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> +	if (vreg->debounce_delay) {
> +		queue_delayed_work(system_percpu_wq, &vreg->delayed_off_work,
> +				   usecs_to_jiffies(vreg->debounce_delay));
> +		return 0;
> +	}

[Severity: High]
Can this lead to a use-after-free during driver teardown?

When the driver unbinds, devres destroys resources in LIFO order.
The custom devres action rpmh_regulator_cancel_delayed_off_work runs first,
canceling the delayed work. Then devm_regulator_release() runs, which calls
regulator_unregister().

If regulator_unregister() flushes pending core disables, it invokes
rpmh_regulator_disable() which re-queues the delayed work here. After devres
finishes freeing vreg, the newly queued work would execute and dereference
the freed memory.

> +
>  	return rpmh_regulator_set_enable_state(rdev, false);
>  }

[ ... ]

> @@ -551 +551,13 @@ static const struct regulator_ops rpmh_regulator_xob_ops = {
>  	.is_enabled		= rpmh_regulator_is_enabled,
>  };
>  
> +static void rpmh_regulator_cancel_delayed_off_work(void *data)
> +{
> +	struct rpmh_vreg *vreg = data;
> +
> +	cancel_delayed_work_sync(&vreg->delayed_off_work);
> +}

[Severity: Medium]
Does cancelling the work here without syncing the hardware state leave the
regulator permanently enabled if a disable request was pending?

The regulator core would consider it disabled, leading to leaked power and
incorrect hardware state.

> +
>  /**
>   * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator

[ ... ]

> @@ -677 +677,14 @@ static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
>  			node, ret);
>  		return ret;
>  	}
> +	vreg->rdev = rdev;

[Severity: High]
Could vreg->rdev be NULL if the delayed disable worker fires too early?

The regulator is exposed to consumers as soon as devm_regulator_register()
returns. If a fast consumer probes, sets a voltage, and immediately disables
the regulator before vreg->rdev is assigned here, the delayed off worker
might execute with vreg->rdev set to NULL.

This would cause a NULL pointer dereference when
_rpmh_regulator_set_enable_state() attempts to set the voltage and passes
the NULL rdev to rdev_get_drvdata().

> +
> +	if (vreg->debounce_delay) {
> +		ret = devm_add_action_or_reset(dev, rpmh_regulator_cancel_delayed_off_work,
> +					       vreg);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	dev_dbg(dev, "%pOFn regulator registered for RPMh resource %s @ 0x%05X\n",
>  		node, rpmh_resource_name, vreg->addr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-regulator-off-on-delay-v4-0-258ba9612da8@oss.qualcomm.com?part=4