| Message ID | 1456501724-28477-4-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Feb 26, 2016 at 03:48:37PM +0000, Jon Hunter wrote: > The genpd framework allows users to add power-domains via the > pm_genpd_init() function, however, there is no corresponding function > to remove a power-domain. For most devices this may be fine as the power > domains are never removed, however, for devices that wish to populate > the power-domains from within a driver, having the ability to remove a > power domain if the probing of the device fails or the driver is unloaded > is necessary. Therefore, add a function to remove a power-domain. Please > note that the power domain can only be removed if there are no devices > using the power-domain and it is not linked to another domain. So I guess this introduces a problem not uncommon to other types of resources. If you remove the driver, even if you fail ->remove() the module may still go away along with any code associated with it. So even if the PM domains can't be removed, you can't prevent the module from going away. That could be somewhat mitigated if we were holding a module reference count, because then the only way to unload the driver would be via sysfs (we could prevent that too, which might be the best way to do this today). There is work underway to solve this generically, Rafael was working on this, so I'm not sure we need to add additional infrastructure to the PM domain code as part of this series. But we may want to mark the Tegra PMC driver as .suppress_bind_attrs = true, to make sure it can't be removed. > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 5 +++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 608bc00655ee..22f6e9d738bf 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1557,6 +1557,37 @@ 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 object. > + * @genpd: PM domain object to remove. > + */ Would it be worth noting here that the object is only removed from the list of domains, but that users are still responsible for freeing any resources (such as memory) associated with it? It seems like this might be obvious enough given the usage model of pm_genpd_init(), but it's something that I noticed missing from the function. Otherwise this looks fine: Reviewed-by: Thierry Reding <treding@nvidia.com>
On 29/02/16 07:15, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Fri, Feb 26, 2016 at 03:48:37PM +0000, Jon Hunter wrote: >> The genpd framework allows users to add power-domains via the >> pm_genpd_init() function, however, there is no corresponding function >> to remove a power-domain. For most devices this may be fine as the power >> domains are never removed, however, for devices that wish to populate >> the power-domains from within a driver, having the ability to remove a >> power domain if the probing of the device fails or the driver is unloaded >> is necessary. Therefore, add a function to remove a power-domain. Please >> note that the power domain can only be removed if there are no devices >> using the power-domain and it is not linked to another domain. > > So I guess this introduces a problem not uncommon to other types of > resources. If you remove the driver, even if you fail ->remove() the > module may still go away along with any code associated with it. So > even if the PM domains can't be removed, you can't prevent the module > from going away. That could be somewhat mitigated if we were holding a > module reference count, because then the only way to unload the driver > would be via sysfs (we could prevent that too, which might be the best > way to do this today). > > There is work underway to solve this generically, Rafael was working on > this, so I'm not sure we need to add additional infrastructure to the PM > domain code as part of this series. But we may want to mark the Tegra > PMC driver as .suppress_bind_attrs = true, to make sure it can't be > removed. I see what you are saying. It nearly seemed to me that I should use BUG_ON() if we fail to remove a genpd in the PMC driver, however, I know that using BUG_ON should be avoided. I will add the suppress_bind_attrs = true for the PMC driver as that does make sense for now. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/02/16 10:14, Jon Hunter wrote: > > On 29/02/16 07:15, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Fri, Feb 26, 2016 at 03:48:37PM +0000, Jon Hunter wrote: >>> The genpd framework allows users to add power-domains via the >>> pm_genpd_init() function, however, there is no corresponding function >>> to remove a power-domain. For most devices this may be fine as the power >>> domains are never removed, however, for devices that wish to populate >>> the power-domains from within a driver, having the ability to remove a >>> power domain if the probing of the device fails or the driver is unloaded >>> is necessary. Therefore, add a function to remove a power-domain. Please >>> note that the power domain can only be removed if there are no devices >>> using the power-domain and it is not linked to another domain. >> >> So I guess this introduces a problem not uncommon to other types of >> resources. If you remove the driver, even if you fail ->remove() the >> module may still go away along with any code associated with it. So >> even if the PM domains can't be removed, you can't prevent the module >> from going away. That could be somewhat mitigated if we were holding a >> module reference count, because then the only way to unload the driver >> would be via sysfs (we could prevent that too, which might be the best >> way to do this today). >> >> There is work underway to solve this generically, Rafael was working on >> this, so I'm not sure we need to add additional infrastructure to the PM >> domain code as part of this series. But we may want to mark the Tegra >> PMC driver as .suppress_bind_attrs = true, to make sure it can't be >> removed. > > I see what you are saying. It nearly seemed to me that I should use > BUG_ON() if we fail to remove a genpd in the PMC driver, however, I know > that using BUG_ON should be avoided. I will add the suppress_bind_attrs > = true for the PMC driver as that does make sense for now. Just to let you know, the PMC driver already has suppress_bind_attrs = true. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 February 2016 at 16:48, Jon Hunter <jonathanh@nvidia.com> wrote: > The genpd framework allows users to add power-domains via the > pm_genpd_init() function, however, there is no corresponding function > to remove a power-domain. For most devices this may be fine as the power > domains are never removed, however, for devices that wish to populate > the power-domains from within a driver, having the ability to remove a > power domain if the probing of the device fails or the driver is unloaded > is necessary. Therefore, add a function to remove a power-domain. Please > note that the power domain can only be removed if there are no devices > using the power-domain and it is not linked to another domain. In general I think you can work a bit on the changelog, for example split it into a few sections to make it more readable. I would like it to state clearly what constraint we have to be able to remove a PM domain. More precisely, the device structure is needed, no of genpd provider etc. Maybe also "linked to another domain" can be rephrased to tell about both masters and subdomains. Finally, perhaps change terminology for the "power-domain" to "PM domain" as I think that's more widely used. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 5 +++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 608bc00655ee..22f6e9d738bf 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1557,6 +1557,37 @@ 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 object. > + * @genpd: PM domain object to remove. > + */ > +int pm_genpd_remove(struct generic_pm_domain *genpd) > +{ > + int ret = 0; > + > + if (IS_ERR_OR_NULL(genpd)) > + return -EINVAL; > + > + mutex_lock(&gpd_list_lock); > + mutex_lock(&genpd->lock); > + > + if (!list_empty(&genpd->master_links) > + || !list_empty(&genpd->slave_links) || genpd->device_count) { > + ret = -EBUSY; > + goto out; > + } > + > + cancel_work_sync(&genpd->power_off_work); This can deadlock as the work function tries to lock the genpd mutex. I suggest to move this outside of the locking section just before returning, but of course still only in case of a non-error. > + list_del(&genpd->gpd_list_node); > + > +out: > + mutex_unlock(&genpd->lock); > + mutex_unlock(&gpd_list_lock); > + Besides the minor comment above. 1) Have you considered how to protects against a registered of_genpd_provider for the genpd? 2) Theoretically some could call pm_genpd_add_subdomain() after this point. I guess we can consider that as an abuse of the API, although do you think we need to protect that from really happening? > + return ret; > +} > +EXPORT_SYMBOL_GPL(pm_genpd_remove); > + > #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 b38dd74dea9b..9ae9c42f807f 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -134,6 +134,7 @@ 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 dev_power_governor simple_qos_governor; > extern struct dev_power_governor pm_domain_always_on_gov; > @@ -177,6 +178,10 @@ 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; > +} > #endif > > static inline int pm_genpd_add_device(struct generic_pm_domain *genpd, > -- > 2.1.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 608bc00655ee..22f6e9d738bf 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1557,6 +1557,37 @@ 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 object. + * @genpd: PM domain object to remove. + */ +int pm_genpd_remove(struct generic_pm_domain *genpd) +{ + int ret = 0; + + if (IS_ERR_OR_NULL(genpd)) + return -EINVAL; + + mutex_lock(&gpd_list_lock); + mutex_lock(&genpd->lock); + + if (!list_empty(&genpd->master_links) + || !list_empty(&genpd->slave_links) || genpd->device_count) { + ret = -EBUSY; + goto out; + } + + cancel_work_sync(&genpd->power_off_work); + list_del(&genpd->gpd_list_node); + +out: + mutex_unlock(&genpd->lock); + mutex_unlock(&gpd_list_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(pm_genpd_remove); + #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 b38dd74dea9b..9ae9c42f807f 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -134,6 +134,7 @@ 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 dev_power_governor simple_qos_governor; extern struct dev_power_governor pm_domain_always_on_gov; @@ -177,6 +178,10 @@ 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; +} #endif static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
The genpd framework allows users to add power-domains via the pm_genpd_init() function, however, there is no corresponding function to remove a power-domain. For most devices this may be fine as the power domains are never removed, however, for devices that wish to populate the power-domains from within a driver, having the ability to remove a power domain if the probing of the device fails or the driver is unloaded is necessary. Therefore, add a function to remove a power-domain. Please note that the power domain can only be removed if there are no devices using the power-domain and it is not linked to another domain. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++ include/linux/pm_domain.h | 5 +++++ 2 files changed, 36 insertions(+)