[RFC PATCH 0/8] PM / Domains: Add support for removing PM domains

[RFC PATCH 0/8] 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. Another alternative would be
to employ some reference counting for the PM domains, however, I did not see
a good reason for allowing external references in the first place (as always
there could be something that I have over-looked!).

Jon Hunter (8):
  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
  PM / Domains: Verify the PM domain is present when adding a provider
  PM / Domains: Remove a provider by referencing the data pointer
  PM / Domains: Prepare for adding support to remove PM domains
  PM / Domains: Add support for removing PM domains

 arch/arm/mach-exynos/pm_domains.c |  23 +--
 drivers/base/power/domain.c       | 324 +++++++++++++++++++++++++++++++++++---
 drivers/staging/board/board.c     |   9 +-
 include/linux/pm_domain.h         |  55 ++++---
 4 files changed, 348 insertions(+), 63 deletions(-)

-- 
2.1.4

[RFC PATCH 1/8] 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   | 16 ++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 5bdb42bf40f4..b24893499454 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1706,6 +1706,50 @@ 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);
+}
+
+/**
+ * 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);
+}
+
+/**
  * 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 49cd8890b873..2b6ee670b231 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -210,6 +210,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 */
@@ -229,6 +233,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_pm_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

[RFC PATCH 1/8] PM / Domains: Add new helper functions for device-tree

[Jon Hunter]

On 04/03/16 11:23, Jon Hunter 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>
> ---
>  drivers/base/power/domain.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 16 ++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 5bdb42bf40f4..b24893499454 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1706,6 +1706,50 @@ 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);
> +}
> +
> +/**
> + * 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);
> +}
> +
> +/**
>   * 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 49cd8890b873..2b6ee670b231 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -210,6 +210,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 */
> @@ -229,6 +233,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_pm_genpd_add_device(struct of_phandle_args *args,
> +					 struct device *dev)

This should be of_genpd_add_device. Will fix.

Jon

-- 
nvpublic

[RFC PATCH 1/8] PM / Domains: Add new helper functions for device-tree

[Jon Hunter]

Hi Ulf,

On 04/03/16 11:23, Jon Hunter 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.

While we are at it, does it make sense to add helpers for
of_genpd_remove_device/subdomain() as well? Seems that these could be
useful for doing the inverse when cleaning up.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 1/8] PM / Domains: Add new helper functions for device-tree

[Ulf Hansson]

On 22 June 2016 at 16:58, Jon Hunter <jonathanh@nvidia.com> wrote:
> Hi Ulf,
>
> On 04/03/16 11:23, Jon Hunter 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.
>
> While we are at it, does it make sense to add helpers for
> of_genpd_remove_device/subdomain() as well? Seems that these could be
> useful for doing the inverse when cleaning up.

I would prefer if we could avoid adding new APIs until we really see
the need for it.

Moreover, I would like to avoid us adding OF specific APIs, unless
those can be really justified.
Hope that made sense.

Kind regards
Uffe

[RFC PATCH 1/8] PM / Domains: Add new helper functions for device-tree

[Jon Hunter]

On 22/06/16 16:08, Ulf Hansson wrote:
> On 22 June 2016 at 16:58, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Hi Ulf,
>>
>> On 04/03/16 11:23, Jon Hunter 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.
>>
>> While we are at it, does it make sense to add helpers for
>> of_genpd_remove_device/subdomain() as well? Seems that these could be
>> useful for doing the inverse when cleaning up.
> 
> I would prefer if we could avoid adding new APIs until we really see
> the need for it.
> 
> Moreover, I would like to avoid us adding OF specific APIs, unless
> those can be really justified.
> Hope that made sense.

Yes makes sense. However, after this series, the
pm_genpd_remove_device/subdomain really become provider only APIs
because clients can no longer get access to the genpd struct. Although
today none of the users of the new of_genpd_add_device/subdomain do any
clean-up on failure, it is possible that someone may. Therefore, I was
thinking that we should have a way for a client to remove a subdomain or
device it has added.

Does that make sense? I guess we could always add those as needed.

I am looking at a use-case for usb where we are populating the
pm-domains at runtime and we may need to clean-up on failure. However, I
can always wait to add more APIs until we really need them.

Let me know what you think about my response to patch 6/8.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 1/8] PM / Domains: Add new helper functions for device-tree

[Ulf Hansson]

On 22 June 2016 at 17:22, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 22/06/16 16:08, Ulf Hansson wrote:
>> On 22 June 2016 at 16:58, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Hi Ulf,
>>>
>>> On 04/03/16 11:23, Jon Hunter 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.
>>>
>>> While we are at it, does it make sense to add helpers for
>>> of_genpd_remove_device/subdomain() as well? Seems that these could be
>>> useful for doing the inverse when cleaning up.
>>
>> I would prefer if we could avoid adding new APIs until we really see
>> the need for it.
>>
>> Moreover, I would like to avoid us adding OF specific APIs, unless
>> those can be really justified.
>> Hope that made sense.
>
> Yes makes sense. However, after this series, the
> pm_genpd_remove_device/subdomain really become provider only APIs
> because clients can no longer get access to the genpd struct. Although
> today none of the users of the new of_genpd_add_device/subdomain do any
> clean-up on failure, it is possible that someone may. Therefore, I was
> thinking that we should have a way for a client to remove a subdomain or
> device it has added.
>
> Does that make sense? I guess we could always add those as needed.
>
> I am looking at a use-case for usb where we are populating the
> pm-domains at runtime and we may need to clean-up on failure. However, I
> can always wait to add more APIs until we really need them.

You may very well be right that these will be needed.

How about posting those patches as separate change and on top of the
series. Then we can decide if we want to pick them now or later.

>
> Let me know what you think about my response to patch 6/8.

I am on it.

Kind regards
Uffe

[RFC PATCH 2/8] 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>
---
 arch/arm/mach-exynos/pm_domains.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 7c21760f590f..871f72a292ae 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -193,29 +193,22 @@ no_clk:
 
 	/* Assign the child power domains to their parents */
 	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
-		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

[RFC PATCH 3/8] 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

[RFC PATCH 4/8] PM / Domains: Don't expose generic_pm_domain structure

[Jon Hunter]

There should be no need to expose the generic_pm_domain structure 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.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b24893499454..c2ba1d6dbad3 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 *pm_genpd_lookup_dev(struct device *dev)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;
 
@@ -1680,7 +1680,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 *of_genpd_get_from_provider(
 					struct of_phandle_args *genpdspec)
 {
 	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
@@ -1703,7 +1703,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
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 2b6ee670b231..510512d5390e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -119,7 +119,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);
@@ -141,10 +140,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)
@@ -201,9 +196,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);
@@ -224,12 +216,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

