Re: [PATCH 1/2] PM / devfreq: Add debugfs support with devfreq_summary file

[Chanwoo Choi <cw00.choi@samsung.com>]

On 1/8/20 6:15 AM, Bjorn Andersson wrote:
> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
> 
>> Add debugfs interface to provide debugging information of devfreq device.
>> It contains 'devfreq_summary' entry to show the summary of registered
>> devfreq devices as following and the additional debugfs file will be added.
>> - /sys/kernel/debug/devfreq/devfreq_summary
>>
>> [Detailed description of each field of 'devfreq_summary' debugfs file]
>> - dev_name	: Device name of h/w.
>> - dev		: Device name made by devfreq core.
>> - parent_dev	: If devfreq device uses the passive governor,
>> 		  show parent devfreq device name.
>> - governor	: Devfreq governor.
>> - polling_ms	: If devfreq device uses the simple_ondemand governor,
>> 		  polling_ms is necessary for the period. (unit: millisecond)
>> - cur_freq_hz	: Current Frequency (unit: hz)
>> - old_freq_hz	: Frequency before changing. (unit: hz)
>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>
>> [For example on Exynos5422-based Odroid-XU3 board]
>> - In order to show the multiple governors on devfreq_summay result,
>> change the governor of devfreq0 from simple_ondemand to userspace.
>>
>> $ cat /sys/kernel/debug/devfreq/devfreq_summary
>> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
>> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
>> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
>> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
>> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
>> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
>> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
>> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
>> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
>> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
>> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
>> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
>> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
>> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
>> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
>> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
>> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
>> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
>> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000
> 
> This looks quite useful.
> 
>>
>> Reported-by: kbuild test robot <lkp@intel.com>
> 
> May I ask how the build test robot came up with this idea?

I'm missing the detailed what are the reported by kbuild test robot.
It reported the possible build error. I'll add the following description
on next version:
	[lkp: Reported the build error]

> 
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 80 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 80 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> [..]
>> +/**
>> + * devfreq_summary_show() - Show the summary of the registered devfreq devices
>> + *				via 'devfreq_summary' debugfs file.
> 
> Please make this proper kerneldoc, i.e:
> 
>  * func() - short description
>  * @s:
>  * @data:
>  * 
>  * Long description
>  * 
>  * Return: foo on bar

OK.

> 
> [..]
>> @@ -1733,6 +1803,16 @@ static int __init devfreq_init(void)
>>  	}
>>  	devfreq_class->dev_groups = devfreq_groups;
>>  
>> +	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
>> +	if (PTR_ERR(devfreq_debugfs) != -ENODEV && IS_ERR(devfreq_debugfs)) {
> 
> Don't PTR_ERR() before IS_ERR().

Before checking IS_ERR(), have to check the PTR_ERR(devfreq_debugfs)
is -ENODEV or not because debugfs_create_dir return the '-ENODEV'
if DEBUG_FS is disabled. If return the -ENODEV, it is not error.

> 
>> +		devfreq_debugfs = NULL;
> 
> I don't think you need this, given that debugfs_create_file() will fail
> gracefully when passed and IS_ERR()

Right. Jut on patch2 checks the 'devfreq_debugfs' is NULL or not
in order to catch the DEBUG_FS is well working for devfreq.
Anyway, it would be better to add it to patch2 because devfreq_debugfs
is not used when failed to create debugfs dir.

> 
>> +		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
> 
> Afaict debugfs_create_() won't fail without printing a message already.

It just print the error message when DEBUG_FS is enabled
and debugfs_create_dir() returns the error.

> 
>> +	} else {
>> +		debugfs_create_file("devfreq_summary", 0444,
>> +				devfreq_debugfs, NULL,
>> +				&devfreq_summary_fops);
>> +	}
>> +
>>  	return 0;
>>  }
>>  subsys_initcall(devfreq_init);
> 
> Regards,
> Bjorn
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics