[PATCH 00/10] PM / Domains: Add support for removing PM domains

[PATCH 00/10] PM / Domains: Add support for removing PM domains

[Jon Hunter]

In order to safely remove PM domains there are a few changes that need to be
made to ensure that no one is holding an external reference to a PM domain
after it has been removed. One solution, implemented here, solves this by
eliminating external references to PM domain.

This is a follow-up to the initial RFC I sent out [0].

Changes from initial RFC:
- Renamed functions made static per Ulf's feedback.
- Added patch to clean-up provider/xlate APIs per Ulf's feedback
- Re-worked and simplified the association between PM domains and PM
  domain providers. Dropped the 'provider_data' variable from the
  generic_pm_domain structure in favour of using the fwnode_handle.
- Split patch for removing PM domains into multiple patches per Ulf's
  feedback.

[0] http://marc.info/?l=linux-pm&m=145709064407085&w=2

Jon Hunter (10):
  PM / Domains: Add new helper functions for device-tree
  ARM: EXYNOS: Remove calls to of_genpd_get_from_provider()
  staging: board: Remove calls to of_genpd_get_from_provider()
  PM / Domains: Don't expose generic_pm_domain structure to clients
  PM / Domains: Don't expose xlate and provider helper functions
  PM / Domains: Verify the PM domain is present when adding a provider
  PM / Domains: Prepare for adding support to remove PM domains
  PM / Domains: Add support for removing PM domains
  PM / Domains: Store the provider in the PM domain structure
  PM / Domains: Add support for removing nested PM domains by provider

 drivers/base/power/domain.c      | 361 +++++++++++++++++++++++++++++++++++----
 drivers/soc/samsung/pm_domains.c |  23 +--
 drivers/staging/board/board.c    |   9 +-
 include/linux/pm_domain.h        |  73 ++++----
 4 files changed, 371 insertions(+), 95 deletions(-)

-- 
2.1.4

[PATCH 01/10] PM / Domains: Add new helper functions for device-tree

[Jon Hunter]

Ideally, if we are returning a reference to a PM domain via a call to
of_genpd_get_from_provider(), then we should keep track of such
references via a reference count. The reference count could then be used
to determine if a PM domain can be safely removed. Alternatively, it is
possible to avoid such external references by providing APIs to access
the PM domain and hence, eliminate any calls to
of_genpd_get_from_provider().

Add new helper functions for adding a device and a subdomain to a PM
domain when using device-tree, so that external calls to
of_genpd_get_from_provider() can be removed.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   | 16 ++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a1f2aff33997..43c2a959a7bf 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1502,6 +1502,52 @@ struct generic_pm_domain *of_genpd_get_from_provider(
 EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
 
 /**
+ * of_genpd_add_device() - Add a device to an I/O PM domain
+ * @genpdspec: OF phandle args to use for look-up PM domain
+ * @dev: Device to be added.
+ *
+ * Looks-up an I/O PM domain based upon phandle args provided and adds
+ * the device to the PM domain. Returns a negative error code on failure.
+ */
+int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
+{
+	struct generic_pm_domain *genpd;
+
+	genpd = of_genpd_get_from_provider(genpdspec);
+	if (IS_ERR(genpd))
+		return PTR_ERR(genpd);
+
+	return pm_genpd_add_device(genpd, dev);
+}
+EXPORT_SYMBOL_GPL(of_genpd_add_device);
+
+/**
+ * of_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
+ * @parent_spec: OF phandle args to use for parent PM domain look-up
+ * @subdomain_spec: OF phandle args to use for subdomain look-up
+ *
+ * Looks-up a parent PM domain and subdomain based upon phandle args
+ * provided and adds the subdomain to the parent PM domain. Returns a
+ * negative error code on failure.
+ */
+int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
+			   struct of_phandle_args *subdomain_spec)
+{
+	struct generic_pm_domain *parent, *subdomain;
+
+	parent = of_genpd_get_from_provider(parent_spec);
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	subdomain = of_genpd_get_from_provider(subdomain_spec);
+	if (IS_ERR(subdomain))
+		return PTR_ERR(subdomain);
+
+	return pm_genpd_add_subdomain(parent, subdomain);
+}
+EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
+
+/**
  * genpd_dev_pm_detach - Detach a device from its PM domain.
  * @dev: Device to detach.
  * @power_off: Currently not used
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 31fec858088c..e1964a242389 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -208,6 +208,10 @@ struct generic_pm_domain *__of_genpd_xlate_simple(
 struct generic_pm_domain *__of_genpd_xlate_onecell(
 					struct of_phandle_args *genpdspec,
 					void *data);
+extern int of_genpd_add_device(struct of_phandle_args *args,
+			       struct device *dev);
+extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
+				  struct of_phandle_args *new_subdomain);
 
 int genpd_dev_pm_attach(struct device *dev);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
@@ -227,6 +231,18 @@ static inline struct generic_pm_domain *of_genpd_get_from_provider(
 #define __of_genpd_xlate_simple		NULL
 #define __of_genpd_xlate_onecell	NULL
 
+static inline int of_genpd_add_device(struct of_phandle_args *args,
+				      struct device *dev)
+{
+	return -ENODEV;
+}
+
+static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
+					 struct of_phandle_args *new_subdomain)
+{
+	return -ENODEV;
+}
+
 static inline int genpd_dev_pm_attach(struct device *dev)
 {
 	return -ENODEV;
-- 
2.1.4

[PATCH 01/10] PM / Domains: Add new helper functions for device-tree

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> Ideally, if we are returning a reference to a PM domain via a call to
> of_genpd_get_from_provider(), then we should keep track of such
> references via a reference count. The reference count could then be used
> to determine if a PM domain can be safely removed. Alternatively, it is
> possible to avoid such external references by providing APIs to access
> the PM domain and hence, eliminate any calls to
> of_genpd_get_from_provider().
>
> Add new helper functions for adding a device and a subdomain to a PM
> domain when using device-tree, so that external calls to
> of_genpd_get_from_provider() can be removed.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 16 ++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a1f2aff33997..43c2a959a7bf 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1502,6 +1502,52 @@ struct generic_pm_domain *of_genpd_get_from_provider(
>  EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
>
>  /**
> + * of_genpd_add_device() - Add a device to an I/O PM domain
> + * @genpdspec: OF phandle args to use for look-up PM domain
> + * @dev: Device to be added.
> + *
> + * Looks-up an I/O PM domain based upon phandle args provided and adds
> + * the device to the PM domain. Returns a negative error code on failure.
> + */
> +int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       genpd = of_genpd_get_from_provider(genpdspec);
> +       if (IS_ERR(genpd))
> +               return PTR_ERR(genpd);
> +
> +       return pm_genpd_add_device(genpd, dev);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_device);
> +
> +/**
> + * of_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
> + * @parent_spec: OF phandle args to use for parent PM domain look-up
> + * @subdomain_spec: OF phandle args to use for subdomain look-up
> + *
> + * Looks-up a parent PM domain and subdomain based upon phandle args
> + * provided and adds the subdomain to the parent PM domain. Returns a
> + * negative error code on failure.
> + */
> +int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
> +                          struct of_phandle_args *subdomain_spec)
> +{
> +       struct generic_pm_domain *parent, *subdomain;
> +
> +       parent = of_genpd_get_from_provider(parent_spec);
> +       if (IS_ERR(parent))
> +               return PTR_ERR(parent);
> +
> +       subdomain = of_genpd_get_from_provider(subdomain_spec);
> +       if (IS_ERR(subdomain))
> +               return PTR_ERR(subdomain);
> +
> +       return pm_genpd_add_subdomain(parent, subdomain);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
> +
> +/**
>   * genpd_dev_pm_detach - Detach a device from its PM domain.
>   * @dev: Device to detach.
>   * @power_off: Currently not used
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 31fec858088c..e1964a242389 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -208,6 +208,10 @@ struct generic_pm_domain *__of_genpd_xlate_simple(
>  struct generic_pm_domain *__of_genpd_xlate_onecell(
>                                         struct of_phandle_args *genpdspec,
>                                         void *data);
> +extern int of_genpd_add_device(struct of_phandle_args *args,
> +                              struct device *dev);
> +extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
> +                                 struct of_phandle_args *new_subdomain);
>
>  int genpd_dev_pm_attach(struct device *dev);
>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> @@ -227,6 +231,18 @@ static inline struct generic_pm_domain *of_genpd_get_from_provider(
>  #define __of_genpd_xlate_simple                NULL
>  #define __of_genpd_xlate_onecell       NULL
>
> +static inline int of_genpd_add_device(struct of_phandle_args *args,
> +                                     struct device *dev)
> +{
> +       return -ENODEV;
> +}
> +
> +static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
> +                                        struct of_phandle_args *new_subdomain)
> +{
> +       return -ENODEV;
> +}
> +
>  static inline int genpd_dev_pm_attach(struct device *dev)
>  {
>         return -ENODEV;
> --
> 2.1.4
>

[PATCH 02/10] ARM: EXYNOS: Remove calls to of_genpd_get_from_provider()

[Jon Hunter]

Update the EXYNOS PM domain code to use the of_genpd_add_subdomain()
and remove any calls to of_genpd_get_from_provider().

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/samsung/pm_domains.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/samsung/pm_domains.c b/drivers/soc/samsung/pm_domains.c
index 4822346aadc6..7112004b8032 100644
--- a/drivers/soc/samsung/pm_domains.c
+++ b/drivers/soc/samsung/pm_domains.c
@@ -215,29 +215,22 @@ no_clk:
 
 	/* Assign the child power domains to their parents */
 	for_each_matching_node(np, exynos_pm_domain_of_match) {
-		struct generic_pm_domain *child_domain, *parent_domain;
-		struct of_phandle_args args;
+		struct of_phandle_args child, parent;
 
-		args.np = np;
-		args.args_count = 0;
-		child_domain = of_genpd_get_from_provider(&args);
-		if (IS_ERR(child_domain))
-			continue;
+		child.np = np;
+		child.args_count = 0;
 
 		if (of_parse_phandle_with_args(np, "power-domains",
-					 "#power-domain-cells", 0, &args) != 0)
-			continue;
-
-		parent_domain = of_genpd_get_from_provider(&args);
-		if (IS_ERR(parent_domain))
+					       "#power-domain-cells", 0,
+					       &parent) != 0)
 			continue;
 
-		if (pm_genpd_add_subdomain(parent_domain, child_domain))
+		if (of_genpd_add_subdomain(&parent, &child))
 			pr_warn("%s failed to add subdomain: %s\n",
-				parent_domain->name, child_domain->name);
+				parent.np->name, child.np->name);
 		else
 			pr_info("%s has as child subdomain: %s.\n",
-				parent_domain->name, child_domain->name);
+				parent.np->name, child.np->name);
 	}
 
 	return 0;
-- 
2.1.4

[PATCH 02/10] ARM: EXYNOS: Remove calls to of_genpd_get_from_provider()

[Krzysztof Kozlowski]

On Tue, Aug 16, 2016 at 10:49:28AM +0100, Jon Hunter wrote:
> Update the EXYNOS PM domain code to use the of_genpd_add_subdomain()
> and remove any calls to of_genpd_get_from_provider().
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/samsung/pm_domains.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)

Looks correct:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

I understand this will go along with patch #1 to PM tree. There might be
some more commits around Exynos PM domain code coming soon. To avoid possible
conflicts, could you put it in a separate branch with patch #1 so a
stable tag could be easily created? I don't see direct necessity now but
it might be needed quite soon.

Best regards,
Krzysztof

[PATCH 02/10] ARM: EXYNOS: Remove calls to of_genpd_get_from_provider()

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> Update the EXYNOS PM domain code to use the of_genpd_add_subdomain()
> and remove any calls to of_genpd_get_from_provider().
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/soc/samsung/pm_domains.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/soc/samsung/pm_domains.c b/drivers/soc/samsung/pm_domains.c
> index 4822346aadc6..7112004b8032 100644
> --- a/drivers/soc/samsung/pm_domains.c
> +++ b/drivers/soc/samsung/pm_domains.c
> @@ -215,29 +215,22 @@ no_clk:
>
>         /* Assign the child power domains to their parents */
>         for_each_matching_node(np, exynos_pm_domain_of_match) {
> -               struct generic_pm_domain *child_domain, *parent_domain;
> -               struct of_phandle_args args;
> +               struct of_phandle_args child, parent;
>
> -               args.np = np;
> -               args.args_count = 0;
> -               child_domain = of_genpd_get_from_provider(&args);
> -               if (IS_ERR(child_domain))
> -                       continue;
> +               child.np = np;
> +               child.args_count = 0;
>
>                 if (of_parse_phandle_with_args(np, "power-domains",
> -                                        "#power-domain-cells", 0, &args) != 0)
> -                       continue;
> -
> -               parent_domain = of_genpd_get_from_provider(&args);
> -               if (IS_ERR(parent_domain))
> +                                              "#power-domain-cells", 0,
> +                                              &parent) != 0)
>                         continue;
>
> -               if (pm_genpd_add_subdomain(parent_domain, child_domain))
> +               if (of_genpd_add_subdomain(&parent, &child))
>                         pr_warn("%s failed to add subdomain: %s\n",
> -                               parent_domain->name, child_domain->name);
> +                               parent.np->name, child.np->name);
>                 else
>                         pr_info("%s has as child subdomain: %s.\n",
> -                               parent_domain->name, child_domain->name);
> +                               parent.np->name, child.np->name);
>         }
>
>         return 0;
> --
> 2.1.4
>

