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

[Dmitry Osipenko]

08.01.2020 10:56, Chanwoo Choi ÐÐÑÐÑ:
> 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@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>>> ---
>>>  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.

You could write it this way:

	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
	err = PTR_ERR_OR_ZERO(devfreq_debugfs);
	if (err && err != -ENODEV) {
	...