In order to safely remove PM domains there are a few changes that need to be
made to ensure that no one is holding an external reference to a PM domain
after it has been removed. One solution, implemented here, solves this by
eliminating external references to PM domain.
This is a follow-up to the initial RFC I sent out [0].
Changes from initial RFC:
- Renamed functions made static per Ulf's feedback.
- Added patch to clean-up provider/xlate APIs per Ulf's feedback
- Re-worked and simplified the association between PM domains and PM
domain providers. Dropped the 'provider_data' variable from the
generic_pm_domain structure in favour of using the fwnode_handle.
- Split patch for removing PM domains into multiple patches per Ulf's
feedback.
[0] http://marc.info/?l=linux-pm&m=145709064407085&w=2
Jon Hunter (10):
PM / Domains: Add new helper functions for device-tree
ARM: EXYNOS: Remove calls to of_genpd_get_from_provider()
staging: board: Remove calls to of_genpd_get_from_provider()
PM / Domains: Don't expose generic_pm_domain structure to clients
PM / Domains: Don't expose xlate and provider helper functions
PM / Domains: Verify the PM domain is present when adding a provider
PM / Domains: Prepare for adding support to remove PM domains
PM / Domains: Add support for removing PM domains
PM / Domains: Store the provider in the PM domain structure
PM / Domains: Add support for removing nested PM domains by provider
drivers/base/power/domain.c | 361 +++++++++++++++++++++++++++++++++++----
drivers/soc/samsung/pm_domains.c | 23 +--
drivers/staging/board/board.c | 9 +-
include/linux/pm_domain.h | 73 ++++----
4 files changed, 371 insertions(+), 95 deletions(-)
--
2.1.4
Ideally, if we are returning a reference to a PM domain via a call to
of_genpd_get_from_provider(), then we should keep track of such
references via a reference count. The reference count could then be used
to determine if a PM domain can be safely removed. Alternatively, it is
possible to avoid such external references by providing APIs to access
the PM domain and hence, eliminate any calls to
of_genpd_get_from_provider().
Add new helper functions for adding a device and a subdomain to a PM
domain when using device-tree, so that external calls to
of_genpd_get_from_provider() can be removed.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 16 ++++++++++++++++
2 files changed, 62 insertions(+)
Update the EXYNOS PM domain code to use the of_genpd_add_subdomain()
and remove any calls to of_genpd_get_from_provider().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/soc/samsung/pm_domains.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
@@ -215,29 +215,22 @@ no_clk:/* Assign the child power domains to their parents */for_each_matching_node(np,exynos_pm_domain_of_match){-structgeneric_pm_domain*child_domain,*parent_domain;-structof_phandle_argsargs;+structof_phandle_argschild,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);elsepr_info("%s has as child subdomain: %s.\n",-parent_domain->name,child_domain->name);+parent.np->name,child.np->name);}return0;
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(-)
There should be no need to expose the generic_pm_domain structure to
clients and this eliminates the need to implement reference counting for
any external reference to a PM domain. Therefore, make the functions
pm_genpd_lookup_dev() and of_genpd_get_from_provider() private to the
PM domain core. The functions are renamed in accordance with the naming
conventions for genpd static functions.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 19 +++++++++----------
include/linux/pm_domain.h | 14 --------------
2 files changed, 9 insertions(+), 24 deletions(-)
@@ -1119,7 +1119,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,dev_dbg(dev,"%s()\n",__func__);-if(!genpd||genpd!=pm_genpd_lookup_dev(dev))+if(!genpd||genpd!=genpd_lookup_dev(dev))return-EINVAL;/* The above validation also means we have existing domain_data. */
Functions __of_genpd_xlate_simple(), __of_genpd_xlate_onecell() and
__of_genpd_add_provider() are not used outside of the core generic PM
domain code. Therefore, reduce the number of APIs exposed by making
these static. At the same time don't expose the typedef for
genpd_xlate_t either and make this a local definition as well.
The functions are renamed to follow the naming conventions for static
functions in the generic PM domain core.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 49 ++++++++++++++++++++++++++++++++++-----------
include/linux/pm_domain.h | 42 +++++++++++++-------------------------
2 files changed, 51 insertions(+), 40 deletions(-)
When a PM domain provider is added, there is currently no way to tell if
any PM domains associated with the provider are present. Naturally, the
PM domain provider should not be registered if the PM domains have not
been added. Nonetheless, verify that the PM domain(s) associated with a
provider are present when registering the PM domain provider.
This change adds a dependency on the function pm_genpd_present() when
CONFIG_PM_GENERIC_DOMAINS_OF is enabled and so ensure this function is
available when CONFIG_PM_GENERIC_DOMAINS_OF selected.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 42 insertions(+), 3 deletions(-)
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(-)
@@ -1597,16 +1631,28 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,structof_phandle_args*subdomain_spec){structgeneric_pm_domain*parent,*subdomain;+intret;++mutex_lock(&gpd_list_lock);parent=genpd_get_from_provider(parent_spec);-if(IS_ERR(parent))-returnPTR_ERR(parent);+if(IS_ERR(parent)){+ret=PTR_ERR(parent);+gotoout;+}subdomain=genpd_get_from_provider(subdomain_spec);-if(IS_ERR(subdomain))-returnPTR_ERR(subdomain);+if(IS_ERR(subdomain)){+ret=PTR_ERR(subdomain);+gotoout;+}++ret=genpd_add_subdomain(parent,subdomain);++out:+mutex_unlock(&gpd_list_lock);-returnpm_genpd_add_subdomain(parent,subdomain);+returnret;}EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
@@ -1705,9 +1751,11 @@ int genpd_dev_pm_attach(struct device *dev)return-ENOENT;}+mutex_lock(&gpd_list_lock);pd=genpd_get_from_provider(&pd_args);of_node_put(pd_args.np);if(IS_ERR(pd)){+mutex_unlock(&gpd_list_lock);dev_dbg(dev,"%s() failed to find PM domain: %ld\n",__func__,PTR_ERR(pd));return-EPROBE_DEFER;
@@ -1716,13 +1764,14 @@ int genpd_dev_pm_attach(struct device *dev)dev_dbg(dev,"adding to PM domain %s\n",pd->name);for(i=1;i<GENPD_RETRY_MAX_MS;i<<=1){-ret=pm_genpd_add_device(pd,dev);+ret=genpd_add_device(pd,dev,NULL);if(ret!=-EAGAIN)break;mdelay(i);cond_resched();}+mutex_unlock(&gpd_list_lock);if(ret<0){dev_err(dev,"failed to add to PM domain %s: %d",
The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.
Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.
PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 5 +++++
2 files changed, 51 insertions(+)
It is possible that a device has more than one provider of PM domains
and to support the removal of a PM domain by provider, it is necessary
to store a reference to the provider in the PM domain structure.
Therefore, store a reference to the firmware node handle in the PM
domain structure and populate it when providers (only device-tree based
providers are currently supported by PM domains) are registered.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 18 ++++++++++++++----
include/linux/pm_domain.h | 1 +
2 files changed, 15 insertions(+), 4 deletions(-)
@@ -51,6 +51,7 @@ struct generic_pm_domain {structmutexlock;structdev_power_governor*gov;structwork_structpower_off_work;+structfwnode_handle*provider;/* Identity of the domain provider */constchar*name;atomic_tsd_count;/* Number of subdomains with power "on" */enumgpd_statusstatus;/* Current state of the domain */
If a device supports PM domains that are subdomains of another PM
domain, then the PM domains should be removed in reverse order to
ensure that the subdomains are removed first. Furthermore, if there is
more than one provider, then there needs to be a way to remove the
domains in reverse order for a specific provider.
Add the function of_genpd_remove_tail() to remove the last PM domain
added by a given PM domain provider and return the generic_pm_domain
structure for the PM domain that was removed.
A PM domain should only be removed once the associated PM domain
provider has been removed from the list of providers. Otherwise, it
could be possible for a client to be associated with a PM domain that
could have been removed. Add a helper function to verify if the PM
domain provider is present and only allow a PM domain to be removed if
the provider has been removed.
The function of_genpd_remove_tail() must hold the gpd_list_lock while
finding and removing a PM domain. It is natural for
of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
PM domain is found to remove it. However, pm_genpd_remove(), also
acquires the gpd_list_lock. Therefore, move the core of the function
pm_genpd_remove() to a new function genpd_remove() which does not
acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
and of_genpd_remove_tail().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
include/linux/pm_domain.h | 7 ++++
2 files changed, 88 insertions(+), 6 deletions(-)
From: Krzysztof Kozlowski <hidden> Date: 2016-08-16 19:26:22
On Tue, Aug 16, 2016 at 10:49:28AM +0100, Jon Hunter wrote:
Update the EXYNOS PM domain code to use the of_genpd_add_subdomain()
and remove any calls to of_genpd_get_from_provider().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/soc/samsung/pm_domains.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
Looks correct:
Reviewed-by: Krzysztof Kozlowski <redacted>
I understand this will go along with patch #1 to PM tree. There might be
some more commits around Exynos PM domain code coming soon. To avoid possible
conflicts, could you put it in a separate branch with patch #1 so a
stable tag could be easily created? I don't see direct necessity now but
it might be needed quite soon.
Best regards,
Krzysztof
In order to safely remove PM domains there are a few changes that need to be
made to ensure that no one is holding an external reference to a PM domain
after it has been removed. One solution, implemented here, solves this by
eliminating external references to PM domain.
This is a follow-up to the initial RFC I sent out [0].
Changes from initial RFC:
- Renamed functions made static per Ulf's feedback.
- Added patch to clean-up provider/xlate APIs per Ulf's feedback
- Re-worked and simplified the association between PM domains and PM
domain providers. Dropped the 'provider_data' variable from the
generic_pm_domain structure in favour of using the fwnode_handle.
- Split patch for removing PM domains into multiple patches per Ulf's
feedback.
Let me know if you have any feedback on this. Please note I will be out
next week.
Cheers
Jon
--
nvpublic
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] 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>
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
Update the EXYNOS PM domain code to use the of_genpd_add_subdomain()
and remove any calls to of_genpd_get_from_provider().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
@@ -215,29 +215,22 @@ no_clk:/* Assign the child power domains to their parents */for_each_matching_node(np,exynos_pm_domain_of_match){-structgeneric_pm_domain*child_domain,*parent_domain;-structof_phandle_argsargs;+structof_phandle_argschild,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);elsepr_info("%s has as child subdomain: %s.\n",-parent_domain->name,child_domain->name);+parent.np->name,child.np->name);}return0;--
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
Update the staging/board PM domain code to use the
of_genpd_add_subdomain() and remove any calls to
of_genpd_get_from_provider().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
There should be no need to expose the generic_pm_domain structure to
clients and this eliminates the need to implement reference counting for
any external reference to a PM domain. Therefore, make the functions
pm_genpd_lookup_dev() and of_genpd_get_from_provider() private to the
PM domain core. The functions are renamed in accordance with the naming
conventions for genpd static functions.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
@@ -1119,7 +1119,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,dev_dbg(dev,"%s()\n",__func__);-if(!genpd||genpd!=pm_genpd_lookup_dev(dev))+if(!genpd||genpd!=genpd_lookup_dev(dev))return-EINVAL;/* The above validation also means we have existing domain_data. */
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
Functions __of_genpd_xlate_simple(), __of_genpd_xlate_onecell() and
__of_genpd_add_provider() are not used outside of the core generic PM
domain code. Therefore, reduce the number of APIs exposed by making
these static. At the same time don't expose the typedef for
genpd_xlate_t either and make this a local definition as well.
The functions are renamed to follow the naming conventions for static
functions in the generic PM domain core.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted hunk
When a PM domain provider is added, there is currently no way to tell if
any PM domains associated with the provider are present. Naturally, the
PM domain provider should not be registered if the PM domains have not
been added. Nonetheless, verify that the PM domain(s) associated with a
provider are present when registering the PM domain provider.
This change adds a dependency on the function pm_genpd_present() when
CONFIG_PM_GENERIC_DOMAINS_OF is enabled and so ensure this function is
available when CONFIG_PM_GENERIC_DOMAINS_OF selected.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 42 insertions(+), 3 deletions(-)
You could simplify this, by assigning ret and initial value of
-EINVAL, then do like this:
if (pm_genpd_present(genpd))
ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
In order to remove PM domains safely from the list of PM domains,
it is necessary to adding locking for the PM domain list around any
places where devices or subdomains are added to a PM domain.
There are places where a reference to a PM domain is obtained via
calling of_genpd_get_from_provider() before adding the device or the
subdomain. In these cases a lock for the PM domain list needs to be
held around the call to of_genpd_get_from_provider() and the call to
add the device/subdomain. To avoid deadlocks by multiple attempts to
obtain the PM domain list lock, add functions genpd_add_device() and
genpd_add_subdomain() which require the user to hold the PM domain
list lock when calling.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
@@ -1597,16 +1631,28 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,structof_phandle_args*subdomain_spec){structgeneric_pm_domain*parent,*subdomain;+intret;++mutex_lock(&gpd_list_lock);parent=genpd_get_from_provider(parent_spec);-if(IS_ERR(parent))-returnPTR_ERR(parent);+if(IS_ERR(parent)){+ret=PTR_ERR(parent);+gotoout;+}subdomain=genpd_get_from_provider(subdomain_spec);-if(IS_ERR(subdomain))-returnPTR_ERR(subdomain);+if(IS_ERR(subdomain)){+ret=PTR_ERR(subdomain);+gotoout;+}++ret=genpd_add_subdomain(parent,subdomain);++out:+mutex_unlock(&gpd_list_lock);-returnpm_genpd_add_subdomain(parent,subdomain);+returnret;}EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
@@ -1705,9 +1751,11 @@ int genpd_dev_pm_attach(struct device *dev)return-ENOENT;}+mutex_lock(&gpd_list_lock);pd=genpd_get_from_provider(&pd_args);of_node_put(pd_args.np);if(IS_ERR(pd)){+mutex_unlock(&gpd_list_lock);dev_dbg(dev,"%s() failed to find PM domain: %ld\n",__func__,PTR_ERR(pd));return-EPROBE_DEFER;
@@ -1716,13 +1764,14 @@ int genpd_dev_pm_attach(struct device *dev)dev_dbg(dev,"adding to PM domain %s\n",pd->name);for(i=1;i<GENPD_RETRY_MAX_MS;i<<=1){-ret=pm_genpd_add_device(pd,dev);+ret=genpd_add_device(pd,dev,NULL);if(ret!=-EAGAIN)break;mdelay(i);cond_resched();}+mutex_unlock(&gpd_list_lock);if(ret<0){dev_err(dev,"failed to add to PM domain %s: %d",--
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.
Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.
PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.
I think we should also check if the there's is a provider registered
for the genpd, as it should also prevent the genpd from being removed.
Right?
I noticed that you are adding the ->provider pointer to the genpd
struct in patch 9/10. Perhaps re-structure $subject patch and 9/10 a
bit to deal with this, so we can add the pm_genpd_remove() API in a
more safe manner.
Kind regards
Uffe
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted hunk
It is possible that a device has more than one provider of PM domains
and to support the removal of a PM domain by provider, it is necessary
to store a reference to the provider in the PM domain structure.
Therefore, store a reference to the firmware node handle in the PM
domain structure and populate it when providers (only device-tree based
providers are currently supported by PM domains) are registered.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 18 ++++++++++++++----
include/linux/pm_domain.h | 1 +
2 files changed, 15 insertions(+), 4 deletions(-)
@@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np,return-EINVAL;}+genpd->provider=&np->fwnode;+ret=genpd_add_provider(np,genpd_xlate_simple,genpd);
I guess you want to reset genpd->provider = NULL, when
genpd_add_provider() fails!?
Perhaps better to assign genpd->provider when you know
genpd_add_provider() has succeeded.
quoted hunk
mutex_unlock(&gpd_list_lock);
@@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np, mutex_lock(&gpd_list_lock); for (i = 0; i < data->num_domains; i++) {- if (!pm_genpd_present(data->domains[i])) {- mutex_unlock(&gpd_list_lock);- return -EINVAL;- }+ if (!pm_genpd_present(data->domains[i]))+ goto error;++ data->domains[i]->provider = &np->fwnode; } ret = genpd_add_provider(np, genpd_xlate_onecell, data);
@@ -51,6 +51,7 @@ struct generic_pm_domain {structmutexlock;structdev_power_governor*gov;structwork_structpower_off_work;+structfwnode_handle*provider;/* Identity of the domain provider */constchar*name;atomic_tsd_count;/* Number of subdomains with power "on" */enumgpd_statusstatus;/* Current state of the domain */--
2.1.4
I also think you should extend this change, to also make the
of_genpd_del_provider() API to reset the genpd->provider = NULL.
Otherwise you can't track when a provider is removed.
Kind regards
Uffe
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted hunk
If a device supports PM domains that are subdomains of another PM
domain, then the PM domains should be removed in reverse order to
ensure that the subdomains are removed first. Furthermore, if there is
more than one provider, then there needs to be a way to remove the
domains in reverse order for a specific provider.
Add the function of_genpd_remove_tail() to remove the last PM domain
added by a given PM domain provider and return the generic_pm_domain
structure for the PM domain that was removed.
A PM domain should only be removed once the associated PM domain
provider has been removed from the list of providers. Otherwise, it
could be possible for a client to be associated with a PM domain that
could have been removed. Add a helper function to verify if the PM
domain provider is present and only allow a PM domain to be removed if
the provider has been removed.
The function of_genpd_remove_tail() must hold the gpd_list_lock while
finding and removing a PM domain. It is natural for
of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
PM domain is found to remove it. However, pm_genpd_remove(), also
acquires the gpd_list_lock. Therefore, move the core of the function
pm_genpd_remove() to a new function genpd_remove() which does not
acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
and of_genpd_remove_tail().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
include/linux/pm_domain.h | 7 ++++
2 files changed, 88 insertions(+), 6 deletions(-)
@@ -1376,12 +1377,10 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)if(IS_ERR_OR_NULL(genpd))return-EINVAL;-mutex_lock(&gpd_list_lock);mutex_lock(&genpd->lock);if(!list_empty(&genpd->master_links)||genpd->device_count){mutex_unlock(&genpd->lock);-mutex_unlock(&gpd_list_lock);pr_err("%s: unable to remove %s\n",__func__,genpd->name);return-EBUSY;}
@@ -1395,11 +1394,25 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)list_del(&genpd->gpd_list_node);mutex_unlock(&genpd->lock);cancel_work_sync(&genpd->power_off_work);-mutex_unlock(&gpd_list_lock);pr_debug("%s: removed %s\n",__func__,genpd->name);returnret;}++/**+*pm_genpd_remove-RemoveagenericI/OPMdomain+*@genpd:PointertoPMdomainthatistoberemoved.+*/+intpm_genpd_remove(structgeneric_pm_domain*genpd)+{+intret;++mutex_lock(&gpd_list_lock);+ret=genpd_remove(genpd);+mutex_unlock(&gpd_list_lock);++returnret;+}EXPORT_SYMBOL_GPL(pm_genpd_remove);
All above changes could have been made already in the patch when
adding the pm_genpd_remove() API. Could you please fold these changes
into that patch instead?
quoted hunk
#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
@@ -1610,6 +1623,26 @@ void of_genpd_del_provider(struct device_node *np) EXPORT_SYMBOL_GPL(of_genpd_del_provider); /**+ * genpd_provider_present() - Verify if a PM domain provider is present+ * @np: Device node pointer associated with the PM domain provider+ */+static bool genpd_provider_present(struct device_node *np)+{+ struct of_genpd_provider *cp;++ mutex_lock(&of_genpd_mutex);+ list_for_each_entry(cp, &of_genpd_providers, link) {+ if (cp->node == np) {+ mutex_unlock(&of_genpd_mutex);+ return true;+ }+ }+ mutex_unlock(&of_genpd_mutex);++ return false;+}++/** * genpd_get_from_provider() - Look-up PM domain * @genpdspec: OF phandle args to use for look-up *
@@ -1713,6 +1746,48 @@ out: EXPORT_SYMBOL_GPL(of_genpd_add_subdomain); /**+ * of_genpd_remove_tail - Remove the last PM domain registered for a provider+ * @provider: Pointer to device structure associated with provider
The naming of this function would be okay, if we only have added
genpds in the gpd_list by using list_add_tail(), although we don't.
Instead we use list_add() and put them first in the list.
So, unless we change to use list_add_tail() when adding genpds (I
assume we can do that!), I would rather change the name of this
function to of_genpd_remove_first().
What option do you prefer?
+ *
+ * Find the last PM domain that was added by a particular provider and
+ * remove this PM domain from the list of PM domains. The provider is
+ * identified by the 'provider' device structure that is passed. The PM
+ * domain will only be removed, if the provider associated with domain
+ * has been removed.
+ *
+ * Returns a valid pointer to struct generic_pm_domain on success or
+ * ERR_PTR() on failure.
+ */
+struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
+{
+ struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
+ int ret;
+
+ if (IS_ERR_OR_NULL(np))
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&gpd_list_lock);
+ list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+ if (gpd->provider == &np->fwnode) {
+ if (!genpd_provider_present(np)) {
+ ret = genpd_remove(gpd);
+ genpd = ret ? ERR_PTR(ret) : gpd;
+ break;
+ }
Maybe use an "else" here instead to avoid the double "break;".
quoted hunk
+ pr_warn("Provider maybe present, unable to remove %s\n",
+ genpd->name);
+ genpd = ERR_PTR(-EBUSY);
+ break;
+ }
+ }
+ mutex_unlock(&gpd_list_lock);
+
+ return genpd;
+}
+EXPORT_SYMBOL_GPL(of_genpd_remove_tail);
+
+/**
* genpd_dev_pm_detach - Detach a device from its PM domain.
* @dev: Device to detach.
* @power_off: Currently not used
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
When a PM domain provider is added, there is currently no way to tell if
any PM domains associated with the provider are present. Naturally, the
PM domain provider should not be registered if the PM domains have not
been added. Nonetheless, verify that the PM domain(s) associated with a
provider are present when registering the PM domain provider.
This change adds a dependency on the function pm_genpd_present() when
CONFIG_PM_GENERIC_DOMAINS_OF is enabled and so ensure this function is
available when CONFIG_PM_GENERIC_DOMAINS_OF selected.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 42 insertions(+), 3 deletions(-)
You could simplify this, by assigning ret and initial value of
-EINVAL, then do like this:
if (pm_genpd_present(genpd))
ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.
Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.
PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.
I think we should also check if the there's is a provider registered
for the genpd, as it should also prevent the genpd from being removed.
Right?
Yes I would agree. I had thought that after patch #4 of this series that
only the provider itself would be able to call this. However, we should
probably still verify that the provider has correctly remove itself.
I noticed that you are adding the ->provider pointer to the genpd
struct in patch 9/10. Perhaps re-structure $subject patch and 9/10 a
bit to deal with this, so we can add the pm_genpd_remove() API in a
more safe manner.
Yes I can add the >provider member before this patch.
Cheers
Jon
--
nvpublic
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
It is possible that a device has more than one provider of PM domains
and to support the removal of a PM domain by provider, it is necessary
to store a reference to the provider in the PM domain structure.
Therefore, store a reference to the firmware node handle in the PM
domain structure and populate it when providers (only device-tree based
providers are currently supported by PM domains) are registered.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 18 ++++++++++++++----
include/linux/pm_domain.h | 1 +
2 files changed, 15 insertions(+), 4 deletions(-)
@@ -51,6 +51,7 @@ struct generic_pm_domain {structmutexlock;structdev_power_governor*gov;structwork_structpower_off_work;+structfwnode_handle*provider;/* Identity of the domain provider */constchar*name;atomic_tsd_count;/* Number of subdomains with power "on" */enumgpd_statusstatus;/* Current state of the domain */--
2.1.4
I also think you should extend this change, to also make the
of_genpd_del_provider() API to reset the genpd->provider = NULL.
Otherwise you can't track when a provider is removed.
Unfortunately that is not going to work. The function
of_genpd_remove_tail() (patch #10) uses the ->provider member to remove
the last domain for the given provider and of_genpd_del_provider() must
be called before hand.
Cheers
Jon
--
nvpublic
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
If a device supports PM domains that are subdomains of another PM
domain, then the PM domains should be removed in reverse order to
ensure that the subdomains are removed first. Furthermore, if there is
more than one provider, then there needs to be a way to remove the
domains in reverse order for a specific provider.
Add the function of_genpd_remove_tail() to remove the last PM domain
added by a given PM domain provider and return the generic_pm_domain
structure for the PM domain that was removed.
A PM domain should only be removed once the associated PM domain
provider has been removed from the list of providers. Otherwise, it
could be possible for a client to be associated with a PM domain that
could have been removed. Add a helper function to verify if the PM
domain provider is present and only allow a PM domain to be removed if
the provider has been removed.
The function of_genpd_remove_tail() must hold the gpd_list_lock while
finding and removing a PM domain. It is natural for
of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
PM domain is found to remove it. However, pm_genpd_remove(), also
acquires the gpd_list_lock. Therefore, move the core of the function
pm_genpd_remove() to a new function genpd_remove() which does not
acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
and of_genpd_remove_tail().
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
include/linux/pm_domain.h | 7 ++++
2 files changed, 88 insertions(+), 6 deletions(-)
@@ -1376,12 +1377,10 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)if(IS_ERR_OR_NULL(genpd))return-EINVAL;-mutex_lock(&gpd_list_lock);mutex_lock(&genpd->lock);if(!list_empty(&genpd->master_links)||genpd->device_count){mutex_unlock(&genpd->lock);-mutex_unlock(&gpd_list_lock);pr_err("%s: unable to remove %s\n",__func__,genpd->name);return-EBUSY;}
@@ -1395,11 +1394,25 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)list_del(&genpd->gpd_list_node);mutex_unlock(&genpd->lock);cancel_work_sync(&genpd->power_off_work);-mutex_unlock(&gpd_list_lock);pr_debug("%s: removed %s\n",__func__,genpd->name);returnret;}++/**+*pm_genpd_remove-RemoveagenericI/OPMdomain+*@genpd:PointertoPMdomainthatistoberemoved.+*/+intpm_genpd_remove(structgeneric_pm_domain*genpd)+{+intret;++mutex_lock(&gpd_list_lock);+ret=genpd_remove(genpd);+mutex_unlock(&gpd_list_lock);++returnret;+}EXPORT_SYMBOL_GPL(pm_genpd_remove);
All above changes could have been made already in the patch when
adding the pm_genpd_remove() API. Could you please fold these changes
into that patch instead?
Ok. I was not sure if it would seem odd to add pm_genpd_remove() and
genpd_remove() in the same patch because pm_genpd_remove() is the only
user of genpd_remove(). However, it would simplify the diff and so I am
fine with that.
quoted
#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
@@ -1610,6 +1623,26 @@ void of_genpd_del_provider(struct device_node *np) EXPORT_SYMBOL_GPL(of_genpd_del_provider); /**+ * genpd_provider_present() - Verify if a PM domain provider is present+ * @np: Device node pointer associated with the PM domain provider+ */+static bool genpd_provider_present(struct device_node *np)+{+ struct of_genpd_provider *cp;++ mutex_lock(&of_genpd_mutex);+ list_for_each_entry(cp, &of_genpd_providers, link) {+ if (cp->node == np) {+ mutex_unlock(&of_genpd_mutex);+ return true;+ }+ }+ mutex_unlock(&of_genpd_mutex);++ return false;+}++/** * genpd_get_from_provider() - Look-up PM domain * @genpdspec: OF phandle args to use for look-up *
@@ -1713,6 +1746,48 @@ out: EXPORT_SYMBOL_GPL(of_genpd_add_subdomain); /**+ * of_genpd_remove_tail - Remove the last PM domain registered for a provider+ * @provider: Pointer to device structure associated with provider
The naming of this function would be okay, if we only have added
genpds in the gpd_list by using list_add_tail(), although we don't.
Instead we use list_add() and put them first in the list.
So, unless we change to use list_add_tail() when adding genpds (I
assume we can do that!), I would rather change the name of this
function to of_genpd_remove_first().
What option do you prefer?
I think that I would prefer either of_genpd_remove_last() or
of_genpd_remove_one(). Although _first is accurate from the list
perspective it seems odd from the user perspective. I think that _last
is more meaningful as we are removing the last that was added regardless
or how things appear on the list. Alternatively, _one could be a good
compromise.
quoted
+ *
+ * Find the last PM domain that was added by a particular provider and
+ * remove this PM domain from the list of PM domains. The provider is
+ * identified by the 'provider' device structure that is passed. The PM
+ * domain will only be removed, if the provider associated with domain
+ * has been removed.
+ *
+ * Returns a valid pointer to struct generic_pm_domain on success or
+ * ERR_PTR() on failure.
+ */
+struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
+{
+ struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
+ int ret;
+
+ if (IS_ERR_OR_NULL(np))
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&gpd_list_lock);
+ list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+ if (gpd->provider == &np->fwnode) {
+ if (!genpd_provider_present(np)) {
+ ret = genpd_remove(gpd);
+ genpd = ret ? ERR_PTR(ret) : gpd;
+ break;
+ }
Maybe use an "else" here instead to avoid the double "break;".
/**
+ * of_genpd_remove_tail - Remove the last PM domain registered for a provider
+ * @provider: Pointer to device structure associated with provider
The naming of this function would be okay, if we only have added
genpds in the gpd_list by using list_add_tail(), although we don't.
Instead we use list_add() and put them first in the list.
So, unless we change to use list_add_tail() when adding genpds (I
assume we can do that!), I would rather change the name of this
function to of_genpd_remove_first().
What option do you prefer?
I think that I would prefer either of_genpd_remove_last() or
of_genpd_remove_one(). Although _first is accurate from the list
perspective it seems odd from the user perspective. I think that _last
is more meaningful as we are removing the last that was added regardless
or how things appear on the list. Alternatively, _one could be a good
compromise.
I am fine with of_genpd_remove_last().
[...]
Kind regards
Uffe
I also think you should extend this change, to also make the
of_genpd_del_provider() API to reset the genpd->provider = NULL.
Otherwise you can't track when a provider is removed.
Unfortunately that is not going to work. The function
of_genpd_remove_tail() (patch #10) uses the ->provider member to remove
the last domain for the given provider and of_genpd_del_provider() must
be called before hand.
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.
Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.
PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.
I think we should also check if the there's is a provider registered
for the genpd, as it should also prevent the genpd from being removed.
Right?
Yes I would agree. I had thought that after patch #4 of this series that
only the provider itself would be able to call this. However, we should
probably still verify that the provider has correctly remove itself.
So now I have the following. I am still not 100% happy. I cannot clear
the ->provider when calling of_genpd_del_provider() and so I cannot use
this to verify if the provider is present and so I need to check the
list of providers and it gets a bit messy. I have been wracking my
brains to find a better alternative (including a single function to
remove the provider and domains at once but there are issues with that
as well).
I think that long term it may make sense to reference the providers
exclusively by the fwnode_handle and make the list of provider non-DT
specific. I could do it now, but it would increase the series.
Cheers
Jon
---
drivers/base/power/domain.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 5 +++
2 files changed, 94 insertions(+)
On 9 September 2016 at 17:17, Jon Hunter [off-list ref] wrote:
On 09/09/16 14:54, Jon Hunter wrote:
quoted
On 08/09/16 12:49, Ulf Hansson wrote:
quoted
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.
Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.
PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.
I think we should also check if the there's is a provider registered
for the genpd, as it should also prevent the genpd from being removed.
Right?
Yes I would agree. I had thought that after patch #4 of this series that
only the provider itself would be able to call this. However, we should
probably still verify that the provider has correctly remove itself.
So now I have the following. I am still not 100% happy. I cannot clear
the ->provider when calling of_genpd_del_provider() and so I cannot use
this to verify if the provider is present and so I need to check the
list of providers and it gets a bit messy. I have been wracking my
brains to find a better alternative (including a single function to
remove the provider and domains at once but there are issues with that
as well).
Instead of using the ->provider pointer to know whether the genpd has
a valid provider, why not just add an additional ->has_provider bool
flag in the genpd struct?
Simply set the flag when adding the provider and reset it when
removing it. Wouldn't that work?
I think that long term it may make sense to reference the providers
exclusively by the fwnode_handle and make the list of provider non-DT
specific. I could do it now, but it would increase the series.
Perhaps a good idea. Although I agree, let's not make that change as a
part of this series.
[...]
Kind regards
Uffe
On 9 September 2016 at 17:17, Jon Hunter [off-list ref] wrote:
quoted
On 09/09/16 14:54, Jon Hunter wrote:
quoted
On 08/09/16 12:49, Ulf Hansson wrote:
quoted
On 16 August 2016 at 11:49, Jon Hunter [off-list ref] wrote:
quoted
The genpd framework allows users to add PM domains via the pm_genpd_init()
function, however, there is no corresponding function to remove a PM
domain. For most devices this may be fine as the PM domains are never
removed, however, for devices that wish to populate the PM domains from
within a driver, having the ability to remove a PM domain if the probing
of the device fails or the driver is unloaded is necessary.
Add the function pm_genpd_remove() to remove a PM domain by referencing
it's generic_pm_domain structure.
PM domains can only be removed if they are not a parent domain to
another PM domain and have no devices associated with them.
I think we should also check if the there's is a provider registered
for the genpd, as it should also prevent the genpd from being removed.
Right?
Yes I would agree. I had thought that after patch #4 of this series that
only the provider itself would be able to call this. However, we should
probably still verify that the provider has correctly remove itself.
So now I have the following. I am still not 100% happy. I cannot clear
the ->provider when calling of_genpd_del_provider() and so I cannot use
this to verify if the provider is present and so I need to check the
list of providers and it gets a bit messy. I have been wracking my
brains to find a better alternative (including a single function to
remove the provider and domains at once but there are issues with that
as well).
Instead of using the ->provider pointer to know whether the genpd has
a valid provider, why not just add an additional ->has_provider bool
flag in the genpd struct?
Simply set the flag when adding the provider and reset it when
removing it. Wouldn't that work?
Yes. I was trying not to add to much clutter to the struct. However, may
be this is the best option.
Cheers
Jon
--
nvpublic