[PATCH 03/10] staging: board: Remove calls to of_genpd_get_from_provider()

[Jon Hunter]

Update the staging/board PM domain code to use the
of_genpd_add_subdomain() and remove any calls to
of_genpd_get_from_provider().

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/staging/board/board.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
index 45807d8287d1..86dc41101610 100644
--- a/drivers/staging/board/board.c
+++ b/drivers/staging/board/board.c
@@ -140,7 +140,6 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
 					const char *domain)
 {
 	struct of_phandle_args pd_args;
-	struct generic_pm_domain *pd;
 	struct device_node *np;
 
 	np = of_find_node_by_path(domain);
@@ -151,14 +150,8 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
 
 	pd_args.np = np;
 	pd_args.args_count = 0;
-	pd = of_genpd_get_from_provider(&pd_args);
-	if (IS_ERR(pd)) {
-		pr_err("Cannot find genpd %s (%ld)\n", domain, PTR_ERR(pd));
-		return PTR_ERR(pd);
-	}
-	pr_debug("Found genpd %s for device %s\n", pd->name, pdev->name);
 
-	return pm_genpd_add_device(pd, &pdev->dev);
+	return of_genpd_add_device(&pd_args, &pdev->dev);
 }
 #else
 static inline int board_staging_add_dev_domain(struct platform_device *pdev,
-- 
2.1.4

[PATCH 03/10] staging: board: Remove calls to of_genpd_get_from_provider()

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> Update the staging/board PM domain code to use the
> of_genpd_add_subdomain() and remove any calls to
> of_genpd_get_from_provider().
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/staging/board/board.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
> index 45807d8287d1..86dc41101610 100644
> --- a/drivers/staging/board/board.c
> +++ b/drivers/staging/board/board.c
> @@ -140,7 +140,6 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
>                                         const char *domain)
>  {
>         struct of_phandle_args pd_args;
> -       struct generic_pm_domain *pd;
>         struct device_node *np;
>
>         np = of_find_node_by_path(domain);
> @@ -151,14 +150,8 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
>
>         pd_args.np = np;
>         pd_args.args_count = 0;
> -       pd = of_genpd_get_from_provider(&pd_args);
> -       if (IS_ERR(pd)) {
> -               pr_err("Cannot find genpd %s (%ld)\n", domain, PTR_ERR(pd));
> -               return PTR_ERR(pd);
> -       }
> -       pr_debug("Found genpd %s for device %s\n", pd->name, pdev->name);
>
> -       return pm_genpd_add_device(pd, &pdev->dev);
> +       return of_genpd_add_device(&pd_args, &pdev->dev);
>  }
>  #else
>  static inline int board_staging_add_dev_domain(struct platform_device *pdev,
> --
> 2.1.4
>

[PATCH 04/10] PM / Domains: Don't expose generic_pm_domain structure to clients

[Jon Hunter]

There should be no need to expose the generic_pm_domain structure to
clients and this eliminates the need to implement reference counting for
any external reference to a PM domain. Therefore, make the functions
pm_genpd_lookup_dev() and of_genpd_get_from_provider() private to the
PM domain core. The functions are renamed in accordance with the naming
conventions for genpd static functions.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 19 +++++++++----------
 include/linux/pm_domain.h   | 14 --------------
 2 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 43c2a959a7bf..7b0aecef2e51 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -45,7 +45,7 @@ static DEFINE_MUTEX(gpd_list_lock);
  * and checks that the PM domain pointer is a real generic PM domain.
  * Any failure results in NULL being returned.
  */
-struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
+static struct generic_pm_domain *genpd_lookup_dev(struct device *dev)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;
 
@@ -1119,7 +1119,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	if (!genpd || genpd != pm_genpd_lookup_dev(dev))
+	if (!genpd || genpd != genpd_lookup_dev(dev))
 		return -EINVAL;
 
 	/* The above validation also means we have existing domain_data. */
@@ -1466,7 +1466,7 @@ void of_genpd_del_provider(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_genpd_del_provider);
 
 /**
- * of_genpd_get_from_provider() - Look-up PM domain
+ * genpd_get_from_provider() - Look-up PM domain
  * @genpdspec: OF phandle args to use for look-up
  *
  * Looks for a PM domain provider under the node specified by @genpdspec and if
@@ -1476,7 +1476,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
  * Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
  * on failure.
  */
-struct generic_pm_domain *of_genpd_get_from_provider(
+static struct generic_pm_domain *genpd_get_from_provider(
 					struct of_phandle_args *genpdspec)
 {
 	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
@@ -1499,7 +1499,6 @@ struct generic_pm_domain *of_genpd_get_from_provider(
 
 	return genpd;
 }
-EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
 
 /**
  * of_genpd_add_device() - Add a device to an I/O PM domain
@@ -1513,7 +1512,7 @@ int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
 {
 	struct generic_pm_domain *genpd;
 
-	genpd = of_genpd_get_from_provider(genpdspec);
+	genpd = genpd_get_from_provider(genpdspec);
 	if (IS_ERR(genpd))
 		return PTR_ERR(genpd);
 
@@ -1535,11 +1534,11 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 {
 	struct generic_pm_domain *parent, *subdomain;
 
-	parent = of_genpd_get_from_provider(parent_spec);
+	parent = genpd_get_from_provider(parent_spec);
 	if (IS_ERR(parent))
 		return PTR_ERR(parent);
 
-	subdomain = of_genpd_get_from_provider(subdomain_spec);
+	subdomain = genpd_get_from_provider(subdomain_spec);
 	if (IS_ERR(subdomain))
 		return PTR_ERR(subdomain);
 
@@ -1561,7 +1560,7 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 	unsigned int i;
 	int ret = 0;
 
-	pd = pm_genpd_lookup_dev(dev);
+	pd = genpd_lookup_dev(dev);
 	if (!pd)
 		return;
 
@@ -1642,7 +1641,7 @@ int genpd_dev_pm_attach(struct device *dev)
 			return -ENOENT;
 	}
 
-	pd = of_genpd_get_from_provider(&pd_args);
+	pd = genpd_get_from_provider(&pd_args);
 	of_node_put(pd_args.np);
 	if (IS_ERR(pd)) {
 		dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e1964a242389..bd411e754f4a 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -116,7 +116,6 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 	return to_gpd_data(dev->power.subsys_data->domain_data);
 }
 
-extern struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev);
 extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
 				 struct device *dev,
 				 struct gpd_timing_data *td);
@@ -138,10 +137,6 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 {
 	return ERR_PTR(-ENOSYS);
 }
-static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
-{
-	return NULL;
-}
 static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
 					struct device *dev,
 					struct gpd_timing_data *td)
@@ -199,9 +194,6 @@ typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
 int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
 			void *data);
 void of_genpd_del_provider(struct device_node *np);
-struct generic_pm_domain *of_genpd_get_from_provider(
-			struct of_phandle_args *genpdspec);
-
 struct generic_pm_domain *__of_genpd_xlate_simple(
 					struct of_phandle_args *genpdspec,
 					void *data);
@@ -222,12 +214,6 @@ static inline int __of_genpd_add_provider(struct device_node *np,
 }
 static inline void of_genpd_del_provider(struct device_node *np) {}
 
-static inline struct generic_pm_domain *of_genpd_get_from_provider(
-			struct of_phandle_args *genpdspec)
-{
-	return NULL;
-}
-
 #define __of_genpd_xlate_simple		NULL
 #define __of_genpd_xlate_onecell	NULL
 
-- 
2.1.4

[PATCH 04/10] PM / Domains: Don't expose generic_pm_domain structure to clients

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> There should be no need to expose the generic_pm_domain structure to
> clients and this eliminates the need to implement reference counting for
> any external reference to a PM domain. Therefore, make the functions
> pm_genpd_lookup_dev() and of_genpd_get_from_provider() private to the
> PM domain core. The functions are renamed in accordance with the naming
> conventions for genpd static functions.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 19 +++++++++----------
>  include/linux/pm_domain.h   | 14 --------------
>  2 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 43c2a959a7bf..7b0aecef2e51 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -45,7 +45,7 @@ static DEFINE_MUTEX(gpd_list_lock);
>   * and checks that the PM domain pointer is a real generic PM domain.
>   * Any failure results in NULL being returned.
>   */
> -struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
> +static struct generic_pm_domain *genpd_lookup_dev(struct device *dev)
>  {
>         struct generic_pm_domain *genpd = NULL, *gpd;
>
> @@ -1119,7 +1119,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
>         dev_dbg(dev, "%s()\n", __func__);
>
> -       if (!genpd || genpd != pm_genpd_lookup_dev(dev))
> +       if (!genpd || genpd != genpd_lookup_dev(dev))
>                 return -EINVAL;
>
>         /* The above validation also means we have existing domain_data. */
> @@ -1466,7 +1466,7 @@ void of_genpd_del_provider(struct device_node *np)
>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>
>  /**
> - * of_genpd_get_from_provider() - Look-up PM domain
> + * genpd_get_from_provider() - Look-up PM domain
>   * @genpdspec: OF phandle args to use for look-up
>   *
>   * Looks for a PM domain provider under the node specified by @genpdspec and if
> @@ -1476,7 +1476,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>   * Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
>   * on failure.
>   */
> -struct generic_pm_domain *of_genpd_get_from_provider(
> +static struct generic_pm_domain *genpd_get_from_provider(
>                                         struct of_phandle_args *genpdspec)
>  {
>         struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
> @@ -1499,7 +1499,6 @@ struct generic_pm_domain *of_genpd_get_from_provider(
>
>         return genpd;
>  }
> -EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
>
>  /**
>   * of_genpd_add_device() - Add a device to an I/O PM domain
> @@ -1513,7 +1512,7 @@ int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
>  {
>         struct generic_pm_domain *genpd;
>
> -       genpd = of_genpd_get_from_provider(genpdspec);
> +       genpd = genpd_get_from_provider(genpdspec);
>         if (IS_ERR(genpd))
>                 return PTR_ERR(genpd);
>
> @@ -1535,11 +1534,11 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
>  {
>         struct generic_pm_domain *parent, *subdomain;
>
> -       parent = of_genpd_get_from_provider(parent_spec);
> +       parent = genpd_get_from_provider(parent_spec);
>         if (IS_ERR(parent))
>                 return PTR_ERR(parent);
>
> -       subdomain = of_genpd_get_from_provider(subdomain_spec);
> +       subdomain = genpd_get_from_provider(subdomain_spec);
>         if (IS_ERR(subdomain))
>                 return PTR_ERR(subdomain);
>
> @@ -1561,7 +1560,7 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>         unsigned int i;
>         int ret = 0;
>
> -       pd = pm_genpd_lookup_dev(dev);
> +       pd = genpd_lookup_dev(dev);
>         if (!pd)
>                 return;
>
> @@ -1642,7 +1641,7 @@ int genpd_dev_pm_attach(struct device *dev)
>                         return -ENOENT;
>         }
>
> -       pd = of_genpd_get_from_provider(&pd_args);
> +       pd = genpd_get_from_provider(&pd_args);
>         of_node_put(pd_args.np);
>         if (IS_ERR(pd)) {
>                 dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e1964a242389..bd411e754f4a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -116,7 +116,6 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>         return to_gpd_data(dev->power.subsys_data->domain_data);
>  }
>
> -extern struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev);
>  extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
>                                  struct device *dev,
>                                  struct gpd_timing_data *td);
> @@ -138,10 +137,6 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>  {
>         return ERR_PTR(-ENOSYS);
>  }
> -static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev)
> -{
> -       return NULL;
> -}
>  static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
>                                         struct device *dev,
>                                         struct gpd_timing_data *td)
> @@ -199,9 +194,6 @@ typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
>  int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>                         void *data);
>  void of_genpd_del_provider(struct device_node *np);
> -struct generic_pm_domain *of_genpd_get_from_provider(
> -                       struct of_phandle_args *genpdspec);
> -
>  struct generic_pm_domain *__of_genpd_xlate_simple(
>                                         struct of_phandle_args *genpdspec,
>                                         void *data);
> @@ -222,12 +214,6 @@ static inline int __of_genpd_add_provider(struct device_node *np,
>  }
>  static inline void of_genpd_del_provider(struct device_node *np) {}
>
> -static inline struct generic_pm_domain *of_genpd_get_from_provider(
> -                       struct of_phandle_args *genpdspec)
> -{
> -       return NULL;
> -}
> -
>  #define __of_genpd_xlate_simple                NULL
>  #define __of_genpd_xlate_onecell       NULL
>
> --
> 2.1.4
>

[PATCH 05/10] PM / Domains: Don't expose xlate and provider helper functions

[Jon Hunter]

Functions __of_genpd_xlate_simple(), __of_genpd_xlate_onecell() and
__of_genpd_add_provider() are not used outside of the core generic PM
domain code. Therefore, reduce the number of APIs exposed by making
these static. At the same time don't expose the typedef for
genpd_xlate_t either and make this a local definition as well.

The functions are renamed to follow the naming conventions for static
functions in the generic PM domain core.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 49 ++++++++++++++++++++++++++++++++++-----------
 include/linux/pm_domain.h   | 42 +++++++++++++-------------------------
 2 files changed, 51 insertions(+), 40 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7b0aecef2e51..d09e45145a3d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1329,6 +1329,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+
+typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
+						   void *data);
+
 /*
  * Device Tree based PM domain providers.
  *
@@ -1340,8 +1344,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_init);
  * maps a PM domain specifier retrieved from the device tree to a PM domain.
  *
  * Two simple mapping functions have been provided for convenience:
- *  - __of_genpd_xlate_simple() for 1:1 device tree node to PM domain mapping.
- *  - __of_genpd_xlate_onecell() for mapping of multiple PM domains per node by
+ *  - genpd_xlate_simple() for 1:1 device tree node to PM domain mapping.
+ *  - genpd_xlate_onecell() for mapping of multiple PM domains per node by
  *    index.
  */
 
@@ -1366,7 +1370,7 @@ static LIST_HEAD(of_genpd_providers);
 static DEFINE_MUTEX(of_genpd_mutex);
 
 /**
- * __of_genpd_xlate_simple() - Xlate function for direct node-domain mapping
+ * genpd_xlate_simple() - Xlate function for direct node-domain mapping
  * @genpdspec: OF phandle args to map into a PM domain
  * @data: xlate function private data - pointer to struct generic_pm_domain
  *
@@ -1374,7 +1378,7 @@ static DEFINE_MUTEX(of_genpd_mutex);
  * have their own device tree nodes. The private data of xlate function needs
  * to be a valid pointer to struct generic_pm_domain.
  */
-struct generic_pm_domain *__of_genpd_xlate_simple(
+static struct generic_pm_domain *genpd_xlate_simple(
 					struct of_phandle_args *genpdspec,
 					void *data)
 {
@@ -1382,10 +1386,9 @@ struct generic_pm_domain *__of_genpd_xlate_simple(
 		return ERR_PTR(-EINVAL);
 	return data;
 }
-EXPORT_SYMBOL_GPL(__of_genpd_xlate_simple);
 
 /**
- * __of_genpd_xlate_onecell() - Xlate function using a single index.
+ * genpd_xlate_onecell() - Xlate function using a single index.
  * @genpdspec: OF phandle args to map into a PM domain
  * @data: xlate function private data - pointer to struct genpd_onecell_data
  *
@@ -1394,7 +1397,7 @@ EXPORT_SYMBOL_GPL(__of_genpd_xlate_simple);
  * A single cell is used as an index into an array of PM domains specified in
  * the genpd_onecell_data struct when registering the provider.
  */
-struct generic_pm_domain *__of_genpd_xlate_onecell(
+static struct generic_pm_domain *genpd_xlate_onecell(
 					struct of_phandle_args *genpdspec,
 					void *data)
 {
@@ -1414,16 +1417,15 @@ struct generic_pm_domain *__of_genpd_xlate_onecell(
 
 	return genpd_data->domains[idx];
 }
-EXPORT_SYMBOL_GPL(__of_genpd_xlate_onecell);
 
 /**
- * __of_genpd_add_provider() - Register a PM domain provider for a node
+ * genpd_add_provider() - Register a PM domain provider for a node
  * @np: Device node pointer associated with the PM domain provider.
  * @xlate: Callback for decoding PM domain from phandle arguments.
  * @data: Context pointer for @xlate callback.
  */
-int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
-			void *data)
+static int genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
+			      void *data)
 {
 	struct of_genpd_provider *cp;
 
@@ -1442,7 +1444,30 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(__of_genpd_add_provider);
+
+/**
+ * of_genpd_add_provider_simple() - Register a simple PM domain provider
+ * @np: Device node pointer associated with the PM domain provider.
+ * @genpd: Pointer to PM domain associated with the PM domain provider.
+ */
+int of_genpd_add_provider_simple(struct device_node *np,
+				 struct generic_pm_domain *genpd)
+{
+	return genpd_add_provider(np, genpd_xlate_simple, genpd);
+}
+EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
+
+/**
+ * of_genpd_add_provider_onecell() - Register a onecell PM domain provider
+ * @np: Device node pointer associated with the PM domain provider.
+ * @data: Pointer to the data associated with the PM domain provider.
+ */
+int of_genpd_add_provider_onecell(struct device_node *np,
+				  struct genpd_onecell_data *data)
+{
+	return genpd_add_provider(np, genpd_xlate_onecell, data);
+}
+EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
 
 /**
  * of_genpd_del_provider() - Remove a previously registered PM domain provider
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index bd411e754f4a..f103869db443 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -187,19 +187,12 @@ struct genpd_onecell_data {
 	unsigned int num_domains;
 };
 
-typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
-						void *data);
-
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
-int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
-			void *data);
+int of_genpd_add_provider_simple(struct device_node *np,
+				 struct generic_pm_domain *genpd);
+int of_genpd_add_provider_onecell(struct device_node *np,
+				  struct genpd_onecell_data *data);
 void of_genpd_del_provider(struct device_node *np);
-struct generic_pm_domain *__of_genpd_xlate_simple(
-					struct of_phandle_args *genpdspec,
-					void *data);
-struct generic_pm_domain *__of_genpd_xlate_onecell(
-					struct of_phandle_args *genpdspec,
-					void *data);
 extern int of_genpd_add_device(struct of_phandle_args *args,
 			       struct device *dev);
 extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
@@ -207,15 +200,19 @@ extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
 
 int genpd_dev_pm_attach(struct device *dev);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
-static inline int __of_genpd_add_provider(struct device_node *np,
-					genpd_xlate_t xlate, void *data)
+static inline int of_genpd_add_provider_simple(struct device_node *np,
+					struct generic_pm_domain *genpd)
 {
-	return 0;
+	return -ENOTSUPP;
+}
+
+static inline int of_genpd_add_provider_onecell(struct device_node *np,
+					struct genpd_onecell_data *data)
+{
+	return -ENOTSUPP;
 }
-static inline void of_genpd_del_provider(struct device_node *np) {}
 
-#define __of_genpd_xlate_simple		NULL
-#define __of_genpd_xlate_onecell	NULL
+static inline void of_genpd_del_provider(struct device_node *np) {}
 
 static inline int of_genpd_add_device(struct of_phandle_args *args,
 				      struct device *dev)
@@ -235,17 +232,6 @@ static inline int genpd_dev_pm_attach(struct device *dev)
 }
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
-static inline int of_genpd_add_provider_simple(struct device_node *np,
-					struct generic_pm_domain *genpd)
-{
-	return __of_genpd_add_provider(np, __of_genpd_xlate_simple, genpd);
-}
-static inline int of_genpd_add_provider_onecell(struct device_node *np,
-					struct genpd_onecell_data *data)
-{
-	return __of_genpd_add_provider(np, __of_genpd_xlate_onecell, data);
-}
-
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
 extern void dev_pm_domain_detach(struct device *dev, bool power_off);
-- 
2.1.4

[PATCH 05/10] PM / Domains: Don't expose xlate and provider helper functions

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> Functions __of_genpd_xlate_simple(), __of_genpd_xlate_onecell() and
> __of_genpd_add_provider() are not used outside of the core generic PM
> domain code. Therefore, reduce the number of APIs exposed by making
> these static. At the same time don't expose the typedef for
> genpd_xlate_t either and make this a local definition as well.
>
> The functions are renamed to follow the naming conventions for static
> functions in the generic PM domain core.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 49 ++++++++++++++++++++++++++++++++++-----------
>  include/linux/pm_domain.h   | 42 +++++++++++++-------------------------
>  2 files changed, 51 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7b0aecef2e51..d09e45145a3d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1329,6 +1329,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> +
> +typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
> +                                                  void *data);
> +
>  /*
>   * Device Tree based PM domain providers.
>   *
> @@ -1340,8 +1344,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_init);
>   * maps a PM domain specifier retrieved from the device tree to a PM domain.
>   *
>   * Two simple mapping functions have been provided for convenience:
> - *  - __of_genpd_xlate_simple() for 1:1 device tree node to PM domain mapping.
> - *  - __of_genpd_xlate_onecell() for mapping of multiple PM domains per node by
> + *  - genpd_xlate_simple() for 1:1 device tree node to PM domain mapping.
> + *  - genpd_xlate_onecell() for mapping of multiple PM domains per node by
>   *    index.
>   */
>
> @@ -1366,7 +1370,7 @@ static LIST_HEAD(of_genpd_providers);
>  static DEFINE_MUTEX(of_genpd_mutex);
>
>  /**
> - * __of_genpd_xlate_simple() - Xlate function for direct node-domain mapping
> + * genpd_xlate_simple() - Xlate function for direct node-domain mapping
>   * @genpdspec: OF phandle args to map into a PM domain
>   * @data: xlate function private data - pointer to struct generic_pm_domain
>   *
> @@ -1374,7 +1378,7 @@ static DEFINE_MUTEX(of_genpd_mutex);
>   * have their own device tree nodes. The private data of xlate function needs
>   * to be a valid pointer to struct generic_pm_domain.
>   */
> -struct generic_pm_domain *__of_genpd_xlate_simple(
> +static struct generic_pm_domain *genpd_xlate_simple(
>                                         struct of_phandle_args *genpdspec,
>                                         void *data)
>  {
> @@ -1382,10 +1386,9 @@ struct generic_pm_domain *__of_genpd_xlate_simple(
>                 return ERR_PTR(-EINVAL);
>         return data;
>  }
> -EXPORT_SYMBOL_GPL(__of_genpd_xlate_simple);
>
>  /**
> - * __of_genpd_xlate_onecell() - Xlate function using a single index.
> + * genpd_xlate_onecell() - Xlate function using a single index.
>   * @genpdspec: OF phandle args to map into a PM domain
>   * @data: xlate function private data - pointer to struct genpd_onecell_data
>   *
> @@ -1394,7 +1397,7 @@ EXPORT_SYMBOL_GPL(__of_genpd_xlate_simple);
>   * A single cell is used as an index into an array of PM domains specified in
>   * the genpd_onecell_data struct when registering the provider.
>   */
> -struct generic_pm_domain *__of_genpd_xlate_onecell(
> +static struct generic_pm_domain *genpd_xlate_onecell(
>                                         struct of_phandle_args *genpdspec,
>                                         void *data)
>  {
> @@ -1414,16 +1417,15 @@ struct generic_pm_domain *__of_genpd_xlate_onecell(
>
>         return genpd_data->domains[idx];
>  }
> -EXPORT_SYMBOL_GPL(__of_genpd_xlate_onecell);
>
>  /**
> - * __of_genpd_add_provider() - Register a PM domain provider for a node
> + * genpd_add_provider() - Register a PM domain provider for a node
>   * @np: Device node pointer associated with the PM domain provider.
>   * @xlate: Callback for decoding PM domain from phandle arguments.
>   * @data: Context pointer for @xlate callback.
>   */
> -int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> -                       void *data)
> +static int genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> +                             void *data)
>  {
>         struct of_genpd_provider *cp;
>
> @@ -1442,7 +1444,30 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(__of_genpd_add_provider);
> +
> +/**
> + * of_genpd_add_provider_simple() - Register a simple PM domain provider
> + * @np: Device node pointer associated with the PM domain provider.
> + * @genpd: Pointer to PM domain associated with the PM domain provider.
> + */
> +int of_genpd_add_provider_simple(struct device_node *np,
> +                                struct generic_pm_domain *genpd)
> +{
> +       return genpd_add_provider(np, genpd_xlate_simple, genpd);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> +
> +/**
> + * of_genpd_add_provider_onecell() - Register a onecell PM domain provider
> + * @np: Device node pointer associated with the PM domain provider.
> + * @data: Pointer to the data associated with the PM domain provider.
> + */
> +int of_genpd_add_provider_onecell(struct device_node *np,
> +                                 struct genpd_onecell_data *data)
> +{
> +       return genpd_add_provider(np, genpd_xlate_onecell, data);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
>
>  /**
>   * of_genpd_del_provider() - Remove a previously registered PM domain provider
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index bd411e754f4a..f103869db443 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -187,19 +187,12 @@ struct genpd_onecell_data {
>         unsigned int num_domains;
>  };
>
> -typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
> -                                               void *data);
> -
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> -int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> -                       void *data);
> +int of_genpd_add_provider_simple(struct device_node *np,
> +                                struct generic_pm_domain *genpd);
> +int of_genpd_add_provider_onecell(struct device_node *np,
> +                                 struct genpd_onecell_data *data);
>  void of_genpd_del_provider(struct device_node *np);
> -struct generic_pm_domain *__of_genpd_xlate_simple(
> -                                       struct of_phandle_args *genpdspec,
> -                                       void *data);
> -struct generic_pm_domain *__of_genpd_xlate_onecell(
> -                                       struct of_phandle_args *genpdspec,
> -                                       void *data);
>  extern int of_genpd_add_device(struct of_phandle_args *args,
>                                struct device *dev);
>  extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
> @@ -207,15 +200,19 @@ extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>
>  int genpd_dev_pm_attach(struct device *dev);
>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> -static inline int __of_genpd_add_provider(struct device_node *np,
> -                                       genpd_xlate_t xlate, void *data)
> +static inline int of_genpd_add_provider_simple(struct device_node *np,
> +                                       struct generic_pm_domain *genpd)
>  {
> -       return 0;
> +       return -ENOTSUPP;
> +}
> +
> +static inline int of_genpd_add_provider_onecell(struct device_node *np,
> +                                       struct genpd_onecell_data *data)
> +{
> +       return -ENOTSUPP;
>  }
> -static inline void of_genpd_del_provider(struct device_node *np) {}
>
> -#define __of_genpd_xlate_simple                NULL
> -#define __of_genpd_xlate_onecell       NULL
> +static inline void of_genpd_del_provider(struct device_node *np) {}
>
>  static inline int of_genpd_add_device(struct of_phandle_args *args,
>                                       struct device *dev)
> @@ -235,17 +232,6 @@ static inline int genpd_dev_pm_attach(struct device *dev)
>  }
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
> -static inline int of_genpd_add_provider_simple(struct device_node *np,
> -                                       struct generic_pm_domain *genpd)
> -{
> -       return __of_genpd_add_provider(np, __of_genpd_xlate_simple, genpd);
> -}
> -static inline int of_genpd_add_provider_onecell(struct device_node *np,
> -                                       struct genpd_onecell_data *data)
> -{
> -       return __of_genpd_add_provider(np, __of_genpd_xlate_onecell, data);
> -}
> -
>  #ifdef CONFIG_PM
>  extern int dev_pm_domain_attach(struct device *dev, bool power_on);
>  extern void dev_pm_domain_detach(struct device *dev, bool power_off);
> --
> 2.1.4
>

[PATCH 06/10] PM / Domains: Verify the PM domain is present when adding a provider

[Jon Hunter]

When a PM domain provider is added, there is currently no way to tell if
any PM domains associated with the provider are present. Naturally, the
PM domain provider should not be registered if the PM domains have not
been added. Nonetheless, verify that the PM domain(s) associated with a
provider are present when registering the PM domain provider.

This change adds a dependency on the function pm_genpd_present() when
CONFIG_PM_GENERIC_DOMAINS_OF is enabled and so ensure this function is
available when CONFIG_PM_GENERIC_DOMAINS_OF selected.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d09e45145a3d..50223ae0c9a7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -586,7 +586,7 @@ static int __init genpd_poweroff_unused(void)
 }
 late_initcall(genpd_poweroff_unused);
 
-#ifdef CONFIG_PM_SLEEP
+#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_GENERIC_DOMAINS_OF)
 
 /**
  * pm_genpd_present - Check if the given PM domain has been initialized.
@@ -606,6 +606,10 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd)
 	return false;
 }
 
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+
 static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
 				    struct device *dev)
 {
@@ -1453,7 +1457,23 @@ static int genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
 int of_genpd_add_provider_simple(struct device_node *np,
 				 struct generic_pm_domain *genpd)
 {
-	return genpd_add_provider(np, genpd_xlate_simple, genpd);
+	int ret;
+
+	if (!np || !genpd)
+		return -EINVAL;
+
+	mutex_lock(&gpd_list_lock);
+
+	if (!pm_genpd_present(genpd)) {
+		mutex_unlock(&gpd_list_lock);
+		return -EINVAL;
+	}
+
+	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
+
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
 
@@ -1465,7 +1485,26 @@ EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
 int of_genpd_add_provider_onecell(struct device_node *np,
 				  struct genpd_onecell_data *data)
 {
-	return genpd_add_provider(np, genpd_xlate_onecell, data);
+	unsigned int i;
+	int ret;
+
+	if (!np || !data)
+		return -EINVAL;
+
+	mutex_lock(&gpd_list_lock);
+
+	for (i = 0; i < data->num_domains; i++) {
+		if (!pm_genpd_present(data->domains[i])) {
+			mutex_unlock(&gpd_list_lock);
+			return -EINVAL;
+		}
+	}
+
+	ret = genpd_add_provider(np, genpd_xlate_onecell, data);
+
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
 
-- 
2.1.4

[PATCH 06/10] PM / Domains: Verify the PM domain is present when adding a provider

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> When a PM domain provider is added, there is currently no way to tell if
> any PM domains associated with the provider are present. Naturally, the
> PM domain provider should not be registered if the PM domains have not
> been added. Nonetheless, verify that the PM domain(s) associated with a
> provider are present when registering the PM domain provider.
>
> This change adds a dependency on the function pm_genpd_present() when
> CONFIG_PM_GENERIC_DOMAINS_OF is enabled and so ensure this function is
> available when CONFIG_PM_GENERIC_DOMAINS_OF selected.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d09e45145a3d..50223ae0c9a7 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -586,7 +586,7 @@ static int __init genpd_poweroff_unused(void)
>  }
>  late_initcall(genpd_poweroff_unused);
>
> -#ifdef CONFIG_PM_SLEEP
> +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_GENERIC_DOMAINS_OF)
>
>  /**
>   * pm_genpd_present - Check if the given PM domain has been initialized.
> @@ -606,6 +606,10 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd)
>         return false;
>  }
>
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +
>  static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
>                                     struct device *dev)
>  {
> @@ -1453,7 +1457,23 @@ static int genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>  int of_genpd_add_provider_simple(struct device_node *np,
>                                  struct generic_pm_domain *genpd)
>  {
> -       return genpd_add_provider(np, genpd_xlate_simple, genpd);
> +       int ret;
> +
> +       if (!np || !genpd)
> +               return -EINVAL;
> +
> +       mutex_lock(&gpd_list_lock);
> +
> +       if (!pm_genpd_present(genpd)) {
> +               mutex_unlock(&gpd_list_lock);
> +               return -EINVAL;
> +       }
> +
> +       ret = genpd_add_provider(np, genpd_xlate_simple, genpd);

You could simplify this, by assigning ret and initial value of
-EINVAL, then do like this:

if (pm_genpd_present(genpd))
    ret = genpd_add_provider(np, genpd_xlate_simple, genpd);

> +
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
>
> @@ -1465,7 +1485,26 @@ EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
>  int of_genpd_add_provider_onecell(struct device_node *np,
>                                   struct genpd_onecell_data *data)
>  {
> -       return genpd_add_provider(np, genpd_xlate_onecell, data);
> +       unsigned int i;
> +       int ret;
> +
> +       if (!np || !data)
> +               return -EINVAL;
> +
> +       mutex_lock(&gpd_list_lock);
> +
> +       for (i = 0; i < data->num_domains; i++) {
> +               if (!pm_genpd_present(data->domains[i])) {
> +                       mutex_unlock(&gpd_list_lock);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       ret = genpd_add_provider(np, genpd_xlate_onecell, data);
> +
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
>
> --
> 2.1.4
>

Besides the minor nitpick above, you may add my ack.

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

[PATCH 06/10] PM / Domains: Verify the PM domain is present when adding a provider

[Jon Hunter]

On 08/09/16 12:39, Ulf Hansson wrote:
> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>> When a PM domain provider is added, there is currently no way to tell if
>> any PM domains associated with the provider are present. Naturally, the
>> PM domain provider should not be registered if the PM domains have not
>> been added. Nonetheless, verify that the PM domain(s) associated with a
>> provider are present when registering the PM domain provider.
>>
>> This change adds a dependency on the function pm_genpd_present() when
>> CONFIG_PM_GENERIC_DOMAINS_OF is enabled and so ensure this function is
>> available when CONFIG_PM_GENERIC_DOMAINS_OF selected.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/base/power/domain.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index d09e45145a3d..50223ae0c9a7 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -586,7 +586,7 @@ static int __init genpd_poweroff_unused(void)
>>  }
>>  late_initcall(genpd_poweroff_unused);
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_GENERIC_DOMAINS_OF)
>>
>>  /**
>>   * pm_genpd_present - Check if the given PM domain has been initialized.
>> @@ -606,6 +606,10 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd)
>>         return false;
>>  }
>>
>> +#endif
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +
>>  static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
>>                                     struct device *dev)
>>  {
>> @@ -1453,7 +1457,23 @@ static int genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>>  int of_genpd_add_provider_simple(struct device_node *np,
>>                                  struct generic_pm_domain *genpd)
>>  {
>> -       return genpd_add_provider(np, genpd_xlate_simple, genpd);
>> +       int ret;
>> +
>> +       if (!np || !genpd)
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&gpd_list_lock);
>> +
>> +       if (!pm_genpd_present(genpd)) {
>> +               mutex_unlock(&gpd_list_lock);
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> 
> You could simplify this, by assigning ret and initial value of
> -EINVAL, then do like this:
> 
> if (pm_genpd_present(genpd))
>     ret = genpd_add_provider(np, genpd_xlate_simple, genpd);

Yes good idea. Will do.

Thanks
Jon

-- 
nvpublic

[PATCH 07/10] PM / Domains: Prepare for adding support to remove PM domains

[Jon Hunter]

In order to remove PM domains safely from the list of PM domains,
it is necessary to adding locking for the PM domain list around any
places where devices or subdomains are added to a PM domain.

There are places where a reference to a PM domain is obtained via
calling of_genpd_get_from_provider() before adding the device or the
subdomain. In these cases a lock for the PM domain list needs to be
held around the call to of_genpd_get_from_provider() and the call to
add the device/subdomain. To avoid deadlocks by multiple attempts to
obtain the PM domain list lock, add functions genpd_add_device() and
genpd_add_subdomain() which require the user to hold the PM domain
list lock when calling.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 97 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 24 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 50223ae0c9a7..7ce4608bbbe0 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1060,14 +1060,8 @@ static void genpd_free_dev_data(struct device *dev,
 	dev_pm_put_subsys_data(dev);
 }
 
-/**
- * __pm_genpd_add_device - Add a device to an I/O PM domain.
- * @genpd: PM domain to add the device to.
- * @dev: Device to be added.
- * @td: Set of PM QoS timing parameters to attach to the device.
- */
-int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
-			  struct gpd_timing_data *td)
+static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
+			    struct gpd_timing_data *td)
 {
 	struct generic_pm_domain_data *gpd_data;
 	int ret = 0;
@@ -1107,6 +1101,24 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 
 	return ret;
 }
+
+/**
+ * __pm_genpd_add_device - Add a device to an I/O PM domain.
+ * @genpd: PM domain to add the device to.
+ * @dev: Device to be added.
+ * @td: Set of PM QoS timing parameters to attach to the device.
+ */
+int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
+			  struct gpd_timing_data *td)
+{
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
+	ret = genpd_add_device(genpd, dev, td);
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(__pm_genpd_add_device);
 
 /**
@@ -1160,13 +1172,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
 
-/**
- * pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
- * @genpd: Master PM domain to add the subdomain to.
- * @subdomain: Subdomain to be added.
- */
-int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
-			   struct generic_pm_domain *subdomain)
+static int genpd_add_subdomain(struct generic_pm_domain *genpd,
+			       struct generic_pm_domain *subdomain)
 {
 	struct gpd_link *link, *itr;
 	int ret = 0;
@@ -1209,6 +1216,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		kfree(link);
 	return ret;
 }
+
+/**
+ * pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
+ * @genpd: Master PM domain to add the subdomain to.
+ * @subdomain: Subdomain to be added.
+ */
+int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
+			   struct generic_pm_domain *subdomain)
+{
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
+	ret = genpd_add_subdomain(genpd, subdomain);
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(pm_genpd_add_subdomain);
 
 /**
@@ -1575,12 +1599,22 @@ static struct generic_pm_domain *genpd_get_from_provider(
 int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
 
 	genpd = genpd_get_from_provider(genpdspec);
-	if (IS_ERR(genpd))
-		return PTR_ERR(genpd);
+	if (IS_ERR(genpd)) {
+		ret = PTR_ERR(genpd);
+		goto out;
+	}
 
-	return pm_genpd_add_device(genpd, dev);
+	ret = genpd_add_device(genpd, dev, NULL);
+
+out:
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_device);
 
@@ -1597,16 +1631,28 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 			   struct of_phandle_args *subdomain_spec)
 {
 	struct generic_pm_domain *parent, *subdomain;
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
 
 	parent = genpd_get_from_provider(parent_spec);
-	if (IS_ERR(parent))
-		return PTR_ERR(parent);
+	if (IS_ERR(parent)) {
+		ret = PTR_ERR(parent);
+		goto out;
+	}
 
 	subdomain = genpd_get_from_provider(subdomain_spec);
-	if (IS_ERR(subdomain))
-		return PTR_ERR(subdomain);
+	if (IS_ERR(subdomain)) {
+		ret = PTR_ERR(subdomain);
+		goto out;
+	}
+
+	ret = genpd_add_subdomain(parent, subdomain);
+
+out:
+	mutex_unlock(&gpd_list_lock);
 
-	return pm_genpd_add_subdomain(parent, subdomain);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
 
@@ -1705,9 +1751,11 @@ int genpd_dev_pm_attach(struct device *dev)
 			return -ENOENT;
 	}
 
+	mutex_lock(&gpd_list_lock);
 	pd = genpd_get_from_provider(&pd_args);
 	of_node_put(pd_args.np);
 	if (IS_ERR(pd)) {
+		mutex_unlock(&gpd_list_lock);
 		dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
 			__func__, PTR_ERR(pd));
 		return -EPROBE_DEFER;
@@ -1716,13 +1764,14 @@ int genpd_dev_pm_attach(struct device *dev)
 	dev_dbg(dev, "adding to PM domain %s\n", pd->name);
 
 	for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
-		ret = pm_genpd_add_device(pd, dev);
+		ret = genpd_add_device(pd, dev, NULL);
 		if (ret != -EAGAIN)
 			break;
 
 		mdelay(i);
 		cond_resched();
 	}
+	mutex_unlock(&gpd_list_lock);
 
 	if (ret < 0) {
 		dev_err(dev, "failed to add to PM domain %s: %d",
-- 
2.1.4

[PATCH 07/10] PM / Domains: Prepare for adding support to remove PM domains

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> In order to remove PM domains safely from the list of PM domains,
> it is necessary to adding locking for the PM domain list around any
> places where devices or subdomains are added to a PM domain.
>
> There are places where a reference to a PM domain is obtained via
> calling of_genpd_get_from_provider() before adding the device or the
> subdomain. In these cases a lock for the PM domain list needs to be
> held around the call to of_genpd_get_from_provider() and the call to
> add the device/subdomain. To avoid deadlocks by multiple attempts to
> obtain the PM domain list lock, add functions genpd_add_device() and
> genpd_add_subdomain() which require the user to hold the PM domain
> list lock when calling.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 97 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 50223ae0c9a7..7ce4608bbbe0 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1060,14 +1060,8 @@ static void genpd_free_dev_data(struct device *dev,
>         dev_pm_put_subsys_data(dev);
>  }
>
> -/**
> - * __pm_genpd_add_device - Add a device to an I/O PM domain.
> - * @genpd: PM domain to add the device to.
> - * @dev: Device to be added.
> - * @td: Set of PM QoS timing parameters to attach to the device.
> - */
> -int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> -                         struct gpd_timing_data *td)
> +static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> +                           struct gpd_timing_data *td)
>  {
>         struct generic_pm_domain_data *gpd_data;
>         int ret = 0;
> @@ -1107,6 +1101,24 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>
>         return ret;
>  }
> +
> +/**
> + * __pm_genpd_add_device - Add a device to an I/O PM domain.
> + * @genpd: PM domain to add the device to.
> + * @dev: Device to be added.
> + * @td: Set of PM QoS timing parameters to attach to the device.
> + */
> +int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> +                         struct gpd_timing_data *td)
> +{
> +       int ret;
> +
> +       mutex_lock(&gpd_list_lock);
> +       ret = genpd_add_device(genpd, dev, td);
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
> +}
>  EXPORT_SYMBOL_GPL(__pm_genpd_add_device);
>
>  /**
> @@ -1160,13 +1172,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>  }
>  EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
>
> -/**
> - * pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
> - * @genpd: Master PM domain to add the subdomain to.
> - * @subdomain: Subdomain to be added.
> - */
> -int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> -                          struct generic_pm_domain *subdomain)
> +static int genpd_add_subdomain(struct generic_pm_domain *genpd,
> +                              struct generic_pm_domain *subdomain)
>  {
>         struct gpd_link *link, *itr;
>         int ret = 0;
> @@ -1209,6 +1216,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>                 kfree(link);
>         return ret;
>  }
> +
> +/**
> + * pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
> + * @genpd: Master PM domain to add the subdomain to.
> + * @subdomain: Subdomain to be added.
> + */
> +int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> +                          struct generic_pm_domain *subdomain)
> +{
> +       int ret;
> +
> +       mutex_lock(&gpd_list_lock);
> +       ret = genpd_add_subdomain(genpd, subdomain);
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
> +}
>  EXPORT_SYMBOL_GPL(pm_genpd_add_subdomain);
>
>  /**
> @@ -1575,12 +1599,22 @@ static struct generic_pm_domain *genpd_get_from_provider(
>  int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
>  {
>         struct generic_pm_domain *genpd;
> +       int ret;
> +
> +       mutex_lock(&gpd_list_lock);
>
>         genpd = genpd_get_from_provider(genpdspec);
> -       if (IS_ERR(genpd))
> -               return PTR_ERR(genpd);
> +       if (IS_ERR(genpd)) {
> +               ret = PTR_ERR(genpd);
> +               goto out;
> +       }
>
> -       return pm_genpd_add_device(genpd, dev);
> +       ret = genpd_add_device(genpd, dev, NULL);
> +
> +out:
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_device);
>
> @@ -1597,16 +1631,28 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
>                            struct of_phandle_args *subdomain_spec)
>  {
>         struct generic_pm_domain *parent, *subdomain;
> +       int ret;
> +
> +       mutex_lock(&gpd_list_lock);
>
>         parent = genpd_get_from_provider(parent_spec);
> -       if (IS_ERR(parent))
> -               return PTR_ERR(parent);
> +       if (IS_ERR(parent)) {
> +               ret = PTR_ERR(parent);
> +               goto out;
> +       }
>
>         subdomain = genpd_get_from_provider(subdomain_spec);
> -       if (IS_ERR(subdomain))
> -               return PTR_ERR(subdomain);
> +       if (IS_ERR(subdomain)) {
> +               ret = PTR_ERR(subdomain);
> +               goto out;
> +       }
> +
> +       ret = genpd_add_subdomain(parent, subdomain);
> +
> +out:
> +       mutex_unlock(&gpd_list_lock);
>
> -       return pm_genpd_add_subdomain(parent, subdomain);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
>
> @@ -1705,9 +1751,11 @@ int genpd_dev_pm_attach(struct device *dev)
>                         return -ENOENT;
>         }
>
> +       mutex_lock(&gpd_list_lock);
>         pd = genpd_get_from_provider(&pd_args);
>         of_node_put(pd_args.np);
>         if (IS_ERR(pd)) {
> +               mutex_unlock(&gpd_list_lock);
>                 dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
>                         __func__, PTR_ERR(pd));
>                 return -EPROBE_DEFER;
> @@ -1716,13 +1764,14 @@ int genpd_dev_pm_attach(struct device *dev)
>         dev_dbg(dev, "adding to PM domain %s\n", pd->name);
>
>         for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
> -               ret = pm_genpd_add_device(pd, dev);
> +               ret = genpd_add_device(pd, dev, NULL);
>                 if (ret != -EAGAIN)
>                         break;
>
>                 mdelay(i);
>                 cond_resched();
>         }
> +       mutex_unlock(&gpd_list_lock);
>
>         if (ret < 0) {
>                 dev_err(dev, "failed to add to PM domain %s: %d",
> --
> 2.1.4
>

[PATCH 08/10] PM / Domains: Add support for removing PM domains

[Jon Hunter]

The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.

Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.

PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  5 +++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7ce4608bbbe0..72b973539205 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1356,6 +1356,52 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+/**
+ * pm_genpd_remove - Remove a generic I/O PM domain
+ * @genpd: Pointer to PM domain that is to be removed.
+ *
+ * To remove the PM domain, this function:
+ *  - Removes the PM domain as a subdomain to any parent domains,
+ *    if it was added.
+ *  - Removes the PM domain from the list of registered PM domains.
+ *
+ * The PM domain will only be removed, if it is not a parent to any
+ * other PM domain and has no devices associated with it.
+ */
+int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	struct gpd_link *l, *link;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(genpd))
+		return -EINVAL;
+
+	mutex_lock(&gpd_list_lock);
+	mutex_lock(&genpd->lock);
+
+	if (!list_empty(&genpd->master_links) || genpd->device_count) {
+		mutex_unlock(&genpd->lock);
+		mutex_unlock(&gpd_list_lock);
+		pr_err("%s: unable to remove %s\n", __func__, genpd->name);
+		return -EBUSY;
+	}
+
+	list_for_each_entry_safe(link, l, &genpd->slave_links, slave_node) {
+		list_del(&link->master_node);
+		list_del(&link->slave_node);
+		kfree(link);
+	}
+
+	list_del(&genpd->gpd_list_node);
+	mutex_unlock(&genpd->lock);
+	cancel_work_sync(&genpd->power_off_work);
+	mutex_unlock(&gpd_list_lock);
+	pr_debug("%s: removed %s\n", __func__, genpd->name);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_remove);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 
 typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index f103869db443..fbdc5c4588ef 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -128,6 +128,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 				     struct generic_pm_domain *target);
 extern int pm_genpd_init(struct generic_pm_domain *genpd,
 			 struct dev_power_governor *gov, bool is_off);
+extern int pm_genpd_remove(struct generic_pm_domain *genpd);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -163,6 +164,10 @@ static inline int pm_genpd_init(struct generic_pm_domain *genpd,
 {
 	return -ENOSYS;
 }
+static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	return -ENOTSUPP;
+}
 #endif
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-- 
2.1.4

[PATCH 08/10] PM / Domains: Add support for removing PM domains

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> The genpd framework allows users to add PM domains via the pm_genpd_init()
> function, however, there is no corresponding function to remove a PM
> domain. For most devices this may be fine as the PM domains are never
> removed, however, for devices that wish to populate the PM domains from
> within a driver, having the ability to remove a PM domain if the probing
> of the device fails or the driver is unloaded is necessary.
>
> Add the function pm_genpd_remove() to remove a PM domain by referencing
> it's generic_pm_domain structure.
>
> PM domains can only be removed if they are not a parent domain to
> another PM domain and have no devices associated with them.

I think we should also check if the there's is a provider registered
for the genpd, as it should also prevent the genpd from being removed.
Right?

I noticed that you are adding the ->provider pointer to the genpd
struct in patch 9/10. Perhaps re-structure $subject patch and 9/10 a
bit to deal with this, so we can add the pm_genpd_remove() API in a
more safe manner.

Kind regards
Uffe

>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  5 +++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7ce4608bbbe0..72b973539205 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1356,6 +1356,52 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>  }
>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>
> +/**
> + * pm_genpd_remove - Remove a generic I/O PM domain
> + * @genpd: Pointer to PM domain that is to be removed.
> + *
> + * To remove the PM domain, this function:
> + *  - Removes the PM domain as a subdomain to any parent domains,
> + *    if it was added.
> + *  - Removes the PM domain from the list of registered PM domains.
> + *
> + * The PM domain will only be removed, if it is not a parent to any
> + * other PM domain and has no devices associated with it.
> + */
> +int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> +       struct gpd_link *l, *link;
> +       int ret = 0;
> +
> +       if (IS_ERR_OR_NULL(genpd))
> +               return -EINVAL;
> +
> +       mutex_lock(&gpd_list_lock);
> +       mutex_lock(&genpd->lock);
> +
> +       if (!list_empty(&genpd->master_links) || genpd->device_count) {
> +               mutex_unlock(&genpd->lock);
> +               mutex_unlock(&gpd_list_lock);
> +               pr_err("%s: unable to remove %s\n", __func__, genpd->name);
> +               return -EBUSY;
> +       }
> +
> +       list_for_each_entry_safe(link, l, &genpd->slave_links, slave_node) {
> +               list_del(&link->master_node);
> +               list_del(&link->slave_node);
> +               kfree(link);
> +       }
> +
> +       list_del(&genpd->gpd_list_node);
> +       mutex_unlock(&genpd->lock);
> +       cancel_work_sync(&genpd->power_off_work);
> +       mutex_unlock(&gpd_list_lock);
> +       pr_debug("%s: removed %s\n", __func__, genpd->name);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_remove);
> +
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>
>  typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f103869db443..fbdc5c4588ef 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -128,6 +128,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>                                      struct generic_pm_domain *target);
>  extern int pm_genpd_init(struct generic_pm_domain *genpd,
>                          struct dev_power_governor *gov, bool is_off);
> +extern int pm_genpd_remove(struct generic_pm_domain *genpd);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -163,6 +164,10 @@ static inline int pm_genpd_init(struct generic_pm_domain *genpd,
>  {
>         return -ENOSYS;
>  }
> +static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> +       return -ENOTSUPP;
> +}
>  #endif
>
>  static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
> --
> 2.1.4
>

