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

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

On 1/8/20 11:14 PM, Dmitry Osipenko wrote:
> 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@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.
> 
> You could write it this way:
> 
> 	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
> 	err = PTR_ERR_OR_ZERO(devfreq_debugfs);
> 	if (err && err != -ENODEV) {

It is same result between your suggestion and following statement'
without any separate local variable. 

	if (IS_ERR(devfreq_debugfs) && PTR_ERR(devfreq_debugfs) != -ENODEV)


> 	...
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics