| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFoU10ASgtXVUiCyj+rWehkMhkX=w=W1ieTksPdpskUN0Q@mail.gmail.com>
Date: Mon, 8 Dec 2025 14:12:46 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Kevin Hilman (TI.com)" <khilman@...libre.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, devicetree@...r.kernel.org,
linux-pm@...r.kernel.org, arm-scmi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v4 1/2] pmdomain: core: support domain hierarchy via power-domain-map
On Thu, 20 Nov 2025 at 01:58, Kevin Hilman (TI.com)
<khilman@...libre.com> wrote:
>
> Add of_genpd_add_subdomain_map() helper function to support
> hierarchical PM domains defined by using power-domains-map
> property (c.f. nexus node maps in DT spec, section 2.5.1).
>
> This enables PM domain providers with #power-domain-cells > 0 to
> establish subdomain relationships via the power-domain-map property,
> which was not previously possible.
>
> This new helper function
> - uses an OF helper to iterate to over entries in power-domain-map
> - For each mapped entry: extracts child specifier, resolves parent phandle,
> extracts parent specifier args, and establishes subdomain relationship
> - Uses genpd_add_subdomain() with proper gpd_list_lock mutex protection
>
> Example from k3-am62l.dtsi:
>
> scmi_pds: protocol@11 {
> #power-domain-cells = <1>;
> power-domain-map = <15 &MAIN_PD>, /* TIMER0 */
> <19 &WKUP_PD>; /* WKUP_TIMER0 */
> };
>
> MAIN_PD: power-controller-main {
> #power-domain-cells = <0>;
> };
>
> WKUP_PD: power-controller-main {
> #power-domain-cells = <0>;
> };
>
> This allows SCMI power domain 15 to become a subdomain of MAIN_PD, and
> domain 19 to become a subdomain of WKUP_PD.
Nitpick:
As long as possible, please use the terminology "parent-domain" and
"child-domain" and avoid "subdomain". There are a couple of cases of
this, in the code too, can you please update all of them?
>
> Signed-off-by: Kevin Hilman (TI.com) <khilman@...libre.com>
> ---
> drivers/pmdomain/core.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 9 +++++++
> 2 files changed, 73 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 61c2277c9ce3..592e9126896c 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3483,6 +3483,70 @@ int of_genpd_parse_idle_states(struct device_node *dn,
> }
> EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
We need to add some description of the function here.
> +int of_genpd_add_subdomain_map(struct device_node *np,
Nitpick:
Hmm, either we should keep consistency with the name
"of_genpd_add_subdomain", according to what you propose - or we should
take the opportunity to move to use "child" in the name instead
(of_genpd_add_child_domain_map()).
Sooner or later it would be nice if we could rename
of_genpd_add_subdomain() (and friends) to of_genpd_add_child_domain().
No big deal at this point, I am fine with whatever name you decide to use.
> + struct genpd_onecell_data *data)
> +{
> + struct generic_pm_domain *genpd, *parent_genpd;
Maybe use "child" and "parent" as variable names instead. This should
make the code a bit more clear.
> + struct of_phandle_args child_args, parent_args;
> + int index = 0;
> + int ret = 0;
> + u32 child_index;
> +
> + if (!np || !data)
> + return -EINVAL;
> +
> + /* Iterate through power-domain-map entries using the OF helper */
> + while (!of_parse_map_iter(np, "power-domain", &index,
> + &child_args, &parent_args)) {
> + /* Extract the child domain index from the child specifier */
> + if (child_args.args_count < 1) {
This should be exactly 1, right?
> + of_node_put(parent_args.np);
> + ret = -EINVAL;
> + break;
If we fail here, we should remove child domains that we added for the
earlier indexes in the while loop, rather than just bailing out.
This applies to other error paths below too.
> + }
> + child_index = child_args.args[0];
> +
> + /* Validate child domain index */
> + if (child_index >= data->num_domains) {
> + of_node_put(parent_args.np);
> + continue;
I don't think we should just continue here, but instead treat this as an error.
> + }
> +
> + genpd = data->domains[child_index];
> + if (!genpd) {
> + of_node_put(parent_args.np);
> + continue;
Ditto.
> + }
> +
> + /* Get parent power domain from provider and establish subdomain relationship */
> + mutex_lock(&gpd_list_lock);
> +
> + parent_genpd = genpd_get_from_provider(&parent_args);
> + if (IS_ERR(parent_genpd)) {
> + mutex_unlock(&gpd_list_lock);
> + of_node_put(parent_args.np);
> + ret = PTR_ERR(parent_genpd);
> + dev_err(&genpd->dev, "failed to get parent domain: %d\n", ret);
Perhaps clarify the print by changing the text to state that we can't
find the parent's OF provider. If the print is needed at all.
> + break;
> + }
> +
> + ret = genpd_add_subdomain(parent_genpd, genpd);
> + mutex_unlock(&gpd_list_lock);
> + of_node_put(parent_args.np);
> +
> + if (ret) {
> + dev_err(&genpd->dev, "failed to add as subdomain of %s: %d\n",
> + parent_genpd->name, ret);
> + break;
> + }
> +
> + dev_info(&genpd->dev, "added as subdomain of %s\n",
> + parent_genpd->name);
> + }
> +
> + return ret;
> +}
Except for taking better care in the error path, it also looks like we
are missing a corresponding function to remove the child-domains that
was added with the above new function.
Perhaps that function can be used in the error paths too?
> +
> /**
> * of_genpd_sync_state() - A common sync_state function for genpd providers
> * @np: The device node the genpd provider is associated with.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f67a2cb7d781..3585acb89b83 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -462,6 +462,8 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
> int of_genpd_remove_subdomain(const struct of_phandle_args *parent_spec,
> const struct of_phandle_args *subdomain_spec);
> struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
> +int of_genpd_add_subdomain_map(struct device_node *np,
> + struct genpd_onecell_data *data);
> int of_genpd_parse_idle_states(struct device_node *dn,
> struct genpd_power_state **states, int *n);
> void of_genpd_sync_state(struct device_node *np);
> @@ -504,6 +506,13 @@ static inline int of_genpd_remove_subdomain(const struct of_phandle_args *parent
> return -ENODEV;
> }
>
> +static int of_genpd_add_subdomain_map(struct device_node *np,
> + struct generic_pm_domain *genpd,
> + int index)
> +{
> + return -ENODEV;
> +}
> +
> static inline int of_genpd_parse_idle_states(struct device_node *dn,
> struct genpd_power_state **states, int *n)
> {
>
> --
> 2.51.0
>
Kind regards
Uffe
Powered by blists - more mailing lists