[PATCH 08/10] PM / Domains: Add support for removing PM domains

[Jon Hunter]

On 08/09/16 12:49, Ulf Hansson wrote:
> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>> The genpd framework allows users to add PM domains via the pm_genpd_init()
>> function, however, there is no corresponding function to remove a PM
>> domain. For most devices this may be fine as the PM domains are never
>> removed, however, for devices that wish to populate the PM domains from
>> within a driver, having the ability to remove a PM domain if the probing
>> of the device fails or the driver is unloaded is necessary.
>>
>> Add the function pm_genpd_remove() to remove a PM domain by referencing
>> it's generic_pm_domain structure.
>>
>> PM domains can only be removed if they are not a parent domain to
>> another PM domain and have no devices associated with them.
> 
> I think we should also check if the there's is a provider registered
> for the genpd, as it should also prevent the genpd from being removed.
> Right?

Yes I would agree. I had thought that after patch #4 of this series that
only the provider itself would be able to call this. However, we should
probably still verify that the provider has correctly remove itself.

> I noticed that you are adding the ->provider pointer to the genpd
> struct in patch 9/10. Perhaps re-structure $subject patch and 9/10 a
> bit to deal with this, so we can add the pm_genpd_remove() API in a
> more safe manner.

Yes I can add the >provider member before this patch.