[RFC PATCH 4/8] PM / Domains: Don't expose generic_pm_domain structure

[Ulf Hansson]

On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
> There should be no need to expose the generic_pm_domain structure 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.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c |  5 ++---
>  include/linux/pm_domain.h   | 14 --------------
>  2 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b24893499454..c2ba1d6dbad3 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 *pm_genpd_lookup_dev(struct device *dev)

Please rename function to genpd_lookup_dev() to follow naming rules
for static functions.

>  {
>         struct generic_pm_domain *genpd = NULL, *gpd;
>
> @@ -1680,7 +1680,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 *of_genpd_get_from_provider(
>                                         struct of_phandle_args *genpdspec)
>  {

Please rename function to genpd_get_from_provider() to follow naming
rules for static functions.

>         struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
> @@ -1703,7 +1703,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
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 2b6ee670b231..510512d5390e 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -119,7 +119,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);
> @@ -141,10 +140,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)
> @@ -201,9 +196,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);
> @@ -224,12 +216,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
>

Besides the minor nitpicks, this looks good to me!

Kind regards
Uffe

[RFC PATCH 5/8] 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
the PM domain is actually present in the system. Naturally, the PM domain
provider should not be registered if the PM domain has not been added.
Nonetheless, to verify that the PM domain associated with a provider is
present, store the 'provider_data' in the PM domain structure when adding
the provider and make sure that the PM domain is found the list of PM
domains registered.

The of_genpd_add_provider_simple() and of_genpd_add_provider_onecell()
functions have been updated to store the 'provider_data' by default to
avoid having to modify all the current PM domain provider
implementations.

By doing this, we can also verify that the provider has been removed
from the list of providers before removing a PM domain.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index c2ba1d6dbad3..72055fef6256 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1570,6 +1570,24 @@ static LIST_HEAD(of_genpd_providers);
 static DEFINE_MUTEX(of_genpd_mutex);
 
 /**
+ * pm_genpd_provider_present - Check if the provider's PM domain is present.
+ * @data: Provider data associated with the PM domain.
+ */
+static bool pm_genpd_provider_present(void *data)
+{
+	struct generic_pm_domain *gpd;
+
+	if (!data)
+		return false;
+
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node)
+		if (gpd->provider_data == data)
+			return true;
+
+	return false;
+}
+
+/**
  * __of_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
@@ -1625,9 +1643,15 @@ EXPORT_SYMBOL_GPL(__of_genpd_xlate_onecell);
  * @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.
+ *
+ * The PM domain assocaited with the provider must have the
+ * 'provider_data' member of the PM domain structure populated with the
+ * same data pointer passed to this function. This is used to verify
+ * that the PM domain associated with the provider is present in the
+ * list of registered PM domains.
  */
-int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
-			void *data)
+static int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
+				   void *data)
 {
 	struct of_genpd_provider *cp;
 
@@ -1635,6 +1659,13 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
 	if (!cp)
 		return -ENOMEM;
 
+	mutex_lock(&gpd_list_lock);
+
+	if (!pm_genpd_provider_present(data)) {
+		mutex_unlock(&gpd_list_lock);
+		return -EINVAL;
+	}
+
 	cp->node = of_node_get(np);
 	cp->data = data;
 	cp->xlate = xlate;
@@ -1642,11 +1673,48 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
 	mutex_lock(&of_genpd_mutex);
 	list_add(&cp->link, &of_genpd_providers);
 	mutex_unlock(&of_genpd_mutex);
+	mutex_unlock(&gpd_list_lock);
 	pr_debug("Added domain provider from %s\n", np->full_name);
 
 	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)
+{
+	if (!np || !genpd)
+		return -EINVAL;
+
+	genpd->provider_data = genpd;
+
+	return __of_genpd_add_provider(np, __of_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)
+{
+	unsigned int i;
+
+	if (!np || !data)
+		return -EINVAL;
+
+	for (i = 0; i < data->num_domains; i++)
+		data->domains[i]->provider_data = data;
+
+	return __of_genpd_add_provider(np, __of_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 510512d5390e..bed84413546f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -53,6 +53,7 @@ struct generic_pm_domain {
 	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
+	void *provider_data;
 	const char *name;
 	atomic_t sd_count;	/* Number of subdomains with power "on" */
 	enum gpd_status status;	/* Current state of the domain */
@@ -193,8 +194,10 @@ 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,
@@ -235,18 +238,18 @@ static inline int genpd_dev_pm_attach(struct device *dev)
 {
 	return -ENODEV;
 }
-#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);
+	return -ENOTSUPP;
 }
 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);
+	return -ENOTSUPP;
 }
+#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
-- 
2.1.4

[RFC PATCH 5/8] PM / Domains: Verify the PM domain is present when adding a provider

[Ulf Hansson]

On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
> When a PM domain provider is added, there is currently no way to tell if
> the PM domain is actually present in the system. Naturally, the PM domain
> provider should not be registered if the PM domain has not been added.
> Nonetheless, to verify that the PM domain associated with a provider is
> present, store the 'provider_data' in the PM domain structure when adding
> the provider and make sure that the PM domain is found the list of PM
> domains registered.
>
> The of_genpd_add_provider_simple() and of_genpd_add_provider_onecell()
> functions have been updated to store the 'provider_data' by default to
> avoid having to modify all the current PM domain provider
> implementations.
>
> By doing this, we can also verify that the provider has been removed
> from the list of providers before removing a PM domain.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 74 +++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pm_domain.h   | 13 +++++---
>  2 files changed, 79 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index c2ba1d6dbad3..72055fef6256 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1570,6 +1570,24 @@ static LIST_HEAD(of_genpd_providers);
>  static DEFINE_MUTEX(of_genpd_mutex);
>
>  /**
> + * pm_genpd_provider_present - Check if the provider's PM domain is present.
> + * @data: Provider data associated with the PM domain.
> + */
> +static bool pm_genpd_provider_present(void *data)

Please rename to genpd_provider_present().

> +{
> +       struct generic_pm_domain *gpd;
> +
> +       if (!data)
> +               return false;
> +
> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node)
> +               if (gpd->provider_data == data)
> +                       return true;
> +
> +       return false;
> +}
> +
> +/**
>   * __of_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
> @@ -1625,9 +1643,15 @@ EXPORT_SYMBOL_GPL(__of_genpd_xlate_onecell);
>   * @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.
> + *
> + * The PM domain assocaited with the provider must have the
> + * 'provider_data' member of the PM domain structure populated with the
> + * same data pointer passed to this function. This is used to verify
> + * that the PM domain associated with the provider is present in the
> + * list of registered PM domains.
>   */
> -int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> -                       void *data)
> +static int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> +                                  void *data)

Please rename to genpd_add_provider().

