Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Probe error path leaks dynamically allocated memory, fails to unregister PM domains, and lacks rollback for partially added subdomains. - [High] Hardware RAM retention capabilities are entirely lost, rendering related state handling logic dead code. - [Medium] Bisection breakage: Disabling REGULATOR_DB8500_PRCMU causes consumer devices expecting `v-ape-supply` to permanently fail to probe. --
commit 26ccd884add2c876cdf4c876688df7b868a2fa06 Author: Linus Walleij <[email protected]> pmdomain: st: ux500: Control DB8500 EPODs Move the DB8500 EPOD state handling into the Ux500 power-domain driver. Keep the old regulator driver mutually exclusive with the pmdomain driver. > diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig > --- a/arch/arm/mach-ux500/Kconfig > +++ b/arch/arm/mach-ux500/Kconfig [ ... ] > @@ -26,7 +26,7 @@ menuconfig ARCH_U8500 > select PL310_ERRATA_753970 if CACHE_L2X0 > select PM_GENERIC_DOMAINS if PM > select REGULATOR > - select REGULATOR_DB8500_PRCMU > + select UX500_PM_DOMAIN > select REGULATOR_FIXED_VOLTAGE > select SOC_BUS > select RESET_CONTROLLER [Severity: Medium] Does this break git bisect by disabling the regulator provider before consumer devices are updated? Consumer devices depending on v-ape-supply may receive -EPROBE_DEFER indefinitely if the regulator driver is disabled here. I see this is fixed in a later commit ("regulator: db8500: Add power domain regulators"), but is it possible to reorder the changes to avoid breaking bisection? > diff --git a/drivers/pmdomain/st/ste-ux500-pm-domain.c > b/drivers/pmdomain/st/ste-ux500-pm-domain.c > --- a/drivers/pmdomain/st/ste-ux500-pm-domain.c > +++ b/drivers/pmdomain/st/ste-ux500-pm-domain.c [ ... ] > +static int disable_epod(u16 epod_id, bool ramret) > +{ > + int ret; > + > + if (ramret) { > + if (!epod_on[epod_id]) { > + ret = prcmu_set_epod(epod_id, EPOD_STATE_OFF); > + if (ret < 0) > + return ret; > + } > + epod_ramret[epod_id] = false; > + } else { > + if (epod_ramret[epod_id]) { > + ret = prcmu_set_epod(epod_id, EPOD_STATE_RAMRET); [Severity: High] Is the hardware RAM retention capability effectively lost here? The struct dbx500_powerdomain_info defines is_ramret, but none of the newly defined domains initialize it to true (e.g., for SVAMMDSP or SIAMMDSP). Because ramret is always false, epod_ramret[] is never set, making the EPOD_STATE_RAMRET state unreachable. Does this dead code need to be activated by setting is_ramret on the appropriate domains? [ ... ] > +static int ux500_pm_domains_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > struct genpd_onecell_data *genpd_data; > int i; > + int ret; > > if (!np) > return -ENODEV; [ ... ] > for (i = 0; i < ARRAY_SIZE(ux500_pm_domains); ++i) > - pm_genpd_init(ux500_pm_domains[i], NULL, false); > + pm_genpd_init(ux500_pm_domains[i], NULL, true); > + > + ret = ux500_pm_domains_add_subdomains(); > + if (ret) > + return ret; [Severity: High] Does this error path leak memory and leave the power domains registered? If ux500_pm_domains_add_subdomains() fails, it returns an error directly. This leaves genpd_data allocated and the domains initialized and linked into the global gpd_list via pm_genpd_init(). If a driver unbind/rebind occurs, could INIT_LIST_HEAD be invoked on elements already in the list, potentially corrupting the doubly linked list and causing a kernel panic? Should pm_genpd_remove() be called for initialized domains, and the added subdomains be rolled back with pm_genpd_remove_subdomain() before returning? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
