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

From: Nathan Chancellor
Date: Sun Dec 29 2019 - 22:27:24 EST

Next message: Jassi Brar: "Re: [PATCH] soc: mediatek: cmdq: avoid racing condition with mutex"
Previous message: Chris Chiu: "Re: [PATCH v2] ALSA: hda/realtek - Enable the bass speaker of ASUS UX431FLC"
In reply to: Chanwoo Choi: "[PATCH] PM / devfreq: Add debugfs support with devfreq_summary file"
Next in thread: Chanwoo Choi: "Re: [PATCH] PM / devfreq: Add debugfs support with devfreq_summary file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
> ---
> 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.

> + 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@xxxxxxxxxxxx
> In-Reply-To: <20191226060749.13881-1-cw00.choi@xxxxxxxxxxx>
> References: <20191226060749.13881-1-cw00.choi@xxxxxxxxxxx>
> TO: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>
> 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://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://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@xxxxxxxxx>
>
> 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://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx Intel Corporation
>

Next message: Jassi Brar: "Re: [PATCH] soc: mediatek: cmdq: avoid racing condition with mutex"
Previous message: Chris Chiu: "Re: [PATCH v2] ALSA: hda/realtek - Enable the bass speaker of ASUS UX431FLC"
In reply to: Chanwoo Choi: "[PATCH] PM / devfreq: Add debugfs support with devfreq_summary file"
Next in thread: Chanwoo Choi: "Re: [PATCH] PM / devfreq: Add debugfs support with devfreq_summary file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]