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 <[email protected]> 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/[email protected]?part=10