>  {
>         struct of_genpd_provider *cp;
>
> @@ -1635,6 +1659,13 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>         if (!cp)
>                 return -ENOMEM;
>
> +       mutex_lock(&gpd_list_lock);
> +
> +       if (!pm_genpd_provider_present(data)) {
> +               mutex_unlock(&gpd_list_lock);
> +               return -EINVAL;
> +       }
> +
>         cp->node = of_node_get(np);
>         cp->data = data;
>         cp->xlate = xlate;
> @@ -1642,11 +1673,48 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>         mutex_lock(&of_genpd_mutex);
>         list_add(&cp->link, &of_genpd_providers);
>         mutex_unlock(&of_genpd_mutex);
> +       mutex_unlock(&gpd_list_lock);
>         pr_debug("Added domain provider from %s\n", np->full_name);
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(__of_genpd_add_provider);

I agree that it makes sense to remove this API and to turn the
function into becoming static. Although perhaps you should do that as
a clean up patch that comes prior $subject patch instead!?

The clean up also includes the exporting of the
of_genpd_add_provider_onecell() and the of_genpd_add_provider_simple()
APIs.

... and while you do these cleanups, you could also remove the
exported functions __of_genpd_xlate_simple() and
__of_genpd_xlate_onecell() as those should be internal to genpd.

> +
> +/**
> + * 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)
> +{
> +       if (!np || !genpd)
> +               return -EINVAL;
> +
> +       genpd->provider_data = genpd;
> +
> +       return __of_genpd_add_provider(np, __of_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)
> +{
> +       unsigned int i;
> +
> +       if (!np || !data)
> +               return -EINVAL;
> +
> +       for (i = 0; i < data->num_domains; i++)
> +               data->domains[i]->provider_data = data;
> +
> +       return __of_genpd_add_provider(np, __of_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 510512d5390e..bed84413546f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -53,6 +53,7 @@ struct generic_pm_domain {
>         struct mutex lock;
>         struct dev_power_governor *gov;
>         struct work_struct power_off_work;
> +       void *provider_data;

As per comment on patch 6/8. Couldn't we store the provider's device
node here instead?

>         const char *name;
>         atomic_t sd_count;      /* Number of subdomains with power "on" */
>         enum gpd_status status; /* Current state of the domain */
> @@ -193,8 +194,10 @@ 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,
> @@ -235,18 +238,18 @@ static inline int genpd_dev_pm_attach(struct device *dev)
>  {
>         return -ENODEV;
>  }
> -#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);
> +       return -ENOTSUPP;
>  }
>  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);
> +       return -ENOTSUPP;
>  }
> +#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>  #ifdef CONFIG_PM
>  extern int dev_pm_domain_attach(struct device *dev, bool power_on);
> --
> 2.1.4
>

Kind regards
Uffe

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Jon Hunter]

To remove a PM domain from the system, it is necessary to ensure
that any PM domain providers associated with the PM domain have
been removed. Otherwise it could be possible to obtain a pointer
to a PM domain structure that has been removed.

