| Message ID | 1446590711-18928-2-git-send-email-alex.aring@gmail.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 3 November 2015 at 23:45, Alexander Aring <alex.aring@gmail.com> wrote: > This patch adds function pm_genpd_uninit for undo a pm_genpd_init. This > is useful for multiple power domains while probing. If the probing fails > after one pm_genpd_init was called we need to undo all previous > registrations of generic pm domains inside the gpd_list list. Yes, agree. Although I think it's a bit mote complicated than what you suggest in this simple approach. :-) > > There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again > registered power domains and not registered domains, the driver can use > this mechanism to have an array with registered and non-registered power > domains, where non-registered power domains are NULL. > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Len Brown <len.brown@intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > --- > drivers/base/power/domain.c | 15 +++++++++++++++ > include/linux/pm_domain.h | 4 ++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 16550c6..65b9d1a 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1730,6 +1730,21 @@ void pm_genpd_init(struct generic_pm_domain *genpd, > } > EXPORT_SYMBOL_GPL(pm_genpd_init); > > +/** > + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object. > + * @genpd: PM domain object to initialize. > + */ > +void pm_genpd_uninit(struct generic_pm_domain *genpd) > +{ > + if (IS_ERR_OR_NULL(genpd)) > + return; > + > + mutex_lock(&gpd_list_lock); > + list_del(&genpd->gpd_list_node); > + mutex_unlock(&gpd_list_lock); This is too fragile. You don't protect from the cases below. 1. The genpd may have devices attached to it. 2. The genpd may have subdomains. To deal with these case, that's when it becomes more complex which I guess is the reason to why nobody really cared until now. Moreover, I think there are some more structures to "uninitialize" besides just unlinking the genpd struct from the gpd list. For example a mutex_destroy() should be done. > +} > +EXPORT_SYMBOL_GPL(pm_genpd_uninit); > + > #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 b1cf7e7..45d4f7a 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -143,6 +143,7 @@ extern int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd); > extern int pm_genpd_name_detach_cpuidle(const char *name); > extern void pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off); > +extern void pm_genpd_uninit(struct generic_pm_domain *genpd); > > extern int pm_genpd_poweron(struct generic_pm_domain *genpd); > extern int pm_genpd_name_poweron(const char *domain_name); > @@ -212,6 +213,9 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off) > { > } > +static inline void pm_genpd_uninit(struct generic_pm_domain *genpd) > +{ > +} > static inline int pm_genpd_poweron(struct generic_pm_domain *genpd) > { > return -ENOSYS; > -- > 2.6.1 > 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
On Thu, Nov 05, 2015 at 10:01:32AM +0100, Ulf Hansson wrote: > On 3 November 2015 at 23:45, Alexander Aring <alex.aring@gmail.com> wrote: > > This patch adds function pm_genpd_uninit for undo a pm_genpd_init. This > > is useful for multiple power domains while probing. If the probing fails > > after one pm_genpd_init was called we need to undo all previous > > registrations of generic pm domains inside the gpd_list list. > > Yes, agree. Although I think it's a bit mote complicated than what you > suggest in this simple approach. :-) > ok. > > > > There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again > > registered power domains and not registered domains, the driver can use > > this mechanism to have an array with registered and non-registered power > > domains, where non-registered power domains are NULL. > > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Cc: Kevin Hilman <khilman@kernel.org> > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: Len Brown <len.brown@intel.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > > --- > > drivers/base/power/domain.c | 15 +++++++++++++++ > > include/linux/pm_domain.h | 4 ++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 16550c6..65b9d1a 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -1730,6 +1730,21 @@ void pm_genpd_init(struct generic_pm_domain *genpd, > > } > > EXPORT_SYMBOL_GPL(pm_genpd_init); > > > > +/** > > + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object. > > + * @genpd: PM domain object to initialize. > > + */ > > +void pm_genpd_uninit(struct generic_pm_domain *genpd) > > +{ > > + if (IS_ERR_OR_NULL(genpd)) > > + return; > > + > > + mutex_lock(&gpd_list_lock); > > + list_del(&genpd->gpd_list_node); > > + mutex_unlock(&gpd_list_lock); > > This is too fragile. You don't protect from the cases below. > > 1. The genpd may have devices attached to it. > 2. The genpd may have subdomains. > > To deal with these case, that's when it becomes more complex which I > guess is the reason to why nobody really cared until now. > Can we not just undo the things which pm_genpd_init does, then let the driver to deal with all other uninitialize of e.g. "subdomains" which might the driver has initialize before? We know there are no slaves or something else which is empty because pm_genpd_init runs INIT_LIST_HEAD, or not? To make such handling above I would add some: "pm_genpd_uninit_recursive" function, which cleanup everything from a power domain, e.g. subdomains, etc and other lists. What do you suggest to me for e.g. the raspberrypi power domain driver, also simple ignore such error handling? For my current use-case I can remove the failure between pm_genpd_init by simple getting all "power states" (the bool is_off for pm_genpd_init), before calling pm_genpd_init. In this case I don't have any failure between calling pm_genpd_init when another pm_genpd_init was before. Looks like this: 1. get all states from firmware to get parameter "is_off", which can fail. We should get here the states for the power domains which we want to register only. 2. Then call pm_genpd_init, which cannot be fail between another pm_genpd_init. (There are only void functions in the middle, which cannot fail). If I call pm_genpd_init, I use "is_off" variable from the step of "1.". But then I have still the call of "of_genpd_add_provider_onecell" at the end of probing which can fail. So this is also not a valid solution, because I need to undo everything before. > Moreover, I think there are some more structures to "uninitialize" > besides just unlinking the genpd struct from the gpd list. For example > a mutex_destroy() should be done. > yes, of course. - Alex -- 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
Hi, On Thu, Nov 05, 2015 at 03:34:45PM +0100, Alexander Aring wrote: > > What do you suggest to me for e.g. the raspberrypi power domain driver, > also simple ignore such error handling? > ping, I also can add some WARN_ON_ONCE, if the list for sub-domains, etc. are not empty. This would then report about wrong use of pm_genpd_uninit. - Alex -- 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
[...] >> > >> > +/** >> > + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object. >> > + * @genpd: PM domain object to initialize. >> > + */ >> > +void pm_genpd_uninit(struct generic_pm_domain *genpd) >> > +{ >> > + if (IS_ERR_OR_NULL(genpd)) >> > + return; >> > + >> > + mutex_lock(&gpd_list_lock); >> > + list_del(&genpd->gpd_list_node); >> > + mutex_unlock(&gpd_list_lock); >> >> This is too fragile. You don't protect from the cases below. >> >> 1. The genpd may have devices attached to it. >> 2. The genpd may have subdomains. >> >> To deal with these case, that's when it becomes more complex which I >> guess is the reason to why nobody really cared until now. >> > > Can we not just undo the things which pm_genpd_init does, then let the > driver to deal with all other uninitialize of e.g. "subdomains" which might > the driver has initialize before? We know there are no slaves or something > else which is empty because pm_genpd_init runs INIT_LIST_HEAD, or not? > > To make such handling above I would add some: > > "pm_genpd_uninit_recursive" function, which cleanup everything from a > power domain, e.g. subdomains, etc and other lists. > > What do you suggest to me for e.g. the raspberrypi power domain driver, > also simple ignore such error handling? No, let's give it try so see if can improve the situation. > > > For my current use-case I can remove the failure between pm_genpd_init by > simple getting all "power states" (the bool is_off for pm_genpd_init), > before calling pm_genpd_init. In this case I don't have any failure between > calling pm_genpd_init when another pm_genpd_init was before. > > Looks like this: > > 1. get all states from firmware to get parameter "is_off", which can fail. > We should get here the states for the power domains which we want to > register only. > > 2. Then call pm_genpd_init, which cannot be fail between another > pm_genpd_init. (There are only void functions in the middle, which cannot > fail). If I call pm_genpd_init, I use "is_off" variable from the step > of "1.". > > > But then I have still the call of "of_genpd_add_provider_onecell" at the end > of probing which can fail. So this is also not a valid solution, because > I need to undo everything before. Okay, got it. > >> Moreover, I think there are some more structures to "uninitialize" >> besides just unlinking the genpd struct from the gpd list. For example >> a mutex_destroy() should be done. >> > > yes, of course. > > - Alex 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
On 11 November 2015 at 19:00, Alexander Aring <alex.aring@gmail.com> wrote: > Hi, > > On Thu, Nov 05, 2015 at 03:34:45PM +0100, Alexander Aring wrote: >> >> What do you suggest to me for e.g. the raspberrypi power domain driver, >> also simple ignore such error handling? >> > > ping, I also can add some WARN_ON_ONCE, if the list for sub-domains, > etc. are not empty. This would then report about wrong use of > pm_genpd_uninit. > > - Alex Sorry for the delay. I think what you suggest would be an okay solution, at least it will improve the current behaviour. We should verify for sub-domains, attached devices, and if the genpd has an of-provider. That's all I can think of right now, but there may be other things as well. 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
Hi, On Wed, Nov 11, 2015 at 09:33:40PM +0100, Ulf Hansson wrote: > On 11 November 2015 at 19:00, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > > > On Thu, Nov 05, 2015 at 03:34:45PM +0100, Alexander Aring wrote: > >> > >> What do you suggest to me for e.g. the raspberrypi power domain driver, > >> also simple ignore such error handling? > >> > > > > ping, I also can add some WARN_ON_ONCE, if the list for sub-domains, > > etc. are not empty. This would then report about wrong use of > > pm_genpd_uninit. > > > > - Alex > > Sorry for the delay. > > I think what you suggest would be an okay solution, at least it will > improve the current behaviour. > > We should verify for sub-domains, attached devices, and if the genpd > has an of-provider. That's all I can think of right now, but there may > be other things as well. > okay, I added now: WARN_ON_ONCE(!list_empty(genpd->master_links) || !list_empty(genpd->slave_links) || !list_empty(genpd->dev_list)); So far I understand is master/slave something about domains/subdomains, the dev_list fis for atteched devices. But how can I check "if the genpd has an of-provider", the "struct generic_pm_domain" doesn't know a "of-provider". There is a static list "of_genpd_providers", do you want to iterate over all and then doing some matching algorithmn? Or do you want to add something inside "struct generic_pm_domain", so a genpd knows about the "of-provider". I am currently confused about how to check on "of-provider". - Alex -- 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 16550c6..65b9d1a 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1730,6 +1730,21 @@ void pm_genpd_init(struct generic_pm_domain *genpd, } EXPORT_SYMBOL_GPL(pm_genpd_init); +/** + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object. + * @genpd: PM domain object to initialize. + */ +void pm_genpd_uninit(struct generic_pm_domain *genpd) +{ + if (IS_ERR_OR_NULL(genpd)) + return; + + mutex_lock(&gpd_list_lock); + list_del(&genpd->gpd_list_node); + mutex_unlock(&gpd_list_lock); +} +EXPORT_SYMBOL_GPL(pm_genpd_uninit); + #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 b1cf7e7..45d4f7a 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -143,6 +143,7 @@ extern int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd); extern int pm_genpd_name_detach_cpuidle(const char *name); extern void pm_genpd_init(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off); +extern void pm_genpd_uninit(struct generic_pm_domain *genpd); extern int pm_genpd_poweron(struct generic_pm_domain *genpd); extern int pm_genpd_name_poweron(const char *domain_name); @@ -212,6 +213,9 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off) { } +static inline void pm_genpd_uninit(struct generic_pm_domain *genpd) +{ +} static inline int pm_genpd_poweron(struct generic_pm_domain *genpd) { return -ENOSYS;
This patch adds function pm_genpd_uninit for undo a pm_genpd_init. This is useful for multiple power domains while probing. If the probing fails after one pm_genpd_init was called we need to undo all previous registrations of generic pm domains inside the gpd_list list. There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again registered power domains and not registered domains, the driver can use this mechanism to have an array with registered and non-registered power domains, where non-registered power domains are NULL. Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@kernel.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Pavel Machek <pavel@ucw.cz> Cc: Len Brown <len.brown@intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Alexander Aring <alex.aring@gmail.com> --- drivers/base/power/domain.c | 15 +++++++++++++++ include/linux/pm_domain.h | 4 ++++ 2 files changed, 19 insertions(+)