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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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?

> 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

[..]
> @@ -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().

> +		devfreq_debugfs = NULL;

I don't think you need this, given that debugfs_create_file() will fail
gracefully when passed and IS_ERR()

> +		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);

Afaict debugfs_create_() won't fail without printing a message already.

> +	} else {
> +		debugfs_create_file("devfreq_summary", 0444,
> +				devfreq_debugfs, NULL,
> +				&devfreq_summary_fops);
> +	}
> +
>  	return 0;
>  }
>  subsys_initcall(devfreq_init);

Regards,
Bjorn