[PATCH RFC 06/27] PM / Domains: add debugfs 'states' and 'timings' seq files
Marc Titinger
mtitinger at baylibre.com
Wed Dec 16 03:07:13 PST 2015
On 11/12/2015 12:46, Ulf Hansson wrote:
> On 17 November 2015 at 23:37, Lina Iyer <lina.iyer at linaro.org> wrote:
Hi Ulf,
thanks for the review, comments and questions below.
Best Regards,
Marc.
>> From: Marc Titinger <mtitinger at baylibre.com>
>>
>> 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 at 100 1060 /660 1580 /1940 ,0 (cached stop)
>> /cpus/cpu at 101 1060 /740 1520 /1600 ,0 (cached stop)
>> /cpus/cpu at 102 880 /620 1380 /1780 ,0 (cached stop)
>> /cpus/cpu at 103 1080 /640 1340 /1600 ,0 (cached stop)
>> a57_pd
>> /cpus/cpu at 0 1160 /740 3280 /1800 ,0 (cached stop)
>> /cpus/cpu at 1 780 /1400 1440 /2080 ,0 (cached stop)
>> /D1 600 /540 7140 /6420 ,2199460 (cached stop)
>>
>> Signed-off-by: Marc Titinger <mtitinger at baylibre.com>
>
> 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
>
More information about the linux-arm-kernel
mailing list