Re: [PATCH RFC 06/27] PM / Domains: add debugfs 'states' and 'timings' seq files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/12/2015 12:46, Ulf Hansson wrote:
On 17 November 2015 at 23:37, Lina Iyer <lina.iyer@xxxxxxxxxx> wrote:


Hi Ulf,

thanks for the review, comments and questions below.

Best Regards,
Marc.

From: Marc Titinger <mtitinger@xxxxxxxxxxxx>

This purpose of these debug seq-files is to help investigate
generic power domain state transitions, based on device constraints.
requires the "multiple states" patches from Axel Haslam.

This last sentence doesn't belong in the change-log, please remove it.


also rename 'summary' from 'pm_genpd_summary'

sample output for 'states'
==========================

   Domain             State name        Eval = 2200nter + Exit = Min_off_on (ns)
-----------------------------------------------------------------------
a53_pd               cluster-sleep-0      1500000+800000=2300000
a57_pd               d1-retention         1000000+800000=1800000
a57_pd               cluster-sleep-0      1500000+800000=2300000

sample output for 'timings'
===========================

     Domain Devices, Timings in ns
                        Stop/Start Save/Restore, Effective
----------------------------------------------------  ---
a53_pd
     /cpus/cpu@100        1060  /660    1580  /1940  ,0 (cached stop)
     /cpus/cpu@101        1060  /740    1520  /1600  ,0 (cached stop)
     /cpus/cpu@102        880   /620    1380  /1780  ,0 (cached stop)
     /cpus/cpu@103        1080  /640    1340  /1600  ,0 (cached stop)
a57_pd
     /cpus/cpu@0          1160  /740    3280  /1800  ,0 (cached stop)
     /cpus/cpu@1          780   /1400   1440  /2080  ,0 (cached stop)
     /D1                  600   /540    7140  /6420  ,2199460 (cached stop)

Signed-off-by: Marc Titinger <mtitinger@xxxxxxxxxxxx>

A general comment. Static functions in genpd shall start with one of
the following prefix.

genpd_*
_genpd_*
__genpd_*

Please change accordingly.

Many static routines were already prefixed like the exported functions with "pm_", shall I make a separate patch for this renaming ?


---
  drivers/base/power/domain.c | 115 +++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 107 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index c300293..9a0df09 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2169,21 +2169,120 @@ static const struct file_operations pm_genpd_summary_fops = {
         .release = single_release,
  };

+static int pm_genpd_states_show(struct seq_file *s, void *data)
+{
+       struct generic_pm_domain *genpd;
+
+       seq_puts(s,
+                "\n  Domain             State name        Enter + Exit = Min_off_on (ns)\n");
+       seq_puts(s,
+                "-----------------------------------------------------------------------\n");
+

You must hold the gpd_list_lock while traversing the gpd list.

+       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+
+               int i;
+
+               for (i = 0; i < genpd->state_count; i++) {

To be sure you have valid data, you should hold the genpd lock here.


At some point while testing with calling suspend from the power_off handler, the cpu would go to sleep with the lock held, hence using this seq-file would not work.

while I agree, I think it is not super likely that a domain/child/devices/states are added or removed at this point (the DT is parsed already), would using list_for_each_entry_safe be safe enough ?

+                       seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
+                                  genpd->name,
+                                  genpd->states[i].name,
+                                  genpd->states[i].power_on_latency_ns,
+                                  genpd->states[i].power_off_latency_ns,
+                                  genpd->states[i].power_off_latency_ns
+                                  + genpd->states[i].power_on_latency_ns);
+               }
+
+       }
+
+       seq_puts(s, "\n");
+
+       return 0;
+}
+
+static int pm_genpd_states_open(struct inode *inode, struct file *file)
+{
+       return single_open(file, pm_genpd_states_show, NULL);
+}
+
+static const struct file_operations pm_genpd_states_fops = {
+       .open = pm_genpd_states_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+};
+
+static int pm_genpd_timing_show(struct seq_file *s, void *data)
+{
+       struct generic_pm_domain *genpd;
+
+       seq_puts(s, "\n    Domain Devices, Timings in ns\n");
+       seq_puts(s,
+                "                       Stop/Start Save/Restore, Effective\n");
+       seq_puts(s,
+                "----------------------------------------------------  ---\n");
+

You must hold the gpd_list_lock while traversing the gpd list.


+       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+               struct pm_domain_data *pm_data;
+
+               seq_printf(s, "%-30s", genpd->name);
+

You must hold the genpd lock while traversing the device list.

+               list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
+                       struct gpd_timing_data *td = &to_gpd_data(pm_data)->td;
+
+                       if (!pm_data->dev->of_node)
+                               continue;
+
+                       seq_printf(s,
+                                  "\n    %-20s %-6lld/%-6lld %-6lld/%-6lld,%lld %s%s",
+                                  pm_data->dev->of_node->full_name,
+                                  td->stop_latency_ns, td->start_latency_ns,
+                                  td->save_state_latency_ns,
+                                  td->restore_state_latency_ns,

This needs a re-base as these values have been merged.

+                                  td->effective_constraint_ns,
+                                  td->cached_stop_ok ? "(cached stop) " : "",
+                                  td->constraint_changed ? "(changed)" : "");
+               }
+               seq_puts(s, "\n");
+       }
+       return 0;
+}
+
+static int pm_genpd_timing_open(struct inode *inode, struct file *file)
+{
+       return single_open(file, pm_genpd_timing_show, NULL);
+}
+
+static const struct file_operations pm_genpd_timing_fops = {
+       .open = pm_genpd_timing_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+};
+
  static int __init pm_genpd_debug_init(void)
  {
-       struct dentry *d;
+       struct dentry *d = NULL;

         pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);

-       if (!pm_genpd_debugfs_dir)
-               return -ENOMEM;
+       if (pm_genpd_debugfs_dir)

In case when CONFIG_DEBUG_FS is unset, this doesn't work very well, as
pm_genpd_debugfs_dir will contain an error code.

Since you are anyway make quite big change to the debugfs support for
genpd, perhaps you can try to fix that up as well!?

Ah yes ok, will do.


+               d = debugfs_create_file("summary", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_summary_fops);
+       if (d)
+               d = debugfs_create_file("states", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_states_fops);
+       if (d)
+               d = debugfs_create_file("timings", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_timing_fops);
+       if (d)
+               return 0;

-       d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
-                       pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
-       if (!d)
-               return -ENOMEM;
+       debugfs_remove_recursive(pm_genpd_debugfs_dir /*can be null*/);

-       return 0;
+       return -ENOMEM;
  }
  late_initcall(pm_genpd_debug_init);


Kind regards
Uffe


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux