Re: [PATCH 10/11] regulator: db8500: Add power domain regulators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `db8500_regulator_disable` callback incorrectly treats a return value of 1 from `pm_runtime_put_sync_suspend` as an error, causing early abort and state desynchronization.
- [High] Mismatched Genpd APIs (`of_genpd_add_device` paired with `dev_pm_domain_detach`) lead to a silent detach failure and Use-After-Free list corruption.
- [High] Embedding a `struct device` in a devres-managed structure (`devm_kzalloc`) with an empty release function causes a lifecycle violation and Use-After-Free.
- [High] The virtual device is never fully registered with `device_add()`, leading to a memory leak of PM QoS constraints.
- [High] The driver saves an OF node pointer to the persistent device structure without incrementing its reference count, risking a dangling pointer.
--

commit abd72295748300bd01e4a7216b2321ff24b1a38f
Author: Linus Walleij <linusw@kernel.org>

regulator: db8500: Add power domain regulators

Add a DB8500 regulator driver for the VAPE and VSMPS2 compatibility nodes,
backing the regulator enable state with corresponding power domains.

> diff --git a/drivers/regulator/db8500-regulator.c b/drivers/regulator/db8500-regulator.c
> new file mode 100644
> index 0000000000000..c5a9a1baaf8ea
> --- /dev/null
> +++ b/drivers/regulator/db8500-regulator.c
[ ... ]
> +static int db8500_regulator_disable(struct regulator_dev *rdev)
> +{
> +	struct db8500_regulator_info *info = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	ret = pm_runtime_put_sync_suspend(&info->pd_dev);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this propagate a success status as an error?

According to the PM subsystem rules, pm_runtime_put_sync_suspend() returns 1
when the device is already in the RPM_SUSPENDED state. Since this checks
if (ret) and returns it, won't the regulator core treat this as a failure,
aborting the disable operation and leaving the regulator's use_count and
enable_count out of sync?

[ ... ]
> +static void db8500_regulator_release(struct device *dev)
> +{
> +}

[Severity: High]
Does this empty release function circumvent kobject lifecycle tracking?

The struct device is embedded in devres memory allocated via devm_kzalloc().
Since put_device() does not guarantee synchronous memory reclamation, will
devres forcibly free the memory when the driver unbinds, creating a
use-after-free for any subsystem still holding a reference to pd_dev?

> +
> +static void db8500_regulator_cleanup(void *data)
> +{
> +	struct db8500_regulator_info *info = data;
> +
> +	pm_runtime_disable(&info->pd_dev);
> +	dev_pm_domain_detach(&info->pd_dev, true);

[Severity: High]
Will this detach silently fail?

The device is attached using of_genpd_add_device(), which adds it to
genpd->dev_list but does not populate the dev->pm_domain->detach callback.

Since dev_pm_domain_detach() checks for that callback, won't it be a no-op,
leaving the device in the genpd active lists when freed and causing a
use-after-free?

Should this use pm_genpd_remove_device() instead?

> +	put_device(&info->pd_dev);
> +}
[ ... ]
> +static int db8500_regulator_register(struct platform_device *pdev,
> +				     struct device_node *np)
> +{
[ ... ]
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	device_initialize(&info->pd_dev);
> +	info->pd_dev.parent = &pdev->dev;
> +	info->pd_dev.of_node = np;

[Severity: High]
Could this leave a dangling pointer in info->pd_dev.of_node?

The np pointer is provided by for_each_available_child_of_node() in
db8500_regulator_probe(), which automatically drops the reference on the
node when advancing the loop.

Since of_node_get() isn't used here to increment the reference count, will
the reference be dropped when the loop concludes?

> +	info->pd_dev.release = db8500_regulator_release;
> +	ret = dev_set_name(&info->pd_dev, "%s-pd", match->name);
> +	if (ret)
> +		goto put_device;
> +
> +	ret = of_parse_phandle_with_args(np, "power-domains", cells, 0, &pd_args);
> +	if (ret)
> +		goto put_device;
> +
> +	ret = of_genpd_add_device(&pd_args, &info->pd_dev);

[Severity: High]
Does this leak PM QoS constraints?

The device is initialized with device_initialize() but never fully registered
with device_add().

Since of_genpd_add_device() automatically registers a resume latency QoS
notifier that dynamically allocates PM QoS constraint structures, and those
are normally freed by device_del(), will these structures leak upon cleanup
because device_del() is never called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-ux500-power-domains-v7-1-v1-0-eb5e50b1a588@kernel.org?part=10