Cheers
Jon

-- 
nvpublic

[PATCH 08/10] PM / Domains: Add support for removing PM domains

[Jon Hunter]

On 09/09/16 14:54, Jon Hunter wrote:
> On 08/09/16 12:49, Ulf Hansson wrote:
>> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> The genpd framework allows users to add PM domains via the pm_genpd_init()
>>> function, however, there is no corresponding function to remove a PM
>>> domain. For most devices this may be fine as the PM domains are never
>>> removed, however, for devices that wish to populate the PM domains from
>>> within a driver, having the ability to remove a PM domain if the probing
>>> of the device fails or the driver is unloaded is necessary.
>>>
>>> Add the function pm_genpd_remove() to remove a PM domain by referencing
>>> it's generic_pm_domain structure.
>>>
>>> PM domains can only be removed if they are not a parent domain to
>>> another PM domain and have no devices associated with them.
>>
>> I think we should also check if the there's is a provider registered
>> for the genpd, as it should also prevent the genpd from being removed.
>> Right?
> 
> Yes I would agree. I had thought that after patch #4 of this series that
> only the provider itself would be able to call this. However, we should
> probably still verify that the provider has correctly remove itself.

So now I have the following. I am still not 100% happy. I cannot clear
the ->provider when calling of_genpd_del_provider() and so I cannot use
this to verify if the provider is present and so I need to check the
list of providers and it gets a bit messy. I have been wracking my
brains to find a better alternative (including a single function to
remove the provider and domains at once but there are issues with that
as well).

