Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Dmitry Osipenko]

08.01.2020 15:01, Chanwoo Choi ÐÐÑÐÑ:
> On 1/8/20 7:56 PM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 1/8/20 6:31 AM, Dmitry Osipenko wrote:
>>> Hello Chanwoo,
>>>
>>> 07.01.2020 12:05, Chanwoo Choi ÐÐÑÐÑ:
>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>> of all devfreq devices for the simple profiling as following:
>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>
>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> in Kconfig in order to save the transition history.
>>>>
>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>> - 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.
>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>> 		  load is used by governor whene deciding the new frequency.
>>>> 		  (unit: percentage)
>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>> [snip]
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/devfreq/Kconfig            |  13 +++
>>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>>  drivers/devfreq/governor.h         |   3 +
>>>>  drivers/devfreq/governor_passive.c |   2 +
>>>>  include/linux/devfreq.h            |   1 +
>>>>  5 files changed, 145 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index 0b1df12e0f21..84936eec0ef9 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>>  	  through sysfs entries. The passive governor recommends that
>>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>>  
>>>> +comment "DEVFREQ Debugging"
>>>> +
>>>> +config NR_DEVFREQ_TRANSITIONS
>>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>>> +	depends on DEBUG_FS
>>>> +	range 10 1000
>>>> +	default "100"
>>>> +	help
>>>> +	  Show the frequency transitions of all devfreq devices via
>>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>>> +	  profiling. It needs to decide the storage size to save transition
>>>> +	  history of all devfreq devices.
>>>> +
>>>>  comment "DEVFREQ Drivers"
>>>>  
>>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index c7f5e4e06420..7abaae06fa65 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>  }
>>>>  EXPORT_SYMBOL(devfreq_update_status);
>>>> +/**
>>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>>> + * @devfreq:	the devfreq instance
>>>> + * @old_freq:	the previous frequency before changing the frequency
>>>> + * @new_freq:	the new frequency after frequency is changed
>>>> + */
>>>> +struct devfreq_transitions {
>>>> +	struct devfreq *devfreq;
>>>> +	struct devfreq_freqs freqs;
>>>> +	unsigned long load;
>>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>>> +
>>>> +static spinlock_t devfreq_debugfs_lock;
>>>
>>> This could be:
>>>
>>> static DEFINE_SPINLOCK(devfreq_debugfs_lock);
>>>
>>> and then spin_lock_init() isn't needed.
>>
>> OK
>>
>>>
>>>
>>> Also, The "<linux/spinlock.h>" should be included directly by devfreq.c
>>
>> OK.
>>
>>>
>>>> +static int debugfs_transitions_index;
>>>> +
>>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>>> +			unsigned long old_freq, unsigned long new_freq,
>>>> +			unsigned long busy_time, unsigned long total_time)
>>>> +{
>>>> +	unsigned long load;
>>>> +	int i;
>>>> +
>>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>>> +		return;
>>>> +
>>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>>> +
>>>> +	i = debugfs_transitions_index;
>>>> +
>>>> +	/*
>>>> +	 * Calculate the load and if load is larger than 100,
>>>> +	 * initialize to 100 because the unit of load is percentage.
>>>> +	 */
>>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>>> +	if (load > 100)
>>>> +		load = 100;
>>>> +
>>>> +	debugfs_transitions[i].devfreq = devfreq;
>>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>>> +	debugfs_transitions[i].load = load;
>>>> +
>>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> +		i = 0;
>>>> +	debugfs_transitions_index = i;
>>>> +
>>>> +	spin_unlock(&devfreq_debugfs_lock);
>>>> +}
>>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>>
>>> What about EXPORT_SYMBOL_GPL()?
>>
>> I'll remove it.
> 
> Ah. It is needed to support module build.
> it is used by passive governor.

My point was about the "GPL" part. The EXPORT_SYMBOL_GPL() prohibits
closed source drivers to use the exported API.