Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Low] Using `dev_err()` to log a failure to acquire resets causes log spam when the error is `-EPROBE_DEFER`. Pre-existing issues: - [Critical] Critical use-after-free and state corruption due to `pm_genpd_remove()` failure during probe error handling. - [High] Missing cleanup of previously acquired clocks if `of_clk_get()` fails during initialization. - [High] Use-after-free on the PM domain's device tree node reference (`pd->node`). -- --- Patch [3]: [RFC PATCH v3 3/6] pmdomain/rockchip: cycle optional power-domain resets on power-on --- commit 7b67005dcd951b64c273c1af0e37dfcc0b7586dd Author: Jiaxing Hu <[email protected]> pmdomain/rockchip: cycle optional power-domain resets on power-on Some Rockchip domains come out of power-on with their bus interface in an undefined state. On the RK3576 NPU this shows up as a hang on the first access after a cold power-on; the vendor kernel cycles the domain's resets at this point and that clears it. Take the domain node's resets if it has any, and pulse them between releasing idle and restoring QoS. The resets are optional, so domains that do not declare any behave exactly as before. > diff --git a/drivers/pmdomain/rockchip/pm-domains.c > b/drivers/pmdomain/rockchip/pm-domains.c > index e1857f8786864..82051f60090f3 100644 > --- a/drivers/pmdomain/rockchip/pm-domains.c > +++ b/drivers/pmdomain/rockchip/pm-domains.c [ ... ] > @@ -857,6 +866,14 @@ static int rockchip_pm_add_one_domain(struct > rockchip_pmu *pmu, > if (error) > goto err_put_clocks; [Severity: High] This is a pre-existing issue, but does this function leak clock handles if of_clk_get() fails? Looking at the loop just before this context: rockchip_pm_add_one_domain() { for (i = 0; i < pd->num_clks; i++) { pd->clks[i].clk = of_clk_get(node, i); if (IS_ERR(pd->clks[i].clk)) { ... return error; } } } If of_clk_get() returns -EPROBE_DEFER for an index > 0, it returns immediately without calling clk_put() for the clocks successfully acquired in earlier iterations. Should this jump to a cleanup path instead? [Severity: High] This is another pre-existing issue in this function, but is it safe to store pd->node without incrementing its reference count? In rockchip_pm_domain_probe(), domains are initialized within a loop that automatically drops the node reference. Later, rockchip_pd_power() uses this potentially freed node: rockchip_pm_add_one_domain() pd->node = node; rockchip_pd_power() devm_of_regulator_get(..., pd->node, ...); Could this lead to a use-after-free if the node is dynamically removed? > > + pd->resets = of_reset_control_array_get_optional_exclusive(node); > + if (IS_ERR(pd->resets)) { > + error = PTR_ERR(pd->resets); > + dev_err(pmu->dev, "%pOFn: failed to get resets: %d\n", node, > error); [Severity: Low] Could this dev_err() cause log spam during normal boot? If of_reset_control_array_get_optional_exclusive() returns -EPROBE_DEFER because the reset controller hasn't probed yet, this will print an error. Would dev_err_probe() be better here to suppress the spam and cleanly track the deferral? > + pd->resets = NULL; > + goto err_unprepare_clocks; > + } [ ... ] > @@ -945,6 +963,7 @@ static void rockchip_pm_remove_one_domain(struct > rockchip_pm_domain *pd) > > clk_bulk_unprepare(pd->num_clks, pd->clks); > clk_bulk_put(pd->num_clks, pd->clks); > + reset_control_put(pd->resets); > > /* protect the zeroing of pm->num_clks */ > mutex_lock(&pd->pmu->mutex); [Severity: Critical] This isn't a bug introduced by this patch, but could ignoring the return value of pm_genpd_remove() lead to a use-after-free and state corruption? During probe error paths, rockchip_pm_domain_cleanup() tries to remove domains. If child links aren't removed first, pm_genpd_remove() fails with -EBUSY. Since this failure is ignored: rockchip_pm_remove_one_domain() ret = pm_genpd_remove(&pd->genpd); // ret is -EBUSY, but we continue tearing down clk_bulk_unprepare(...); reset_control_put(pd->resets); The domain remains active in the global gpd_list, but its internal resources are freed, and devm eventually frees the rockchip_pm_domain struct. When the subsystem later iterates over domains, won't it dereference the freed memory or the newly dangling pd->resets pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
