From: Chanwoo Choi <cw00.choi@samsung.com>
To: Nathan Chancellor <natechancellor@gmail.com>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
chanwoo@kernel.org, myungjoo.ham@samsung.com,
kyungmin.park@samsung.com, clang-built-linux@googlegroups.com
Subject: Re: [PATCH] PM / devfreq: Add debugfs support with devfreq_summary file
Date: Mon, 30 Dec 2019 14:13:39 +0900 [thread overview]
Message-ID: <22655ea9-d57d-f24e-3912-7e588c036ee3@samsung.com> (raw)
In-Reply-To: <20191230032655.GA40172@ubuntu-m2-xlarge-x86>
Hi Nathan,
On 12/30/19 12:26 PM, Nathan Chancellor wrote:
> Hi Chanwoo,
>
> On Thu, Dec 26, 2019 at 03:07:49PM +0900, 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
>>
>> [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 cur_freq min_freq max_freq
>> ------------------------------ ---------- ---------- --------------- ------------ ------------ ------------
>> 10c20000.memory-controller devfreq0 userspace 165000000 165000000 825000000
>> soc:bus_wcore devfreq1 simple_ondemand 400000000 84000000 400000000
>> soc:bus_noc devfreq2 devfreq1 passive 100000000 67000000 100000000
>> soc:bus_fsys_apb devfreq3 devfreq1 passive 200000000 100000000 200000000
>> soc:bus_fsys devfreq4 devfreq1 passive 200000000 100000000 200000000
>> soc:bus_fsys2 devfreq5 devfreq1 passive 150000000 75000000 150000000
>> soc:bus_mfc devfreq6 devfreq1 passive 333000000 96000000 333000000
>> soc:bus_gen devfreq7 devfreq1 passive 267000000 89000000 267000000
>> soc:bus_peri devfreq8 devfreq1 passive 67000000 67000000 67000000
>> soc:bus_g2d devfreq9 devfreq1 passive 333000000 84000000 333000000
>> soc:bus_g2d_acp devfreq10 devfreq1 passive 267000000 67000000 267000000
>> soc:bus_jpeg devfreq11 devfreq1 passive 300000000 75000000 300000000
>> soc:bus_jpeg_apb devfreq12 devfreq1 passive 167000000 84000000 167000000
>> soc:bus_disp1_fimd devfreq13 devfreq1 passive 200000000 120000000 200000000
>> soc:bus_disp1 devfreq14 devfreq1 passive 300000000 120000000 300000000
>> soc:bus_gscl_scaler devfreq15 devfreq1 passive 300000000 150000000 300000000
>> soc:bus_mscl devfreq16 devfreq1 passive 400000000 84000000 400000000
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>> drivers/devfreq/devfreq.c | 65 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 65 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index acd21345a070..d7177cc0a914 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -10,6 +10,7 @@
>> #include <linux/kernel.h>
>> #include <linux/kmod.h>
>> #include <linux/sched.h>
>> +#include <linux/debugfs.h>
>> #include <linux/errno.h>
>> #include <linux/err.h>
>> #include <linux/init.h>
>> @@ -33,6 +34,7 @@
>> #define HZ_PER_KHZ 1000
>>
>> static struct class *devfreq_class;
>> +static struct dentry *devfreq_debugfs;
>>
>> /*
>> * devfreq core provides delayed work based load monitoring helper
>> @@ -1670,6 +1672,62 @@ static struct attribute *devfreq_attrs[] = {
>> };
>> ATTRIBUTE_GROUPS(devfreq);
>>
>> +static int devfreq_summary_show(struct seq_file *s, void *data)
>> +{
>> + struct devfreq *devfreq;
>> + struct devfreq *parent_devfreq;
>> + unsigned long cur_freq, min_freq, max_freq;
>> +
>> + seq_printf(s, "%-30s %-10s %-10s %-15s %-12s %-12s %-12s\n",
>> + "dev name",
>> + "dev",
>> + "parent dev",
>> + "governor",
>> + "cur_freq",
>> + "min_freq",
>> + "max_freq");
>> + seq_printf(s, "%-30s %-10s %-10s %-15s %-12s %-12s %-12s\n",
>> + "------------------------------",
>> + "----------",
>> + "----------",
>> + "---------------",
>> + "------------",
>> + "------------",
>> + "------------");
>> +
>> + mutex_lock(&devfreq_list_lock);
>> +
>
> The 0day team has been doing builds with Clang and it pointed out that
> when CONFIG_DEVFREQ_GOV_PASSIVE is unset, parent_devfreq is always
> uninitialized. The else branch should probably be eliminated, moving the
> parent_devfreq NULL initialization above the if preprocessor directive.
>
> The full report is below.
Thanks for the report. I'll fix it as following:
struct devfreq *parent_devfreq = NULL;
>
>> + list_for_each_entry_reverse(devfreq, &devfreq_list, node) {
>> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> + if (!strncmp(devfreq->governor_name, DEVFREQ_GOV_PASSIVE,
>> + DEVFREQ_NAME_LEN)) {
>> + struct devfreq_passive_data *data = devfreq->data;
>> + parent_devfreq = data->parent;
>> + } else {
>> + parent_devfreq = NULL;
>> + }
>> +#endif
>> + mutex_lock(&devfreq->lock);
>> + cur_freq = devfreq->previous_freq,
>> + get_freq_range(devfreq, &min_freq, &max_freq);
>> + mutex_unlock(&devfreq->lock);
>> +
>> + seq_printf(s, "%-30s %-10s %-10s %-15s %-12ld %-12ld %-12ld\n",
>> + dev_name(devfreq->dev.parent),
>> + dev_name(&devfreq->dev),
>> + parent_devfreq ? dev_name(&parent_devfreq->dev) : "",
>> + devfreq->governor_name,
>> + cur_freq,
>> + min_freq,
>> + max_freq);
>> + }
>> +
>> + mutex_unlock(&devfreq_list_lock);
>> +
>> + return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>> +
>> static int __init devfreq_init(void)
>> {
>> devfreq_class = class_create(THIS_MODULE, "devfreq");
>> @@ -1686,6 +1744,13 @@ 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))
>> + pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>> +
>> + debugfs_create_file("devfreq_summary", 0444, devfreq_debugfs, NULL,
>> + &devfreq_summary_fops);
>> +
>> return 0;
>> }
>> subsys_initcall(devfreq_init);
>> --
>> 2.17.1
>>
>
> Cheers,
> Nathan
>
> On Thu, Dec 26, 2019 at 11:47:26PM +0800, kbuild test robot wrote:
>> CC: kbuild-all@lists.01.org
>> In-Reply-To: <20191226060749.13881-1-cw00.choi@samsung.com>
>> References: <20191226060749.13881-1-cw00.choi@samsung.com>
>> TO: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> Hi Chanwoo,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on linux/master]
>> [also build test WARNING on linus/master v5.5-rc3 next-20191220]
>> [if your patch is applied to the wrong git tree, please drop us a note to help
>> improve the system. BTW, we also suggest to use '--base' option to specify the
>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>>
>> url: https://protect2.fireeye.com/url?k=aa57311e-f7cb307e-aa56ba51-0cc47a31307c-5e67a2de952cf65a&u=https://github.com/0day-ci/linux/commits/Chanwoo-Choi/PM-devfreq-Add-debugfs-support-with-devfreq_summary-file/20191226-140227
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1522d9da40bdfe502c91163e6d769332897201fa
>> config: arm64-defconfig (attached as .config)
>> compiler: clang version 10.0.0 (git://gitmirror/llvm_project c5b4a2386b51a18daad7e42040c685c2e9708c47)
>> reproduce:
>> wget https://protect2.fireeye.com/url?k=e9857e19-b4197f79-e984f556-0cc47a31307c-e0c94321f498a093&u=https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # save the attached .config to linux build tree
>> make.cross ARCH=arm64
>>
>> If you fix the issue, kindly add following tag
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> All warnings (new ones prefixed by >>):
>>
>>>> drivers/devfreq/devfreq.c:1653:4: warning: variable 'parent_devfreq' is uninitialized when used here [-Wuninitialized]
>> parent_devfreq ? dev_name(&parent_devfreq->dev) : "",
>> ^~~~~~~~~~~~~~
>> drivers/devfreq/devfreq.c:1613:32: note: initialize the variable 'parent_devfreq' to silence this warning
>> struct devfreq *parent_devfreq;
>> ^
>> = NULL
>> 1 warning generated.
>>
>> vim +/parent_devfreq +1653 drivers/devfreq/devfreq.c
>>
>> 1609
>> 1610 static int devfreq_summary_show(struct seq_file *s, void *data)
>> 1611 {
>> 1612 struct devfreq *devfreq;
>> 1613 struct devfreq *parent_devfreq;
>> 1614 unsigned long cur_freq, min_freq, max_freq;
>> 1615
>> 1616 seq_printf(s, "%-30s %-10s %-10s %-15s %-12s %-12s %-12s\n",
>> 1617 "dev name",
>> 1618 "dev",
>> 1619 "parent dev",
>> 1620 "governor",
>> 1621 "cur_freq",
>> 1622 "min_freq",
>> 1623 "max_freq");
>> 1624 seq_printf(s, "%-30s %-10s %-10s %-15s %-12s %-12s %-12s\n",
>> 1625 "------------------------------",
>> 1626 "----------",
>> 1627 "----------",
>> 1628 "---------------",
>> 1629 "------------",
>> 1630 "------------",
>> 1631 "------------");
>> 1632
>> 1633 mutex_lock(&devfreq_list_lock);
>> 1634
>> 1635 list_for_each_entry_reverse(devfreq, &devfreq_list, node) {
>> 1636 #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> 1637 if (!strncmp(devfreq->governor_name, DEVFREQ_GOV_PASSIVE,
>> 1638 DEVFREQ_NAME_LEN)) {
>> 1639 struct devfreq_passive_data *data = devfreq->data;
>> 1640 parent_devfreq = data->parent;
>> 1641 } else {
>> 1642 parent_devfreq = NULL;
>> 1643 }
>> 1644 #endif
>> 1645 mutex_lock(&devfreq->lock);
>> 1646 cur_freq = devfreq->previous_freq,
>> 1647 get_freq_range(devfreq, &min_freq, &max_freq);
>> 1648 mutex_unlock(&devfreq->lock);
>> 1649
>> 1650 seq_printf(s, "%-30s %-10s %-10s %-15s %-12ld %-12ld %-12ld\n",
>> 1651 dev_name(devfreq->dev.parent),
>> 1652 dev_name(&devfreq->dev),
>>> 1653 parent_devfreq ? dev_name(&parent_devfreq->dev) : "",
>> 1654 devfreq->governor_name,
>> 1655 cur_freq,
>> 1656 min_freq,
>> 1657 max_freq);
>> 1658 }
>> 1659
>> 1660 mutex_unlock(&devfreq_list_lock);
>> 1661
>> 1662 return 0;
>> 1663 }
>> 1664 DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>> 1665
>>
>> ---
>> 0-DAY kernel test infrastructure Open Source Technology Center
>> https://protect2.fireeye.com/url?k=f6f1d925-ab6dd845-f6f0526a-0cc47a31307c-7e8a6c295935ff12&u=https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
prev parent reply other threads:[~2019-12-30 5:06 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20191226060101epcas1p11a225c00bb7ab2f6b7895b4cb00b9871@epcas1p1.samsung.com>
2019-12-26 6:07 ` [PATCH] PM / devfreq: Add debugfs support with devfreq_summary file Chanwoo Choi
2019-12-30 3:26 ` Nathan Chancellor
2019-12-30 5:13 ` Chanwoo Choi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=22655ea9-d57d-f24e-3912-7e588c036ee3@samsung.com \
--to=cw00.choi@samsung.com \
--cc=chanwoo@kernel.org \
--cc=clang-built-linux@googlegroups.com \
--cc=kyungmin.park@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
--cc=natechancellor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.