I think that long term it may make sense to reference the providers
exclusively by the fwnode_handle and make the list of provider non-DT
specific. I could do it now, but it would increase the series.

Cheers
Jon

---
 drivers/base/power/domain.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  5 +++
 2 files changed, 94 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 82f87038f108..57c64fc1a164 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -39,6 +39,8 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static bool genpd_provider_present(struct device_node *np);
+
 /*
  * Get the generic PM domain for a particular struct device.
  * This validates the struct device pointer, the PM domain pointer,
@@ -1356,6 +1358,68 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+static int genpd_remove(struct generic_pm_domain *genpd)
+{
+	struct gpd_link *l, *link;
+
+	if (IS_ERR_OR_NULL(genpd))
+		return -EINVAL;
+
+	mutex_lock(&genpd->lock);
+
+	if (is_of_node(genpd->provider)) {
+		if (genpd_provider_present(to_of_node(genpd->provider))) {
+			mutex_unlock(&genpd->lock);
+			pr_err("Provider present, unable to remove %s\n",
+			       genpd->name);
+			return -EBUSY;
+		}
+	}
+
+	if (!list_empty(&genpd->master_links) || genpd->device_count) {
+		mutex_unlock(&genpd->lock);
+		pr_err("%s: unable to remove %s\n", __func__, genpd->name);
+		return -EBUSY;
+	}
+
+	list_for_each_entry_safe(link, l, &genpd->slave_links, slave_node) {
+		list_del(&link->master_node);
+		list_del(&link->slave_node);
+		kfree(link);
+	}
+
+	list_del(&genpd->gpd_list_node);
+	mutex_unlock(&genpd->lock);
+	cancel_work_sync(&genpd->power_off_work);
+	pr_debug("%s: removed %s\n", __func__, genpd->name);
+
+	return 0;
+}
+
+/**
+ * pm_genpd_remove - Remove a generic I/O PM domain
+ * @genpd: Pointer to PM domain that is to be removed.
+ *
+ * To remove the PM domain, this function:
+ *  - Removes the PM domain as a subdomain to any parent domains,
+ *    if it was added.
+ *  - Removes the PM domain from the list of registered PM domains.
+ *
+ * The PM domain will only be removed, if it is not a parent to any
+ * other PM domain and has no devices associated with it.
+ */
+int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
+	ret = genpd_remove(genpd);
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_remove);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 
 typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
