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

From: Chanwoo Choi
Date: Wed Jan 08 2020 - 19:37:31 EST

On 1/8/20 11:23 PM, Dmitry Osipenko wrote:
> 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.

I'm sorry of my misunderstanding.
OK. I'll use EXPORT_SYMBOL_GPL()

--
Best Regards,
Chanwoo Choi
Samsung Electronics