Message ID | 20201203233546.3482813-1-tstrudel@google.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | PM: domains: create debugfs nodes when adding power domains | expand |
On Fri, 4 Dec 2020 at 00:35, Thierry Strudel <tstrudel@google.com> wrote: > > debugfs nodes were created in genpd_debug_init alled in late_initcall > preventing power domains registered though loadable modules to have > a debugfs entry. > > Create/remove debugfs nodes when the power domain is added/removed > to/from the internal gpd_list. > > Signed-off-by: Thierry Strudel <tstrudel@google.com> > --- > drivers/base/power/domain.c | 61 ++++++++++++++++++++++++------------- > 1 file changed, 39 insertions(+), 22 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 743268996336..3f168ec93762 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1880,6 +1880,8 @@ static void genpd_lock_init(struct generic_pm_domain *genpd) > } > } > > +static void genpd_debug_add(struct generic_pm_domain *genpd); > + > /** > * pm_genpd_init - Initialize a generic I/O PM domain object. > * @genpd: PM domain object to initialize. > @@ -1954,12 +1956,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > > mutex_lock(&gpd_list_lock); > list_add(&genpd->gpd_list_node, &gpd_list); > + genpd_debug_add(genpd); > mutex_unlock(&gpd_list_lock); > > return 0; > } > EXPORT_SYMBOL_GPL(pm_genpd_init); > > +static void genpd_debug_remove(struct generic_pm_domain *genpd); > + Please avoid these forward declarations. I think it's better to move the code around. > static int genpd_remove(struct generic_pm_domain *genpd) > { > struct gpd_link *l, *link; > @@ -1987,6 +1992,7 @@ static int genpd_remove(struct generic_pm_domain *genpd) > kfree(link); > } > > + genpd_debug_remove(genpd); > list_del(&genpd->gpd_list_node); > genpd_unlock(genpd); > cancel_work_sync(&genpd->power_off_work); > @@ -3177,36 +3183,44 @@ DEFINE_SHOW_ATTRIBUTE(total_idle_time); > DEFINE_SHOW_ATTRIBUTE(devices); > DEFINE_SHOW_ATTRIBUTE(perf_state); > > -static int __init genpd_debug_init(void) > +static void genpd_debug_add(struct generic_pm_domain *genpd) > { > struct dentry *d; > - struct generic_pm_domain *genpd; > > + d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); What happens if "genpd_debugfs_dir" is NULL, which will be the case until the late_initcall(genpd_debug_init) has been executed!? > + > + debugfs_create_file("current_state", 0444, > + d, genpd, &status_fops); > + debugfs_create_file("sub_domains", 0444, > + d, genpd, &sub_domains_fops); > + debugfs_create_file("idle_states", 0444, > + d, genpd, &idle_states_fops); > + debugfs_create_file("active_time", 0444, > + d, genpd, &active_time_fops); > + debugfs_create_file("total_idle_time", 0444, > + d, genpd, &total_idle_time_fops); > + debugfs_create_file("devices", 0444, > + d, genpd, &devices_fops); > + if (genpd->set_performance_state) > + debugfs_create_file("perf_state", 0444, > + d, genpd, &perf_state_fops); > +} [...] Kind regards Uffe
Hello Uffe, > > > > +static void genpd_debug_remove(struct generic_pm_domain *genpd); > > + > > Please avoid these forward declarations. I think it's better to move > the code around. I can move up static void genpd_debug_remove(struct generic_pm_domain *genpd) but moving static void genpd_debug_add(struct generic_pm_domain *genpd) requires moving all those functions implementation: DEFINE_SHOW_ATTRIBUTE(summary); DEFINE_SHOW_ATTRIBUTE(status); DEFINE_SHOW_ATTRIBUTE(sub_domains); DEFINE_SHOW_ATTRIBUTE(idle_states); DEFINE_SHOW_ATTRIBUTE(active_time); DEFINE_SHOW_ATTRIBUTE(total_idle_time); DEFINE_SHOW_ATTRIBUTE(devices); DEFINE_SHOW_ATTRIBUTE(perf_state); are you fine keeping static void genpd_debug_add(struct generic_pm_domain *genpd) as a forward declaration ? > > > static int genpd_remove(struct generic_pm_domain *genpd) > > { > > struct gpd_link *l, *link; > > @@ -1987,6 +1992,7 @@ static int genpd_remove(struct generic_pm_domain *genpd) > > kfree(link); > > } > > > > + genpd_debug_remove(genpd); > > list_del(&genpd->gpd_list_node); > > genpd_unlock(genpd); > > cancel_work_sync(&genpd->power_off_work); > > @@ -3177,36 +3183,44 @@ DEFINE_SHOW_ATTRIBUTE(total_idle_time); > > DEFINE_SHOW_ATTRIBUTE(devices); > > DEFINE_SHOW_ATTRIBUTE(perf_state); > > > > -static int __init genpd_debug_init(void) > > +static void genpd_debug_add(struct generic_pm_domain *genpd) > > { > > struct dentry *d; > > - struct generic_pm_domain *genpd; > > > > + d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); > > What happens if "genpd_debugfs_dir" is NULL, which will be the case > until the late_initcall(genpd_debug_init) has been executed!? Good point, I'll return early if NULL and in static int __init genpd_debug_init(void) I'll iterate on the list to create the nodes > Kind regards > Uffe Best regards Thierry
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 743268996336..3f168ec93762 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1880,6 +1880,8 @@ static void genpd_lock_init(struct generic_pm_domain *genpd) } } +static void genpd_debug_add(struct generic_pm_domain *genpd); + /** * pm_genpd_init - Initialize a generic I/O PM domain object. * @genpd: PM domain object to initialize. @@ -1954,12 +1956,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, mutex_lock(&gpd_list_lock); list_add(&genpd->gpd_list_node, &gpd_list); + genpd_debug_add(genpd); mutex_unlock(&gpd_list_lock); return 0; } EXPORT_SYMBOL_GPL(pm_genpd_init); +static void genpd_debug_remove(struct generic_pm_domain *genpd); + static int genpd_remove(struct generic_pm_domain *genpd) { struct gpd_link *l, *link; @@ -1987,6 +1992,7 @@ static int genpd_remove(struct generic_pm_domain *genpd) kfree(link); } + genpd_debug_remove(genpd); list_del(&genpd->gpd_list_node); genpd_unlock(genpd); cancel_work_sync(&genpd->power_off_work); @@ -3177,36 +3183,44 @@ DEFINE_SHOW_ATTRIBUTE(total_idle_time); DEFINE_SHOW_ATTRIBUTE(devices); DEFINE_SHOW_ATTRIBUTE(perf_state); -static int __init genpd_debug_init(void) +static void genpd_debug_add(struct generic_pm_domain *genpd) { struct dentry *d; - struct generic_pm_domain *genpd; + d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); + + debugfs_create_file("current_state", 0444, + d, genpd, &status_fops); + debugfs_create_file("sub_domains", 0444, + d, genpd, &sub_domains_fops); + debugfs_create_file("idle_states", 0444, + d, genpd, &idle_states_fops); + debugfs_create_file("active_time", 0444, + d, genpd, &active_time_fops); + debugfs_create_file("total_idle_time", 0444, + d, genpd, &total_idle_time_fops); + debugfs_create_file("devices", 0444, + d, genpd, &devices_fops); + if (genpd->set_performance_state) + debugfs_create_file("perf_state", 0444, + d, genpd, &perf_state_fops); +} + +static void genpd_debug_remove(struct generic_pm_domain *genpd) +{ + struct dentry *d; + + d = debugfs_lookup(genpd->name, genpd_debugfs_dir); + debugfs_remove(d); +} + +static int __init genpd_debug_init(void) +{ genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL); debugfs_create_file("pm_genpd_summary", S_IRUGO, genpd_debugfs_dir, NULL, &summary_fops); - list_for_each_entry(genpd, &gpd_list, gpd_list_node) { - d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); - - debugfs_create_file("current_state", 0444, - d, genpd, &status_fops); - debugfs_create_file("sub_domains", 0444, - d, genpd, &sub_domains_fops); - debugfs_create_file("idle_states", 0444, - d, genpd, &idle_states_fops); - debugfs_create_file("active_time", 0444, - d, genpd, &active_time_fops); - debugfs_create_file("total_idle_time", 0444, - d, genpd, &total_idle_time_fops); - debugfs_create_file("devices", 0444, - d, genpd, &devices_fops); - if (genpd->set_performance_state) - debugfs_create_file("perf_state", 0444, - d, genpd, &perf_state_fops); - } - return 0; } late_initcall(genpd_debug_init); @@ -3216,4 +3230,7 @@ static void __exit genpd_debug_exit(void) debugfs_remove_recursive(genpd_debugfs_dir); } __exitcall(genpd_debug_exit); +#else +static void genpd_debug_add(struct generic_pm_domain *genpd) {} +static void genpd_debug_remove(struct generic_pm_domain *genpd) {} #endif /* CONFIG_DEBUG_FS */
debugfs nodes were created in genpd_debug_init alled in late_initcall preventing power domains registered though loadable modules to have a debugfs entry. Create/remove debugfs nodes when the power domain is added/removed to/from the internal gpd_list. Signed-off-by: Thierry Strudel <tstrudel@google.com> --- drivers/base/power/domain.c | 61 ++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 22 deletions(-)