linux-kernel - Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 22 Mar 2018 10:30:18 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Kevin Hilman <khilman@...nel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Linux PM <linux-pm@...r.kernel.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Nishanth Menon <nm@...com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> The power-domain core would be using the OPP core going forward and the
> OPP core has the basic requirement of a device structure for its working.

According to the OPP core also seems to require the ->dev.of_node to
be set. Actually, it seems like that part belongs in patch4 instead,
while starting to make use of the opp OF APIs. Could you please move
it?
>
> Add a struct device to the genpd structure and also add a genpd bus type
> for the devices.

This seems reasonable, however I am wondering if we could simplify
this as bit. See more comments below.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/base/power/domain.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0c80bea05bcb..013c1e206167 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1630,6 +1630,10 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>         }
>  }
>
> +static struct bus_type genpd_bus_type = {
> +       .name =         "genpd",
> +};

This seems silly. Can't we just avoid having a bus type altogether,
thus no need to call bus_register() as well?

> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                         return ret;
>         }
>
> +       genpd->dev.bus = &genpd_bus_type;
> +       device_initialize(&genpd->dev);
> +       dev_set_name(&genpd->dev, "%s", genpd->name);
> +
> +       ret = device_add(&genpd->dev);

What's the point of adding the device? Can we skip this step, as I
guess the opp core is only using the device as cookie rather actual
using it? No?

> +       if (ret) {
> +               dev_err(&genpd->dev, "failed to add device: %d\n", ret);
> +               put_device(&genpd->dev);
> +               kfree(genpd->free);
> +               return ret;
> +       }
> +
>         mutex_lock(&gpd_list_lock);
>         list_add(&genpd->gpd_list_node, &gpd_list);
>         mutex_unlock(&gpd_list_lock);
> @@ -1724,6 +1740,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>
>         list_del(&genpd->gpd_list_node);
>         genpd_unlock(genpd);
> +       device_del(&genpd->dev);
>         cancel_work_sync(&genpd->power_off_work);
>         kfree(genpd->free);
>         pr_debug("%s: removed %s\n", __func__, genpd->name);
> @@ -1888,6 +1905,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                 if (!ret) {
>                         genpd->provider = &np->fwnode;
>                         genpd->has_provider = true;
> +                       genpd->dev.of_node = np;
>                 }
>         }
>
> @@ -1924,6 +1942,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>
>                 data->domains[i]->provider = &np->fwnode;
>                 data->domains[i]->has_provider = true;
> +               data->domains[i]->dev.of_node = np;
>         }
>
>         ret = genpd_add_provider(np, data->xlate, data);
> @@ -2691,3 +2710,21 @@ static void __exit genpd_debug_exit(void)
>  }
>  __exitcall(genpd_debug_exit);
>  #endif /* CONFIG_DEBUG_FS */
> +
> +static int __init pm_genpd_core_init(void)
> +{
> +       int ret;
> +
> +       ret = bus_register(&genpd_bus_type);
> +       if (ret)
> +               pr_err("bus_register failed (%d)\n", ret);
> +
> +       return ret;
> +}
> +pure_initcall(pm_genpd_core_init);
> +
> +static void __exit pm_genpd_core_exit(void)
> +{
> +       bus_unregister(&genpd_bus_type);
> +}
> +__exitcall(pm_genpd_core_exit);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 04dbef9847d3..aaacaa35005d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -49,6 +49,7 @@ struct genpd_power_state {
>  struct genpd_lock_ops;
>
>  struct generic_pm_domain {
> +       struct device dev;
>         struct dev_pm_domain domain;    /* PM domain operations */
>         struct list_head gpd_list_node; /* Node in the global PM domains list */
>         struct list_head master_links;  /* Links with PM domain as a master */
> --
> 2.15.0.194.g9af6a3dea062
>

Kind regards
Uffe