@@ -1563,6 +1627,26 @@ void of_genpd_del_provider(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_genpd_del_provider);
 
 /**
+ * genpd_provider_present() - Verify if a PM domain provider is present
+ * @np: Device node pointer associated with the PM domain provider
+ */
+static bool genpd_provider_present(struct device_node *np)
+{
+	struct of_genpd_provider *cp;
+
+	mutex_lock(&of_genpd_mutex);
+	list_for_each_entry(cp, &of_genpd_providers, link) {
+		if (cp->node == np) {
+			mutex_unlock(&of_genpd_mutex);
+			return true;
+		}
+	}
+	mutex_unlock(&of_genpd_mutex);
+
+	return false;
+}
+
+/**
  * genpd_get_from_provider() - Look-up PM domain
  * @genpdspec: OF phandle args to use for look-up
  *
@@ -1798,6 +1882,11 @@ out:
 	return ret ? -EPROBE_DEFER : 0;
 }
 EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
+#else
+static bool genpd_provider_present(struct device_node *np)
+{
+	return false;
+}
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e71764ac7248..4aa285e44eb0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -129,6 +129,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 				     struct generic_pm_domain *target);
 extern int pm_genpd_init(struct generic_pm_domain *genpd,
 			 struct dev_power_governor *gov, bool is_off);
+extern int pm_genpd_remove(struct generic_pm_domain *genpd);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -164,6 +165,10 @@ static inline int pm_genpd_init(struct generic_pm_domain *genpd,
 {
 	return -ENOSYS;
 }
+static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	return -ENOTSUPP;
+}
 #endif
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-- 
2.1.4 

-- 
nvpublic

[PATCH 08/10] PM / Domains: Add support for removing PM domains

[Ulf Hansson]

On 9 September 2016 at 17:17, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 09/09/16 14:54, Jon Hunter wrote:
>> On 08/09/16 12:49, Ulf Hansson wrote:
>>> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> The genpd framework allows users to add PM domains via the pm_genpd_init()
>>>> function, however, there is no corresponding function to remove a PM
>>>> domain. For most devices this may be fine as the PM domains are never
>>>> removed, however, for devices that wish to populate the PM domains from
>>>> within a driver, having the ability to remove a PM domain if the probing
>>>> of the device fails or the driver is unloaded is necessary.
>>>>
>>>> Add the function pm_genpd_remove() to remove a PM domain by referencing
>>>> it's generic_pm_domain structure.
>>>>
>>>> PM domains can only be removed if they are not a parent domain to
>>>> another PM domain and have no devices associated with them.
>>>
>>> I think we should also check if the there's is a provider registered
>>> for the genpd, as it should also prevent the genpd from being removed.
>>> Right?
>>
>> Yes I would agree. I had thought that after patch #4 of this series that
>> only the provider itself would be able to call this. However, we should
>> probably still verify that the provider has correctly remove itself.
>
> So now I have the following. I am still not 100% happy. I cannot clear
> the ->provider when calling of_genpd_del_provider() and so I cannot use
> this to verify if the provider is present and so I need to check the
> list of providers and it gets a bit messy. I have been wracking my
> brains to find a better alternative (including a single function to
> remove the provider and domains at once but there are issues with that
> as well).

Instead of using the ->provider pointer to know whether the genpd has
a valid provider, why not just add an additional ->has_provider bool
flag in the genpd struct?

Simply set the flag when adding the provider and reset it when
removing it. Wouldn't that work?

>
> I think that long term it may make sense to reference the providers
> exclusively by the fwnode_handle and make the list of provider non-DT
> specific. I could do it now, but it would increase the series.

Perhaps a good idea. Although I agree, let's not make that change as a
part of this series.

[...]

Kind regards
Uffe

[PATCH 08/10] PM / Domains: Add support for removing PM domains

[Jon Hunter]

On 12/09/16 08:21, Ulf Hansson wrote:
> On 9 September 2016 at 17:17, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 09/09/16 14:54, Jon Hunter wrote:
>>> On 08/09/16 12:49, Ulf Hansson wrote:
>>>> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> The genpd framework allows users to add PM domains via the pm_genpd_init()
>>>>> function, however, there is no corresponding function to remove a PM
>>>>> domain. For most devices this may be fine as the PM domains are never
>>>>> removed, however, for devices that wish to populate the PM domains from
>>>>> within a driver, having the ability to remove a PM domain if the probing
>>>>> of the device fails or the driver is unloaded is necessary.
>>>>>
>>>>> Add the function pm_genpd_remove() to remove a PM domain by referencing
>>>>> it's generic_pm_domain structure.
>>>>>
>>>>> PM domains can only be removed if they are not a parent domain to
>>>>> another PM domain and have no devices associated with them.
>>>>
>>>> I think we should also check if the there's is a provider registered
>>>> for the genpd, as it should also prevent the genpd from being removed.
>>>> Right?
>>>
>>> Yes I would agree. I had thought that after patch #4 of this series that
>>> only the provider itself would be able to call this. However, we should
>>> probably still verify that the provider has correctly remove itself.
>>
>> So now I have the following. I am still not 100% happy. I cannot clear
>> the ->provider when calling of_genpd_del_provider() and so I cannot use
>> this to verify if the provider is present and so I need to check the
>> list of providers and it gets a bit messy. I have been wracking my
>> brains to find a better alternative (including a single function to
>> remove the provider and domains at once but there are issues with that
>> as well).
> 
> Instead of using the ->provider pointer to know whether the genpd has
> a valid provider, why not just add an additional ->has_provider bool
> flag in the genpd struct?
> 
> Simply set the flag when adding the provider and reset it when
> removing it. Wouldn't that work?

Yes. I was trying not to add to much clutter to the struct. However, may
be this is the best option.

Cheers
Jon

-- 
nvpublic

[PATCH 09/10] PM / Domains: Store the provider in the PM domain structure

[Jon Hunter]

It is possible that a device has more than one provider of PM domains
and to support the removal of a PM domain by provider, it is necessary
to store a reference to the provider in the PM domain structure.
Therefore, store a reference to the firmware node handle in the PM
domain structure and populate it when providers (only device-tree based
providers are currently supported by PM domains) are registered.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 18 ++++++++++++++----
 include/linux/pm_domain.h   |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 72b973539205..0bc145e8e902 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
 		return -EINVAL;
 	}
 
+	genpd->provider = &np->fwnode;
+
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
 
 	mutex_unlock(&gpd_list_lock);
@@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 	mutex_lock(&gpd_list_lock);
 
 	for (i = 0; i < data->num_domains; i++) {
-		if (!pm_genpd_present(data->domains[i])) {
-			mutex_unlock(&gpd_list_lock);
-			return -EINVAL;
-		}
+		if (!pm_genpd_present(data->domains[i]))
+			goto error;
+
+		data->domains[i]->provider = &np->fwnode;
 	}
 
 	ret = genpd_add_provider(np, genpd_xlate_onecell, data);
@@ -1575,6 +1577,14 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 	mutex_unlock(&gpd_list_lock);
 
 	return ret;
+
+error:
+	while (i--)
+		data->domains[i]->provider = NULL;
+
+	mutex_unlock(&gpd_list_lock);
+
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index fbdc5c4588ef..4aa285e44eb0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -51,6 +51,7 @@ struct generic_pm_domain {
 	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
+	struct fwnode_handle *provider;	/* Identity of the domain provider */
 	const char *name;
 	atomic_t sd_count;	/* Number of subdomains with power "on" */
 	enum gpd_status status;	/* Current state of the domain */
-- 
2.1.4

[PATCH 09/10] PM / Domains: Store the provider in the PM domain structure

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> It is possible that a device has more than one provider of PM domains
> and to support the removal of a PM domain by provider, it is necessary
> to store a reference to the provider in the PM domain structure.
> Therefore, store a reference to the firmware node handle in the PM
> domain structure and populate it when providers (only device-tree based
> providers are currently supported by PM domains) are registered.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 18 ++++++++++++++----
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 72b973539205..0bc145e8e902 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                 return -EINVAL;
>         }
>
> +       genpd->provider = &np->fwnode;
> +
>         ret = genpd_add_provider(np, genpd_xlate_simple, genpd);

I guess you want to reset genpd->provider = NULL, when
genpd_add_provider() fails!?

Perhaps better to assign genpd->provider when you know
genpd_add_provider() has succeeded.