PM domains now have a reference to the pointer for the PM domain
provider's data variable. Add a function so that a PM domain can
remove a PM domain provider by referencing the data pointer.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 72055fef6256..438885f2455f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_genpd_del_provider);
 
 /**
+ * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
+ * @data: Pointer to the data associated with the PM domain provider
+ *
+ * Look up a PM domain provider based upon a pointer to it's data and
+ * remove the PM domain provider from the list of providers.
+ */
+void of_genpd_del_provider_by_data(void *data)
+{
+	struct of_genpd_provider *c, *cp;
+
+	mutex_lock(&of_genpd_mutex);
+	list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
+		if (cp->data == data) {
+			list_del(&cp->link);
+			of_node_put(cp->node);
+			kfree(cp);
+			break;
+		}
+	}
+	mutex_unlock(&of_genpd_mutex);
+}
+EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
+
+/**
  * of_genpd_get_from_provider() - Look-up PM domain
  * @genpdspec: OF phandle args to use for look-up
  *
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index bed84413546f..7b7921a65cb0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
 int of_genpd_add_provider_onecell(struct device_node *np,
 				  struct genpd_onecell_data *data);
 void of_genpd_del_provider(struct device_node *np);
+void of_genpd_del_provider_by_data(void *data);
 struct generic_pm_domain *__of_genpd_xlate_simple(
 					struct of_phandle_args *genpdspec,
 					void *data);
@@ -218,6 +219,7 @@ static inline int __of_genpd_add_provider(struct device_node *np,
 	return 0;
 }
 static inline void of_genpd_del_provider(struct device_node *np) {}
+static inline void of_genpd_del_provider_by_data(void *data) {}
 
 #define __of_genpd_xlate_simple		NULL
 #define __of_genpd_xlate_onecell	NULL
-- 
2.1.4

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Ulf Hansson]

On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
> To remove a PM domain from the system, it is necessary to ensure
> that any PM domain providers associated with the PM domain have
> been removed. Otherwise it could be possible to obtain a pointer
> to a PM domain structure that has been removed.
>
> PM domains now have a reference to the pointer for the PM domain
> provider's data variable. Add a function so that a PM domain can
> remove a PM domain provider by referencing the data pointer.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  2 ++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 72055fef6256..438885f2455f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>
>  /**
> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
> + * @data: Pointer to the data associated with the PM domain provider
> + *
> + * Look up a PM domain provider based upon a pointer to it's data and
> + * remove the PM domain provider from the list of providers.
> + */
> +void of_genpd_del_provider_by_data(void *data)
> +{
> +       struct of_genpd_provider *c, *cp;
> +
> +       mutex_lock(&of_genpd_mutex);
> +       list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
> +               if (cp->data == data) {
> +                       list_del(&cp->link);
> +                       of_node_put(cp->node);
> +                       kfree(cp);
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&of_genpd_mutex);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
> +
> +/**
>   * of_genpd_get_from_provider() - Look-up PM domain
>   * @genpdspec: OF phandle args to use for look-up
>   *
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index bed84413546f..7b7921a65cb0 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>  int of_genpd_add_provider_onecell(struct device_node *np,
>                                   struct genpd_onecell_data *data);
>  void of_genpd_del_provider(struct device_node *np);

There's currently only one user of of_genpd_del_provider().

Could this patch just convert that user to the new API, so we don't
need to keep both the legacy and new one?

I guess we could then just stick to the name "of_genpd_del_provider()".

> +void of_genpd_del_provider_by_data(void *data);
>  struct generic_pm_domain *__of_genpd_xlate_simple(
>                                         struct of_phandle_args *genpdspec,
>                                         void *data);
> @@ -218,6 +219,7 @@ static inline int __of_genpd_add_provider(struct device_node *np,
>         return 0;
>  }
>  static inline void of_genpd_del_provider(struct device_node *np) {}
> +static inline void of_genpd_del_provider_by_data(void *data) {}
>
>  #define __of_genpd_xlate_simple                NULL
>  #define __of_genpd_xlate_onecell       NULL
> --
> 2.1.4
>

Kind regards
Uffe

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Jon Hunter]

On 15/06/16 15:38, Ulf Hansson wrote:
> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
>> To remove a PM domain from the system, it is necessary to ensure
>> that any PM domain providers associated with the PM domain have
>> been removed. Otherwise it could be possible to obtain a pointer
>> to a PM domain structure that has been removed.
>>
>> PM domains now have a reference to the pointer for the PM domain
>> provider's data variable. Add a function so that a PM domain can
>> remove a PM domain provider by referencing the data pointer.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  2 ++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 72055fef6256..438885f2455f 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>
>>  /**
>> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
>> + * @data: Pointer to the data associated with the PM domain provider
>> + *
>> + * Look up a PM domain provider based upon a pointer to it's data and
>> + * remove the PM domain provider from the list of providers.
>> + */
>> +void of_genpd_del_provider_by_data(void *data)
>> +{
>> +       struct of_genpd_provider *c, *cp;
>> +
>> +       mutex_lock(&of_genpd_mutex);
>> +       list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
>> +               if (cp->data == data) {
>> +                       list_del(&cp->link);
>> +                       of_node_put(cp->node);
>> +                       kfree(cp);
>> +                       break;
>> +               }
>> +       }
>> +       mutex_unlock(&of_genpd_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
>> +
>> +/**
>>   * of_genpd_get_from_provider() - Look-up PM domain
>>   * @genpdspec: OF phandle args to use for look-up
>>   *
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index bed84413546f..7b7921a65cb0 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>  int of_genpd_add_provider_onecell(struct device_node *np,
>>                                   struct genpd_onecell_data *data);
>>  void of_genpd_del_provider(struct device_node *np);
> 
> There's currently only one user of of_genpd_del_provider().
> 
> Could this patch just convert that user to the new API, so we don't
> need to keep both the legacy and new one?
> 
> I guess we could then just stick to the name "of_genpd_del_provider()".

I had a look at this and to do that we would end up with
of_genpd_del_provider(struct device_node *np, void *data) where the user
should only pass one of the arguments. It seems a bit odd. However,
unless I have forgotten something, I wonder if we should just make
of_genpd_del_provider_by_name() a local function and not export this at
all? It seems more natural for users to delete a provider by the
device_node than by name rather than the data argument.

The only problem I see with making of_genpd_del_provider_by_name() local
is that I need to add a prototype for the function at the top of the
domain.c source file so that it builds because __pm_genpd_remove() is
defined above it. Yes I could move __pm_genpd_remove() to the bottom of
the file but then it is not located next to pm_genpd_init() which seems odd.

Let me know what you think.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Jon Hunter]

Hi Ulf,

On 21/06/16 14:47, Jon Hunter wrote:
> 
> On 15/06/16 15:38, Ulf Hansson wrote:
>> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> To remove a PM domain from the system, it is necessary to ensure
>>> that any PM domain providers associated with the PM domain have
>>> been removed. Otherwise it could be possible to obtain a pointer
>>> to a PM domain structure that has been removed.
>>>
>>> PM domains now have a reference to the pointer for the PM domain
>>> provider's data variable. Add a function so that a PM domain can
>>> remove a PM domain provider by referencing the data pointer.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>  include/linux/pm_domain.h   |  2 ++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 72055fef6256..438885f2455f 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>>>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>
>>>  /**
>>> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
>>> + * @data: Pointer to the data associated with the PM domain provider
>>> + *
>>> + * Look up a PM domain provider based upon a pointer to it's data and
>>> + * remove the PM domain provider from the list of providers.
>>> + */
>>> +void of_genpd_del_provider_by_data(void *data)
>>> +{
>>> +       struct of_genpd_provider *c, *cp;
>>> +
>>> +       mutex_lock(&of_genpd_mutex);
>>> +       list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
>>> +               if (cp->data == data) {
>>> +                       list_del(&cp->link);
>>> +                       of_node_put(cp->node);
>>> +                       kfree(cp);
>>> +                       break;
>>> +               }
>>> +       }
>>> +       mutex_unlock(&of_genpd_mutex);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
>>> +
>>> +/**
>>>   * of_genpd_get_from_provider() - Look-up PM domain
>>>   * @genpdspec: OF phandle args to use for look-up
>>>   *
>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>>> index bed84413546f..7b7921a65cb0 100644
>>> --- a/include/linux/pm_domain.h
>>> +++ b/include/linux/pm_domain.h
>>> @@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>>  int of_genpd_add_provider_onecell(struct device_node *np,
>>>                                   struct genpd_onecell_data *data);
>>>  void of_genpd_del_provider(struct device_node *np);
>>
>> There's currently only one user of of_genpd_del_provider().
>>
>> Could this patch just convert that user to the new API, so we don't
>> need to keep both the legacy and new one?
>>
>> I guess we could then just stick to the name "of_genpd_del_provider()".
> 
> I had a look at this and to do that we would end up with
> of_genpd_del_provider(struct device_node *np, void *data) where the user
> should only pass one of the arguments. It seems a bit odd. However,
> unless I have forgotten something, I wonder if we should just make
> of_genpd_del_provider_by_name() a local function and not export this at
> all? It seems more natural for users to delete a provider by the
> device_node than by name rather than the data argument.
> 
> The only problem I see with making of_genpd_del_provider_by_name() local
> is that I need to add a prototype for the function at the top of the
> domain.c source file so that it builds because __pm_genpd_remove() is
> defined above it. Yes I could move __pm_genpd_remove() to the bottom of
> the file but then it is not located next to pm_genpd_init() which seems odd.
> 
> Let me know what you think.

Any response on this? This is the last item that we need to sort out?

Cheers
Jon

-- 
nvpublic

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Ulf Hansson]

On 21 June 2016 at 15:47, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 15/06/16 15:38, Ulf Hansson wrote:
>> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> To remove a PM domain from the system, it is necessary to ensure
>>> that any PM domain providers associated with the PM domain have
>>> been removed. Otherwise it could be possible to obtain a pointer
>>> to a PM domain structure that has been removed.
>>>
>>> PM domains now have a reference to the pointer for the PM domain
>>> provider's data variable. Add a function so that a PM domain can
>>> remove a PM domain provider by referencing the data pointer.


>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>  include/linux/pm_domain.h   |  2 ++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 72055fef6256..438885f2455f 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>>>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>
>>>  /**
>>> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
>>> + * @data: Pointer to the data associated with the PM domain provider
>>> + *
>>> + * Look up a PM domain provider based upon a pointer to it's data and
>>> + * remove the PM domain provider from the list of providers.
>>> + */
>>> +void of_genpd_del_provider_by_data(void *data)
>>> +{
>>> +       struct of_genpd_provider *c, *cp;
>>> +
>>> +       mutex_lock(&of_genpd_mutex);
>>> +       list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
>>> +               if (cp->data == data) {
>>> +                       list_del(&cp->link);
>>> +                       of_node_put(cp->node);
>>> +                       kfree(cp);
>>> +                       break;
>>> +               }
>>> +       }
>>> +       mutex_unlock(&of_genpd_mutex);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
>>> +
>>> +/**
>>>   * of_genpd_get_from_provider() - Look-up PM domain
>>>   * @genpdspec: OF phandle args to use for look-up
>>>   *
>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>>> index bed84413546f..7b7921a65cb0 100644
>>> --- a/include/linux/pm_domain.h
>>> +++ b/include/linux/pm_domain.h
>>> @@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>>  int of_genpd_add_provider_onecell(struct device_node *np,
>>>                                   struct genpd_onecell_data *data);
>>>  void of_genpd_del_provider(struct device_node *np);
>>
>> There's currently only one user of of_genpd_del_provider().
>>
>> Could this patch just convert that user to the new API, so we don't
>> need to keep both the legacy and new one?
>>
>> I guess we could then just stick to the name "of_genpd_del_provider()".
>
> I had a look at this and to do that we would end up with
> of_genpd_del_provider(struct device_node *np, void *data) where the user
> should only pass one of the arguments. It seems a bit odd. However,
> unless I have forgotten something, I wonder if we should just make
> of_genpd_del_provider_by_name() a local function and not export this at
> all? It seems more natural for users to delete a provider by the
> device_node than by name rather than the data argument.
>
> The only problem I see with making of_genpd_del_provider_by_name() local
> is that I need to add a prototype for the function at the top of the
> domain.c source file so that it builds because __pm_genpd_remove() is
> defined above it. Yes I could move __pm_genpd_remove() to the bottom of
> the file but then it is not located next to pm_genpd_init() which seems odd.
>
> Let me know what you think.

Sorry for delay! I have now looked into this in more detail.

When an genpd provider is added today, it's supposed to get a
corresponding *unique* OF device node associated with it, right!?

If we store this OF device node from the provider in the struct
generic_pm_domain, instead of the "provider_data pointer", we wouldn't
need to the add of_genpd_del_provider_by_data() at all. Because we can
use the currently available of_genpd_del_provider(), right!?

Or what am I missing? :-)

Kind regards
Uffe

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Jon Hunter]

On 05/08/16 12:55, Ulf Hansson wrote:
> On 21 June 2016 at 15:47, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 15/06/16 15:38, Ulf Hansson wrote:
>>> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> To remove a PM domain from the system, it is necessary to ensure
>>>> that any PM domain providers associated with the PM domain have
>>>> been removed. Otherwise it could be possible to obtain a pointer
>>>> to a PM domain structure that has been removed.
>>>>
>>>> PM domains now have a reference to the pointer for the PM domain
>>>> provider's data variable. Add a function so that a PM domain can
>>>> remove a PM domain provider by referencing the data pointer.
> 
> 
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>>  include/linux/pm_domain.h   |  2 ++
>>>>  2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>> index 72055fef6256..438885f2455f 100644
>>>> --- a/drivers/base/power/domain.c
>>>> +++ b/drivers/base/power/domain.c
>>>> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>>>>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>>
>>>>  /**
>>>> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
>>>> + * @data: Pointer to the data associated with the PM domain provider
>>>> + *
>>>> + * Look up a PM domain provider based upon a pointer to it's data and
>>>> + * remove the PM domain provider from the list of providers.
>>>> + */
>>>> +void of_genpd_del_provider_by_data(void *data)
>>>> +{
>>>> +       struct of_genpd_provider *c, *cp;
>>>> +
>>>> +       mutex_lock(&of_genpd_mutex);
>>>> +       list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
>>>> +               if (cp->data == data) {
>>>> +                       list_del(&cp->link);
>>>> +                       of_node_put(cp->node);
>>>> +                       kfree(cp);
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +       mutex_unlock(&of_genpd_mutex);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
>>>> +
>>>> +/**
>>>>   * of_genpd_get_from_provider() - Look-up PM domain
>>>>   * @genpdspec: OF phandle args to use for look-up
>>>>   *
>>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>>>> index bed84413546f..7b7921a65cb0 100644
>>>> --- a/include/linux/pm_domain.h
>>>> +++ b/include/linux/pm_domain.h
>>>> @@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>>>  int of_genpd_add_provider_onecell(struct device_node *np,
>>>>                                   struct genpd_onecell_data *data);
>>>>  void of_genpd_del_provider(struct device_node *np);
>>>
>>> There's currently only one user of of_genpd_del_provider().
>>>
>>> Could this patch just convert that user to the new API, so we don't
>>> need to keep both the legacy and new one?
>>>
>>> I guess we could then just stick to the name "of_genpd_del_provider()".
>>
>> I had a look at this and to do that we would end up with
>> of_genpd_del_provider(struct device_node *np, void *data) where the user
>> should only pass one of the arguments. It seems a bit odd. However,
>> unless I have forgotten something, I wonder if we should just make
>> of_genpd_del_provider_by_name() a local function and not export this at
>> all? It seems more natural for users to delete a provider by the
>> device_node than by name rather than the data argument.
>>
>> The only problem I see with making of_genpd_del_provider_by_name() local
>> is that I need to add a prototype for the function at the top of the
>> domain.c source file so that it builds because __pm_genpd_remove() is
>> defined above it. Yes I could move __pm_genpd_remove() to the bottom of
>> the file but then it is not located next to pm_genpd_init() which seems odd.
>>
>> Let me know what you think.
> 
> Sorry for delay! I have now looked into this in more detail.

No problem. Thanks!

> When an genpd provider is added today, it's supposed to get a
> corresponding *unique* OF device node associated with it, right!?
> 
> If we store this OF device node from the provider in the struct
> generic_pm_domain, instead of the "provider_data pointer", we wouldn't
> need to the add of_genpd_del_provider_by_data() at all. Because we can
> use the currently available of_genpd_del_provider(), right!?
> 
> Or what am I missing? :-)

No that would work as well. I guess I was trying to make it non-DT
specific. However, for now it can be to simplify matters and it could
always be extended later if necessary.

I am also thinking about making pm_genpd_remove_tail()
of_genpd_remove_tail() as it seems silly to have both a struct device
pointer and a struct device_node pointer stored for the provider.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Rafael J. Wysocki]

On Thu, Aug 11, 2016 at 6:39 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 05/08/16 12:55, Ulf Hansson wrote:
>> On 21 June 2016 at 15:47, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>
>>> On 15/06/16 15:38, Ulf Hansson wrote:
>>>> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> To remove a PM domain from the system, it is necessary to ensure
>>>>> that any PM domain providers associated with the PM domain have
>>>>> been removed. Otherwise it could be possible to obtain a pointer
>>>>> to a PM domain structure that has been removed.
>>>>>
>>>>> PM domains now have a reference to the pointer for the PM domain
>>>>> provider's data variable. Add a function so that a PM domain can
>>>>> remove a PM domain provider by referencing the data pointer.
>>
>>
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>> ---
>>>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>>>  include/linux/pm_domain.h   |  2 ++
>>>>>  2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>>> index 72055fef6256..438885f2455f 100644
>>>>> --- a/drivers/base/power/domain.c
>>>>> +++ b/drivers/base/power/domain.c
>>>>> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>>>>>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>>>
>>>>>  /**
>>>>> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
>>>>> + * @data: Pointer to the data associated with the PM domain provider
>>>>> + *
>>>>> + * Look up a PM domain provider based upon a pointer to it's data and
>>>>> + * remove the PM domain provider from the list of providers.
>>>>> + */
>>>>> +void of_genpd_del_provider_by_data(void *data)
>>>>> +{
>>>>> +       struct of_genpd_provider *c, *cp;
>>>>> +
>>>>> +       mutex_lock(&of_genpd_mutex);
>>>>> +       list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
>>>>> +               if (cp->data == data) {
>>>>> +                       list_del(&cp->link);
>>>>> +                       of_node_put(cp->node);
>>>>> +                       kfree(cp);
>>>>> +                       break;
>>>>> +               }
>>>>> +       }
>>>>> +       mutex_unlock(&of_genpd_mutex);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(of_genpd_del_provider_by_data);
>>>>> +
>>>>> +/**
>>>>>   * of_genpd_get_from_provider() - Look-up PM domain
>>>>>   * @genpdspec: OF phandle args to use for look-up
>>>>>   *
>>>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>>>>> index bed84413546f..7b7921a65cb0 100644
>>>>> --- a/include/linux/pm_domain.h
>>>>> +++ b/include/linux/pm_domain.h
>>>>> @@ -199,6 +199,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
>>>>>  int of_genpd_add_provider_onecell(struct device_node *np,
>>>>>                                   struct genpd_onecell_data *data);
>>>>>  void of_genpd_del_provider(struct device_node *np);
>>>>
>>>> There's currently only one user of of_genpd_del_provider().
>>>>
>>>> Could this patch just convert that user to the new API, so we don't
>>>> need to keep both the legacy and new one?
>>>>
>>>> I guess we could then just stick to the name "of_genpd_del_provider()".
>>>
>>> I had a look at this and to do that we would end up with
>>> of_genpd_del_provider(struct device_node *np, void *data) where the user
>>> should only pass one of the arguments. It seems a bit odd. However,
>>> unless I have forgotten something, I wonder if we should just make
>>> of_genpd_del_provider_by_name() a local function and not export this at
>>> all? It seems more natural for users to delete a provider by the
>>> device_node than by name rather than the data argument.
>>>
>>> The only problem I see with making of_genpd_del_provider_by_name() local
>>> is that I need to add a prototype for the function at the top of the
>>> domain.c source file so that it builds because __pm_genpd_remove() is
>>> defined above it. Yes I could move __pm_genpd_remove() to the bottom of
>>> the file but then it is not located next to pm_genpd_init() which seems odd.
>>>
>>> Let me know what you think.
>>
>> Sorry for delay! I have now looked into this in more detail.
>
> No problem. Thanks!
>
>> When an genpd provider is added today, it's supposed to get a
>> corresponding *unique* OF device node associated with it, right!?
>>
>> If we store this OF device node from the provider in the struct
>> generic_pm_domain, instead of the "provider_data pointer", we wouldn't
>> need to the add of_genpd_del_provider_by_data() at all. Because we can
>> use the currently available of_genpd_del_provider(), right!?
>>
>> Or what am I missing? :-)

Please don't store device_node pointers in generic data structures at
least in the code that I maintain (some other people may not care).

Store struct fwnode_handle pointers instead if you have to.

Thanks,
Rafael

[RFC PATCH 6/8] PM / Domains: Remove a provider by referencing the data pointer

[Jon Hunter]

On 04/03/16 11:23, Jon Hunter wrote:
> To remove a PM domain from the system, it is necessary to ensure
> that any PM domain providers associated with the PM domain have
> been removed. Otherwise it could be possible to obtain a pointer
> to a PM domain structure that has been removed.
> 
> PM domains now have a reference to the pointer for the PM domain
> provider's data variable. Add a function so that a PM domain can
> remove a PM domain provider by referencing the data pointer.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 72055fef6256..438885f2455f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1738,6 +1738,30 @@ void of_genpd_del_provider(struct device_node *np)
>  EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>  
>  /**
> + * of_genpd_del_provider_by_data() - Remove a registered PM domain provider
> + * @data: Pointer to the data associated with the PM domain provider
> + *
> + * Look up a PM domain provider based upon a pointer to it's data and
> + * remove the PM domain provider from the list of providers.
> + */
> +void of_genpd_del_provider_by_data(void *data)
> +{
> +	struct of_genpd_provider *c, *cp;
> +
> +	mutex_lock(&of_genpd_mutex);
> +	list_for_each_entry_safe(cp, c, &of_genpd_providers, link) {
> +		if (cp->data == data) {
> +			list_del(&cp->link);
> +			of_node_put(cp->node);
> +			kfree(cp);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&of_genpd_mutex);

On further the thought I believe that the above does not need to be safe
variant of list_for_each_entry because we are breaking out of the loop
when we find the one we are looking for. of_genpd_del_provider() does
the same.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 7/8] 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 438885f2455f..9b33377bf01b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1214,14 +1214,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;
@@ -1261,6 +1255,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);
 
 /**
@@ -1314,13 +1326,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;
@@ -1363,6 +1370,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);
 
 /**
@@ -1807,12 +1831,22 @@ static struct generic_pm_domain *of_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 = of_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;
 }
 
 /**
@@ -1828,16 +1862,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 = of_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 = of_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;
 }
 
 /**
@@ -1935,9 +1981,11 @@ int genpd_dev_pm_attach(struct device *dev)
 			return -ENOENT;
 	}
 
+	mutex_lock(&gpd_list_lock);
 	pd = of_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;
@@ -1946,13 +1994,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

[RFC PATCH 8/8] 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.

If a device supports nested or subdomains, then the PM domains
should be removed in reverse order to ensure that the subdomains are
removed first. Hence, add the function pm_genpd_remove_tail() to remove
the last PM domain added by a given provider and return the
generic_pm_domain structure for the PM domain that was removed.

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

When removing PM domains, the PM domain will also be removed from the
list of providers, if it was registered.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 9b33377bf01b..17090e1c91d6 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1556,6 +1556,102 @@ void 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 from the list of providers, if registered.
+ *  - 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. Must be called
+ * with the gpd_list_lock held.
+ */
+static 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;
+
+	if (genpd->provider_data)
+		of_genpd_del_provider_by_data(genpd->provider_data);
+
+	mutex_lock(&genpd->lock);
+
+	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 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 = __pm_genpd_remove(genpd);
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
+
+/**
+ * pm_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 the provider whose 'provider'
+ * device structure matches the device structure given. The 'provider'
+ * device structure for a given PM domain should be initialised by the
+ * device that is creating the PM domains and hence, calling
+ * pm_genpd_init().
+ *
+ * Returns a valid pointer to struct generic_pm_domain on success or
+ * ERR_PTR() on failure.
+ */
+struct generic_pm_domain *pm_genpd_remove_tail(struct device *provider)
+{
+	struct generic_pm_domain *g, *gpd, *genpd = ERR_PTR(-ENOENT);
+	int ret;
+
+	if (IS_ERR_OR_NULL(provider))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry_safe(gpd, g, &gpd_list, gpd_list_node) {
+		if (gpd->provider == provider) {
+			ret = __pm_genpd_remove(gpd);
+			genpd = ret ? ERR_PTR(ret) : gpd;
+			break;
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return genpd;
+}
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 7b7921a65cb0..78b23718392f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -53,6 +53,7 @@ struct generic_pm_domain {
 	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
+	struct device *provider;	/* Identity of the domain provider */
 	void *provider_data;
 	const char *name;
 	atomic_t sd_count;	/* Number of subdomains with power "on" */
@@ -132,6 +133,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 				     struct generic_pm_domain *target);
 extern void 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 generic_pm_domain *pm_genpd_remove_tail(struct device *provider);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -166,6 +169,15 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd,
 				 struct dev_power_governor *gov, bool is_off)
 {
 }
+static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	return -ENOTSUPP;
+}
+static inline
+struct generic_pm_domain *pm_genpd_remove_tail(struct device *provider)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
 #endif
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-- 
2.1.4

[RFC PATCH 8/8] PM / Domains: Add support for removing PM domains

[Ulf Hansson]

On 4 March 2016 at 12:23, 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.
>
> If a device supports nested or subdomains, then the PM domains
> should be removed in reverse order to ensure that the subdomains are
> removed first. Hence, add the function pm_genpd_remove_tail() to remove
> the last PM domain added by a given provider and return the
> generic_pm_domain structure for the PM domain that was removed.

Perhaps split this up, so the pm_genpd_remove_tail() gets added in a
separate patch on top.

>
> PM domains can only be removed if they are not a parent domain to
> another PM domain and have no devices associated with them.
>
> When removing PM domains, the PM domain will also be removed from the
> list of providers, if it was registered.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/domain.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   | 12 ++++++
>  2 files changed, 108 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 9b33377bf01b..17090e1c91d6 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1556,6 +1556,102 @@ void 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 from the list of providers, if registered.
> + *  - 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. Must be called
> + * with the gpd_list_lock held.
> + */
> +static int __pm_genpd_remove(struct generic_pm_domain *genpd)

Please rename to genpd_remove() as it's a static function.

> +{
> +       struct gpd_link *l, *link;
> +       int ret = 0;
> +
> +       if (IS_ERR_OR_NULL(genpd))
> +               return -EINVAL;
> +
> +       if (genpd->provider_data)
> +               of_genpd_del_provider_by_data(genpd->provider_data);
> +
> +       mutex_lock(&genpd->lock);
> +
> +       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 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 = __pm_genpd_remove(genpd);
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
> +}
> +
> +/**
> + * pm_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 the provider whose 'provider'
> + * device structure matches the device structure given. The 'provider'
> + * device structure for a given PM domain should be initialised by the
> + * device that is creating the PM domains and hence, calling
> + * pm_genpd_init().

Maybe make this a bigger "Important note" as what is needed to
actually benefit from using this function.

> + *
> + * Returns a valid pointer to struct generic_pm_domain on success or
> + * ERR_PTR() on failure.
> + */
> +struct generic_pm_domain *pm_genpd_remove_tail(struct device *provider)
> +{
> +       struct generic_pm_domain *g, *gpd, *genpd = ERR_PTR(-ENOENT);
> +       int ret;
> +
> +       if (IS_ERR_OR_NULL(provider))
> +               return ERR_PTR(-EINVAL);
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry_safe(gpd, g, &gpd_list, gpd_list_node) {
> +               if (gpd->provider == provider) {
> +                       ret = __pm_genpd_remove(gpd);
> +                       genpd = ret ? ERR_PTR(ret) : gpd;
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return genpd;
> +}
> +
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  /*
>   * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 7b7921a65cb0..78b23718392f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -53,6 +53,7 @@ struct generic_pm_domain {
>         struct mutex lock;
>         struct dev_power_governor *gov;
>         struct work_struct power_off_work;
> +       struct device *provider;        /* Identity of the domain provider */
>         void *provider_data;
>         const char *name;
>         atomic_t sd_count;      /* Number of subdomains with power "on" */
> @@ -132,6 +133,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>                                      struct generic_pm_domain *target);
>  extern void 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 generic_pm_domain *pm_genpd_remove_tail(struct device *provider);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -166,6 +169,15 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd,
>                                  struct dev_power_governor *gov, bool is_off)
>  {
>  }
> +static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> +       return -ENOTSUPP;
> +}
> +static inline
> +struct generic_pm_domain *pm_genpd_remove_tail(struct device *provider)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
>  #endif
>
>  static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
> --
> 2.1.4
>

Kind regards
Uffe

[RFC PATCH 8/8] PM / Domains: Add support for removing PM domains

[Jon Hunter]

On 15/06/16 15:33, Ulf Hansson wrote:
> On 4 March 2016 at 12:23, 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.
>>
>> If a device supports nested or subdomains, then the PM domains
>> should be removed in reverse order to ensure that the subdomains are
>> removed first. Hence, add the function pm_genpd_remove_tail() to remove
>> the last PM domain added by a given provider and return the
>> generic_pm_domain structure for the PM domain that was removed.
> 
> Perhaps split this up, so the pm_genpd_remove_tail() gets added in a
> separate patch on top.

OK.

>>
>> PM domains can only be removed if they are not a parent domain to
>> another PM domain and have no devices associated with them.
>>
>> When removing PM domains, the PM domain will also be removed from the
>> list of providers, if it was registered.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/base/power/domain.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   | 12 ++++++
>>  2 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 9b33377bf01b..17090e1c91d6 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1556,6 +1556,102 @@ void 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 from the list of providers, if registered.
>> + *  - 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. Must be called
>> + * with the gpd_list_lock held.
>> + */
>> +static int __pm_genpd_remove(struct generic_pm_domain *genpd)
> 
> Please rename to genpd_remove() as it's a static function.

OK.

>> +{
>> +       struct gpd_link *l, *link;
>> +       int ret = 0;
>> +
>> +       if (IS_ERR_OR_NULL(genpd))
>> +               return -EINVAL;
>> +
>> +       if (genpd->provider_data)
>> +               of_genpd_del_provider_by_data(genpd->provider_data);
>> +
>> +       mutex_lock(&genpd->lock);
>> +
>> +       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 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 = __pm_genpd_remove(genpd);
>> +       mutex_unlock(&gpd_list_lock);
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>> + * pm_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 the provider whose 'provider'
>> + * device structure matches the device structure given. The 'provider'
>> + * device structure for a given PM domain should be initialised by the
>> + * device that is creating the PM domains and hence, calling
>> + * pm_genpd_init().
> 
> Maybe make this a bigger "Important note" as what is needed to
> actually benefit from using this function.

Yes, good point. Will update.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 0/8] PM / Domains: Add support for removing PM domains

[Ulf Hansson]

On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> 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. Another alternative would be
> to employ some reference counting for the PM domains, however, I did not see
> a good reason for allowing external references in the first place (as always
> there could be something that I have over-looked!).
>
> Jon Hunter (8):
>   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
>   PM / Domains: Verify the PM domain is present when adding a provider
>   PM / Domains: Remove a provider by referencing the data pointer
>   PM / Domains: Prepare for adding support to remove PM domains
>   PM / Domains: Add support for removing PM domains
>
>  arch/arm/mach-exynos/pm_domains.c |  23 +--
>  drivers/base/power/domain.c       | 324 +++++++++++++++++++++++++++++++++++---
>  drivers/staging/board/board.c     |   9 +-
>  include/linux/pm_domain.h         |  55 ++++---
>  4 files changed, 348 insertions(+), 63 deletions(-)
>
> --
> 2.1.4
>

Jon, thanks for posting this!
My initial impressions is the I like the ideas for where you moves this.

I will review it in more details as soon as I can (travelling next week).

Kind regards
Uffe

[RFC PATCH 0/8] PM / Domains: Add support for removing PM domains

[Jon Hunter]

Hi Uffe,

On 04/03/16 12:33, Ulf Hansson wrote:
> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> 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. Another alternative would be
>> to employ some reference counting for the PM domains, however, I did not see
>> a good reason for allowing external references in the first place (as always
>> there could be something that I have over-looked!).
>>
>> Jon Hunter (8):
>>   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
>>   PM / Domains: Verify the PM domain is present when adding a provider
>>   PM / Domains: Remove a provider by referencing the data pointer
>>   PM / Domains: Prepare for adding support to remove PM domains
>>   PM / Domains: Add support for removing PM domains
>>
>>  arch/arm/mach-exynos/pm_domains.c |  23 +--
>>  drivers/base/power/domain.c       | 324 +++++++++++++++++++++++++++++++++++---
>>  drivers/staging/board/board.c     |   9 +-
>>  include/linux/pm_domain.h         |  55 ++++---
>>  4 files changed, 348 insertions(+), 63 deletions(-)
>>
>> --
>> 2.1.4
>>
> 
> Jon, thanks for posting this!
> My initial impressions is the I like the ideas for where you moves this.
> 
> I will review it in more details as soon as I can (travelling next week).

Just wanted to see if you have had chance to look at this yet? Not urgent.

Cheers
Jon

[RFC PATCH 0/8] PM / Domains: Add support for removing PM domains

[Jon Hunter]

Hi Ulf,

On 28/03/16 13:38, Jon Hunter wrote:
> Hi Uffe,
> 
> On 04/03/16 12:33, Ulf Hansson wrote:
>> On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> 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. Another alternative would be
>>> to employ some reference counting for the PM domains, however, I did not see
>>> a good reason for allowing external references in the first place (as always
>>> there could be something that I have over-looked!).
>>>
>>> Jon Hunter (8):
>>>   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
>>>   PM / Domains: Verify the PM domain is present when adding a provider
>>>   PM / Domains: Remove a provider by referencing the data pointer
>>>   PM / Domains: Prepare for adding support to remove PM domains
>>>   PM / Domains: Add support for removing PM domains
>>>
>>>  arch/arm/mach-exynos/pm_domains.c |  23 +--
>>>  drivers/base/power/domain.c       | 324 +++++++++++++++++++++++++++++++++++---
>>>  drivers/staging/board/board.c     |   9 +-
>>>  include/linux/pm_domain.h         |  55 ++++---
>>>  4 files changed, 348 insertions(+), 63 deletions(-)
>>>
>>> --
>>> 2.1.4
>>>
>>
>> Jon, thanks for posting this!
>> My initial impressions is the I like the ideas for where you moves this.
>>
>> I will review it in more details as soon as I can (travelling next week).
> 
> Just wanted to see if you have had chance to look at this yet? Not urgent.

Any feedback on this?

FYI, patch #2 does not apply cleanly on the latest -next but the others
should.

Cheers
Jon

-- 
nvpublic

[RFC PATCH 0/8] PM / Domains: Add support for removing PM domains

[Ulf Hansson]

On 4 March 2016 at 12:23, Jon Hunter <jonathanh@nvidia.com> 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. Another alternative would be
> to employ some reference counting for the PM domains, however, I did not see
> a good reason for allowing external references in the first place (as always
> there could be something that I have over-looked!).
>
> Jon Hunter (8):
>   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
>   PM / Domains: Verify the PM domain is present when adding a provider
>   PM / Domains: Remove a provider by referencing the data pointer
>   PM / Domains: Prepare for adding support to remove PM domains
>   PM / Domains: Add support for removing PM domains
>
>  arch/arm/mach-exynos/pm_domains.c |  23 +--
>  drivers/base/power/domain.c       | 324 +++++++++++++++++++++++++++++++++++---
>  drivers/staging/board/board.c     |   9 +-
>  include/linux/pm_domain.h         |  55 ++++---
>  4 files changed, 348 insertions(+), 63 deletions(-)
>
> --
> 2.1.4
>

Apologize for the long delay!

I have looked through this now and I really like this!

There were a few comments, but those should be easily taken care of.
So please go ahead and re-spin a new version!

There are a few outstanding genpd patches which I have posted a while
ago but which isn't yet queued for 4.8.
I don't know whether those will conflict with this series (if they get
merged prior to this), but I guess it's better if you anyway base you
series on Rafael's latest changes instead of on top of mine...

Kind regards
Uffe