| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 8 Jan 2020 16:56:32 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
leonard.crestez@....com, lukasz.luba@....com, a.swigon@...sung.com,
m.szyprowski@...sung.com, enric.balletbo@...labora.com,
hl@...k-chips.com, digetx@...il.com, jcrouse@...eaurora.org,
chanwoo@...nel.org, myungjoo.ham@...sung.com,
kyungmin.park@...sung.com
Subject: Re: [PATCH 1/2] PM / devfreq: Add debugfs support with
devfreq_summary file
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@...el.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@...sung.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
Powered by blists - more mailing lists