>
>         mutex_unlock(&gpd_list_lock);
> @@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>         mutex_lock(&gpd_list_lock);
>
>         for (i = 0; i < data->num_domains; i++) {
> -               if (!pm_genpd_present(data->domains[i])) {
> -                       mutex_unlock(&gpd_list_lock);
> -                       return -EINVAL;
> -               }
> +               if (!pm_genpd_present(data->domains[i]))
> +                       goto error;
> +
> +               data->domains[i]->provider = &np->fwnode;
>         }
>
>         ret = genpd_add_provider(np, genpd_xlate_onecell, data);
> @@ -1575,6 +1577,14 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>         mutex_unlock(&gpd_list_lock);
>
>         return ret;
> +
> +error:
> +       while (i--)
> +               data->domains[i]->provider = NULL;
> +
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index fbdc5c4588ef..4aa285e44eb0 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -51,6 +51,7 @@ struct generic_pm_domain {
>         struct mutex lock;
>         struct dev_power_governor *gov;
>         struct work_struct power_off_work;
> +       struct fwnode_handle *provider; /* Identity of the domain provider */
>         const char *name;
>         atomic_t sd_count;      /* Number of subdomains with power "on" */
>         enum gpd_status status; /* Current state of the domain */
> --
> 2.1.4
>

I also think you should extend this change, to also make the
of_genpd_del_provider() API to reset the genpd->provider = NULL.
Otherwise you can't track when a provider is removed.

Kind regards
Uffe

[PATCH 09/10] PM / Domains: Store the provider in the PM domain structure

[Jon Hunter]

On 08/09/16 12:56, Ulf Hansson wrote:
> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>> It is possible that a device has more than one provider of PM domains
>> and to support the removal of a PM domain by provider, it is necessary
>> to store a reference to the provider in the PM domain structure.
>> Therefore, store a reference to the firmware node handle in the PM
>> domain structure and populate it when providers (only device-tree based
>> providers are currently supported by PM domains) are registered.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/base/power/domain.c | 18 ++++++++++++++----
>>  include/linux/pm_domain.h   |  1 +
>>  2 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 72b973539205..0bc145e8e902 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>                 return -EINVAL;
>>         }
>>
>> +       genpd->provider = &np->fwnode;
>> +
>>         ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> 
> I guess you want to reset genpd->provider = NULL, when
> genpd_add_provider() fails!?

Yes, will fix.

> Perhaps better to assign genpd->provider when you know
> genpd_add_provider() has succeeded.

Yes seems sane!

>>
>>         mutex_unlock(&gpd_list_lock);
>> @@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>>         mutex_lock(&gpd_list_lock);
>>
>>         for (i = 0; i < data->num_domains; i++) {
>> -               if (!pm_genpd_present(data->domains[i])) {
>> -                       mutex_unlock(&gpd_list_lock);
>> -                       return -EINVAL;
>> -               }
>> +               if (!pm_genpd_present(data->domains[i]))
>> +                       goto error;
>> +
>> +               data->domains[i]->provider = &np->fwnode;
>>         }
>>
>>         ret = genpd_add_provider(np, genpd_xlate_onecell, data);
>> @@ -1575,6 +1577,14 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>>         mutex_unlock(&gpd_list_lock);
>>
>>         return ret;
>> +
>> +error:
>> +       while (i--)
>> +               data->domains[i]->provider = NULL;
>> +
>> +       mutex_unlock(&gpd_list_lock);
>> +
>> +       return -EINVAL;
>>  }
>>  EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index fbdc5c4588ef..4aa285e44eb0 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -51,6 +51,7 @@ struct generic_pm_domain {
>>         struct mutex lock;
>>         struct dev_power_governor *gov;
>>         struct work_struct power_off_work;
>> +       struct fwnode_handle *provider; /* Identity of the domain provider */
>>         const char *name;
>>         atomic_t sd_count;      /* Number of subdomains with power "on" */
>>         enum gpd_status status; /* Current state of the domain */
>> --
>> 2.1.4
>>
> 
> I also think you should extend this change, to also make the
> of_genpd_del_provider() API to reset the genpd->provider = NULL.
> Otherwise you can't track when a provider is removed.

Unfortunately that is not going to work. The function
of_genpd_remove_tail() (patch #10) uses the ->provider member to remove
the last domain for the given provider and of_genpd_del_provider() must
be called before hand.

Cheers
Jon

-- 
nvpublic

[PATCH 09/10] PM / Domains: Store the provider in the PM domain structure

[Ulf Hansson]

[...]

>>
>> I also think you should extend this change, to also make the
>> of_genpd_del_provider() API to reset the genpd->provider = NULL.
>> Otherwise you can't track when a provider is removed.
>
> Unfortunately that is not going to work. The function
> of_genpd_remove_tail() (patch #10) uses the ->provider member to remove
> the last domain for the given provider and of_genpd_del_provider() must
> be called before hand.

Yes, of course - you are right!

Kind regards
Uffe

[PATCH 10/10] PM / Domains: Add support for removing nested PM domains by provider

[Jon Hunter]

If a device supports PM domains that are subdomains of another PM
domain, then the PM domains should be removed in reverse order to
ensure that the subdomains are removed first. Furthermore, if there is
more than one provider, then there needs to be a way to remove the
domains in reverse order for a specific provider.

Add the function of_genpd_remove_tail() to remove the last PM domain
added by a given PM domain provider and return the generic_pm_domain
structure for the PM domain that was removed.

A PM domain should only be removed once the associated PM domain
provider has been removed from the list of providers. Otherwise, it
could be possible for a client to be associated with a PM domain that
could have been removed. Add a helper function to verify if the PM
domain provider is present and only allow a PM domain to be removed if
the provider has been removed.

The function of_genpd_remove_tail() must hold the gpd_list_lock while
finding and removing a PM domain. It is natural for
of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
PM domain is found to remove it. However, pm_genpd_remove(), also
acquires the gpd_list_lock. Therefore, move the core of the function
pm_genpd_remove() to a new function genpd_remove() which does not
acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
and of_genpd_remove_tail().

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
 include/linux/pm_domain.h   |  7 ++++
 2 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0bc145e8e902..b6d1d0441a2d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1357,7 +1357,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
 /**
- * pm_genpd_remove - Remove a generic I/O PM domain
+ * genpd_remove - Remove a generic I/O PM domain
  * @genpd: Pointer to PM domain that is to be removed.
  *
  * To remove the PM domain, this function:
@@ -1366,9 +1366,10 @@ EXPORT_SYMBOL_GPL(pm_genpd_init);
  *  - Removes the PM domain from the list of registered PM domains.
  *
  * The PM domain will only be removed, if it is not a parent to any
- * other PM domain and has no devices associated with it.
+ * other PM domain and has no devices associated with it. Must be called
+ * with the gpd_list_lock held.
  */
-int pm_genpd_remove(struct generic_pm_domain *genpd)
+static int genpd_remove(struct generic_pm_domain *genpd)
 {
 	struct gpd_link *l, *link;
 	int ret = 0;
@@ -1376,12 +1377,10 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
-	mutex_lock(&gpd_list_lock);
 	mutex_lock(&genpd->lock);
 
 	if (!list_empty(&genpd->master_links) || genpd->device_count) {
 		mutex_unlock(&genpd->lock);
-		mutex_unlock(&gpd_list_lock);
 		pr_err("%s: unable to remove %s\n", __func__, genpd->name);
 		return -EBUSY;
 	}
@@ -1395,11 +1394,25 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
 	list_del(&genpd->gpd_list_node);
 	mutex_unlock(&genpd->lock);
 	cancel_work_sync(&genpd->power_off_work);
-	mutex_unlock(&gpd_list_lock);
 	pr_debug("%s: removed %s\n", __func__, genpd->name);
 
 	return ret;
 }
+
+/**
+ * pm_genpd_remove - Remove a generic I/O PM domain
+ * @genpd: Pointer to PM domain that is to be removed.
+ */
+int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	int ret;
+
+	mutex_lock(&gpd_list_lock);
+	ret = genpd_remove(genpd);
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(pm_genpd_remove);
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
@@ -1610,6 +1623,26 @@ void of_genpd_del_provider(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_genpd_del_provider);
 
 /**
+ * genpd_provider_present() - Verify if a PM domain provider is present
+ * @np: Device node pointer associated with the PM domain provider
+ */
+static bool genpd_provider_present(struct device_node *np)
+{
+	struct of_genpd_provider *cp;
+
+	mutex_lock(&of_genpd_mutex);
+	list_for_each_entry(cp, &of_genpd_providers, link) {
+		if (cp->node == np) {
+			mutex_unlock(&of_genpd_mutex);
+			return true;
+		}
+	}
+	mutex_unlock(&of_genpd_mutex);
+
+	return false;
+}
+
+/**
  * genpd_get_from_provider() - Look-up PM domain
  * @genpdspec: OF phandle args to use for look-up
  *
@@ -1713,6 +1746,48 @@ out:
 EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
 
 /**
+ * of_genpd_remove_tail - Remove the last PM domain registered for a provider
+ * @provider: Pointer to device structure associated with provider
+ *
+ * Find the last PM domain that was added by a particular provider and
+ * remove this PM domain from the list of PM domains. The provider is
+ * identified by the 'provider' device structure that is passed. The PM
+ * domain will only be removed, if the provider associated with domain
+ * has been removed.
+ *
+ * Returns a valid pointer to struct generic_pm_domain on success or
+ * ERR_PTR() on failure.
+ */
+struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
+{
+	struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
+	int ret;
+
+	if (IS_ERR_OR_NULL(np))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+		if (gpd->provider == &np->fwnode) {
+			if (!genpd_provider_present(np)) {
+				ret = genpd_remove(gpd);
+				genpd = ret ? ERR_PTR(ret) : gpd;
+				break;
+			}
+
+			pr_warn("Provider maybe present, unable to remove %s\n",
+				genpd->name);
+			genpd = ERR_PTR(-EBUSY);
+			break;
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return genpd;
+}
+EXPORT_SYMBOL_GPL(of_genpd_remove_tail);
+
+/**
  * genpd_dev_pm_detach - Detach a device from its PM domain.
  * @dev: Device to detach.
  * @power_off: Currently not used
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 4aa285e44eb0..253f348b0ee9 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -203,6 +203,7 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
 			       struct device *dev);
 extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
 				  struct of_phandle_args *new_subdomain);
+extern struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np);
 
 int genpd_dev_pm_attach(struct device *dev);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
@@ -236,6 +237,12 @@ static inline int genpd_dev_pm_attach(struct device *dev)
 {
 	return -ENODEV;
 }
+
+static inline
+struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 #ifdef CONFIG_PM
-- 
2.1.4

[PATCH 10/10] PM / Domains: Add support for removing nested PM domains by provider

[Ulf Hansson]

On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> If a device supports PM domains that are subdomains of another PM
> domain, then the PM domains should be removed in reverse order to
> ensure that the subdomains are removed first. Furthermore, if there is
> more than one provider, then there needs to be a way to remove the
> domains in reverse order for a specific provider.
>
> Add the function of_genpd_remove_tail() to remove the last PM domain
> added by a given PM domain provider and return the generic_pm_domain
> structure for the PM domain that was removed.
>
> A PM domain should only be removed once the associated PM domain
> provider has been removed from the list of providers. Otherwise, it
> could be possible for a client to be associated with a PM domain that
> could have been removed. Add a helper function to verify if the PM
> domain provider is present and only allow a PM domain to be removed if
> the provider has been removed.
>
> The function of_genpd_remove_tail() must hold the gpd_list_lock while
> finding and removing a PM domain. It is natural for
> of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
> PM domain is found to remove it. However, pm_genpd_remove(), also
> acquires the gpd_list_lock. Therefore, move the core of the function
> pm_genpd_remove() to a new function genpd_remove() which does not
> acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
> and of_genpd_remove_tail().
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pm_domain.h   |  7 ++++
>  2 files changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0bc145e8e902..b6d1d0441a2d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1357,7 +1357,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>
>  /**
> - * pm_genpd_remove - Remove a generic I/O PM domain
> + * genpd_remove - Remove a generic I/O PM domain
>   * @genpd: Pointer to PM domain that is to be removed.
>   *
>   * To remove the PM domain, this function:
> @@ -1366,9 +1366,10 @@ EXPORT_SYMBOL_GPL(pm_genpd_init);
>   *  - Removes the PM domain from the list of registered PM domains.
>   *
>   * The PM domain will only be removed, if it is not a parent to any
> - * other PM domain and has no devices associated with it.
> + * other PM domain and has no devices associated with it. Must be called
> + * with the gpd_list_lock held.
>   */
> -int pm_genpd_remove(struct generic_pm_domain *genpd)
> +static int genpd_remove(struct generic_pm_domain *genpd)
>  {
>         struct gpd_link *l, *link;
>         int ret = 0;
> @@ -1376,12 +1377,10 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
>         if (IS_ERR_OR_NULL(genpd))
>                 return -EINVAL;
>
> -       mutex_lock(&gpd_list_lock);
>         mutex_lock(&genpd->lock);
>
>         if (!list_empty(&genpd->master_links) || genpd->device_count) {
>                 mutex_unlock(&genpd->lock);
> -               mutex_unlock(&gpd_list_lock);
>                 pr_err("%s: unable to remove %s\n", __func__, genpd->name);
>                 return -EBUSY;
>         }
> @@ -1395,11 +1394,25 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
>         list_del(&genpd->gpd_list_node);
>         mutex_unlock(&genpd->lock);
>         cancel_work_sync(&genpd->power_off_work);
> -       mutex_unlock(&gpd_list_lock);
>         pr_debug("%s: removed %s\n", __func__, genpd->name);
>
>         return ret;
>  }
> +
> +/**
> + * pm_genpd_remove - Remove a generic I/O PM domain
> + * @genpd: Pointer to PM domain that is to be removed.
> + */
> +int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> +       int ret;
> +
> +       mutex_lock(&gpd_list_lock);
> +       ret = genpd_remove(genpd);
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
> +}
>  EXPORT_SYMBOL_GPL(pm_genpd_remove);

All above changes could have been made already in the patch when
adding the pm_genpd_remove() API. Could you please fold these changes
into that patch instead?

>
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> @@ -1610,6 +1623,26 @@ void of_genpd_del_provider(struct device_node *np)
>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>
>  /**
> + * genpd_provider_present() - Verify if a PM domain provider is present
> + * @np: Device node pointer associated with the PM domain provider
> + */
> +static bool genpd_provider_present(struct device_node *np)
> +{
> +       struct of_genpd_provider *cp;
> +
> +       mutex_lock(&of_genpd_mutex);
> +       list_for_each_entry(cp, &of_genpd_providers, link) {
> +               if (cp->node == np) {
> +                       mutex_unlock(&of_genpd_mutex);
> +                       return true;
> +               }
> +       }
> +       mutex_unlock(&of_genpd_mutex);
> +
> +       return false;
> +}
> +
> +/**
>   * genpd_get_from_provider() - Look-up PM domain
>   * @genpdspec: OF phandle args to use for look-up
>   *
> @@ -1713,6 +1746,48 @@ out:
>  EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
>
>  /**
> + * of_genpd_remove_tail - Remove the last PM domain registered for a provider
> + * @provider: Pointer to device structure associated with provider

The naming of this function would be okay, if we only have added
genpds in the gpd_list by using list_add_tail(), although we don't.
Instead we use list_add() and put them first in the list.

So, unless we change to use list_add_tail() when adding genpds (I
assume we can do that!), I would rather change the name of this
function to of_genpd_remove_first().

What option do you prefer?

> + *
> + * Find the last PM domain that was added by a particular provider and
> + * remove this PM domain from the list of PM domains. The provider is
> + * identified by the 'provider' device structure that is passed. The PM
> + * domain will only be removed, if the provider associated with domain
> + * has been removed.
> + *
> + * Returns a valid pointer to struct generic_pm_domain on success or
> + * ERR_PTR() on failure.
> + */
> +struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
> +{
> +       struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
> +       int ret;
> +
> +       if (IS_ERR_OR_NULL(np))
> +               return ERR_PTR(-EINVAL);
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +               if (gpd->provider == &np->fwnode) {
> +                       if (!genpd_provider_present(np)) {
> +                               ret = genpd_remove(gpd);
> +                               genpd = ret ? ERR_PTR(ret) : gpd;
> +                               break;
> +                       }

Maybe use an "else" here instead to avoid the double "break;".

> +                       pr_warn("Provider maybe present, unable to remove %s\n",
> +                               genpd->name);
> +                       genpd = ERR_PTR(-EBUSY);
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return genpd;
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_remove_tail);
> +
> +/**
>   * genpd_dev_pm_detach - Detach a device from its PM domain.
>   * @dev: Device to detach.
>   * @power_off: Currently not used
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 4aa285e44eb0..253f348b0ee9 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -203,6 +203,7 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
>                                struct device *dev);
>  extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>                                   struct of_phandle_args *new_subdomain);
> +extern struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np);
>
>  int genpd_dev_pm_attach(struct device *dev);
>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> @@ -236,6 +237,12 @@ static inline int genpd_dev_pm_attach(struct device *dev)
>  {
>         return -ENODEV;
>  }
> +
> +static inline
> +struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>  #ifdef CONFIG_PM
> --
> 2.1.4
>

Kind regards
Uffe

[PATCH 10/10] PM / Domains: Add support for removing nested PM domains by provider

[Jon Hunter]

On 08/09/16 13:30, Ulf Hansson wrote:
> On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>> If a device supports PM domains that are subdomains of another PM
>> domain, then the PM domains should be removed in reverse order to
>> ensure that the subdomains are removed first. Furthermore, if there is
>> more than one provider, then there needs to be a way to remove the
>> domains in reverse order for a specific provider.
>>
>> Add the function of_genpd_remove_tail() to remove the last PM domain
>> added by a given PM domain provider and return the generic_pm_domain
>> structure for the PM domain that was removed.
>>
>> A PM domain should only be removed once the associated PM domain
>> provider has been removed from the list of providers. Otherwise, it
>> could be possible for a client to be associated with a PM domain that
>> could have been removed. Add a helper function to verify if the PM
>> domain provider is present and only allow a PM domain to be removed if
>> the provider has been removed.
>>
>> The function of_genpd_remove_tail() must hold the gpd_list_lock while
>> finding and removing a PM domain. It is natural for
>> of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
>> PM domain is found to remove it. However, pm_genpd_remove(), also
>> acquires the gpd_list_lock. Therefore, move the core of the function
>> pm_genpd_remove() to a new function genpd_remove() which does not
>> acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
>> and of_genpd_remove_tail().
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/pm_domain.h   |  7 ++++
>>  2 files changed, 88 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 0bc145e8e902..b6d1d0441a2d 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1357,7 +1357,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>>
>>  /**
>> - * pm_genpd_remove - Remove a generic I/O PM domain
>> + * genpd_remove - Remove a generic I/O PM domain
>>   * @genpd: Pointer to PM domain that is to be removed.
>>   *
>>   * To remove the PM domain, this function:
>> @@ -1366,9 +1366,10 @@ EXPORT_SYMBOL_GPL(pm_genpd_init);
>>   *  - Removes the PM domain from the list of registered PM domains.
>>   *
>>   * The PM domain will only be removed, if it is not a parent to any
>> - * other PM domain and has no devices associated with it.
>> + * other PM domain and has no devices associated with it. Must be called
>> + * with the gpd_list_lock held.
>>   */
>> -int pm_genpd_remove(struct generic_pm_domain *genpd)
>> +static int genpd_remove(struct generic_pm_domain *genpd)
>>  {
>>         struct gpd_link *l, *link;
>>         int ret = 0;
>> @@ -1376,12 +1377,10 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
>>         if (IS_ERR_OR_NULL(genpd))
>>                 return -EINVAL;
>>
>> -       mutex_lock(&gpd_list_lock);
>>         mutex_lock(&genpd->lock);
>>
>>         if (!list_empty(&genpd->master_links) || genpd->device_count) {
>>                 mutex_unlock(&genpd->lock);
>> -               mutex_unlock(&gpd_list_lock);
>>                 pr_err("%s: unable to remove %s\n", __func__, genpd->name);
>>                 return -EBUSY;
>>         }
>> @@ -1395,11 +1394,25 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
>>         list_del(&genpd->gpd_list_node);
>>         mutex_unlock(&genpd->lock);
>>         cancel_work_sync(&genpd->power_off_work);
>> -       mutex_unlock(&gpd_list_lock);
>>         pr_debug("%s: removed %s\n", __func__, genpd->name);
>>
>>         return ret;
>>  }
>> +
>> +/**
>> + * pm_genpd_remove - Remove a generic I/O PM domain
>> + * @genpd: Pointer to PM domain that is to be removed.
>> + */
>> +int pm_genpd_remove(struct generic_pm_domain *genpd)
>> +{
>> +       int ret;
>> +
>> +       mutex_lock(&gpd_list_lock);
>> +       ret = genpd_remove(genpd);
>> +       mutex_unlock(&gpd_list_lock);
>> +
>> +       return ret;
>> +}
>>  EXPORT_SYMBOL_GPL(pm_genpd_remove);
> 
> All above changes could have been made already in the patch when
> adding the pm_genpd_remove() API. Could you please fold these changes
> into that patch instead?

Ok. I was not sure if it would seem odd to add pm_genpd_remove() and
genpd_remove() in the same patch because pm_genpd_remove() is the only
user of genpd_remove(). However, it would simplify the diff and so I am
fine with that.

>>
>>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>> @@ -1610,6 +1623,26 @@ void of_genpd_del_provider(struct device_node *np)
>>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>
>>  /**
>> + * genpd_provider_present() - Verify if a PM domain provider is present
>> + * @np: Device node pointer associated with the PM domain provider
>> + */
>> +static bool genpd_provider_present(struct device_node *np)
>> +{
>> +       struct of_genpd_provider *cp;
>> +
>> +       mutex_lock(&of_genpd_mutex);
>> +       list_for_each_entry(cp, &of_genpd_providers, link) {
>> +               if (cp->node == np) {
>> +                       mutex_unlock(&of_genpd_mutex);
>> +                       return true;
>> +               }
>> +       }
>> +       mutex_unlock(&of_genpd_mutex);
>> +
>> +       return false;
>> +}
>> +
>> +/**
>>   * genpd_get_from_provider() - Look-up PM domain
>>   * @genpdspec: OF phandle args to use for look-up
>>   *
>> @@ -1713,6 +1746,48 @@ out:
>>  EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
>>
>>  /**
>> + * of_genpd_remove_tail - Remove the last PM domain registered for a provider
>> + * @provider: Pointer to device structure associated with provider
> 
> The naming of this function would be okay, if we only have added
> genpds in the gpd_list by using list_add_tail(), although we don't.
> Instead we use list_add() and put them first in the list.
> 
> So, unless we change to use list_add_tail() when adding genpds (I
> assume we can do that!), I would rather change the name of this
> function to of_genpd_remove_first().
> 
> What option do you prefer?

I think that I would prefer either of_genpd_remove_last() or
of_genpd_remove_one(). Although _first is accurate from the list
perspective it seems odd from the user perspective. I think that _last
is more meaningful as we are removing the last that was added regardless
or how things appear on the list. Alternatively, _one could be a good
compromise.


>> + *
>> + * Find the last PM domain that was added by a particular provider and
>> + * remove this PM domain from the list of PM domains. The provider is
>> + * identified by the 'provider' device structure that is passed. The PM
>> + * domain will only be removed, if the provider associated with domain
>> + * has been removed.
>> + *
>> + * Returns a valid pointer to struct generic_pm_domain on success or
>> + * ERR_PTR() on failure.
>> + */
>> +struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
>> +{
>> +       struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
>> +       int ret;
>> +
>> +       if (IS_ERR_OR_NULL(np))
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       mutex_lock(&gpd_list_lock);
>> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
>> +               if (gpd->provider == &np->fwnode) {
>> +                       if (!genpd_provider_present(np)) {
>> +                               ret = genpd_remove(gpd);
>> +                               genpd = ret ? ERR_PTR(ret) : gpd;
>> +                               break;
>> +                       }
> 
> Maybe use an "else" here instead to avoid the double "break;".

Ok.

Cheers
Jon

-- 
nvpublic

[PATCH 10/10] PM / Domains: Add support for removing nested PM domains by provider

[Ulf Hansson]

[...]

>>>  /**
>>> + * of_genpd_remove_tail - Remove the last PM domain registered for a provider
>>> + * @provider: Pointer to device structure associated with provider
>>
>> The naming of this function would be okay, if we only have added
>> genpds in the gpd_list by using list_add_tail(), although we don't.
>> Instead we use list_add() and put them first in the list.
>>
>> So, unless we change to use list_add_tail() when adding genpds (I
>> assume we can do that!), I would rather change the name of this
>> function to of_genpd_remove_first().
>>
>> What option do you prefer?
>
> I think that I would prefer either of_genpd_remove_last() or
> of_genpd_remove_one(). Although _first is accurate from the list
> perspective it seems odd from the user perspective. I think that _last
> is more meaningful as we are removing the last that was added regardless
> or how things appear on the list. Alternatively, _one could be a good
> compromise.

I am fine with of_genpd_remove_last().

[...]

Kind regards
Uffe

[PATCH 00/10] PM / Domains: Add support for removing PM domains

[Jon Hunter]

Hi Ulf,

On 16/08/16 10:49, Jon Hunter wrote:
> In order to safely remove PM domains there are a few changes that need to be
> made to ensure that no one is holding an external reference to a PM domain
> after it has been removed. One solution, implemented here, solves this by
> eliminating external references to PM domain.
> 
> This is a follow-up to the initial RFC I sent out [0].
> 
> Changes from initial RFC:
> - Renamed functions made static per Ulf's feedback.
> - Added patch to clean-up provider/xlate APIs per Ulf's feedback
> - Re-worked and simplified the association between PM domains and PM
>   domain providers. Dropped the 'provider_data' variable from the
>   generic_pm_domain structure in favour of using the fwnode_handle.
> - Split patch for removing PM domains into multiple patches per Ulf's
>   feedback.

Let me know if you have any feedback on this. Please note I will be out
next week.

Cheers
Jon

-- 
nvpublic