Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file
To:     Dmitry Osipenko <digetx@gmail.com>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     leonard.crestez@nxp.com, lukasz.luba@arm.com, a.swigon@samsung.com,
        m.szyprowski@samsung.com, enric.balletbo@collabora.com,
        hl@rock-chips.com, bjorn.andersson@linaro.org,
        jcrouse@codeaurora.org, chanwoo@kernel.org,
        myungjoo.ham@samsung.com, kyungmin.park@samsung.com
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <402b0752-d750-b57b-fbb2-a8de59dbaa96@samsung.com>
Date:   Thu, 9 Jan 2020 09:44:27 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <3381b9a0-1023-f11c-1348-44f9de0e0e17@gmail.com>
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20200107090519.3231-1-cw00.choi@samsung.com>
        <CGME20200107085812epcas1p4670ae2265573d887aa75cab36c04b1ea@epcas1p4.samsung.com>
        <20200107090519.3231-3-cw00.choi@samsung.com>
        <2c259936-f099-a239-9d6b-08f9b253644f@gmail.com>
        <92367824-d0ac-1be3-e432-292ea1796143@samsung.com>
        <e58888d7-9803-da5e-1054-690fe52284a1@samsung.com>
        <3381b9a0-1023-f11c-1348-44f9de0e0e17@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@samsung.com>
>>>>> ---
>>>>>  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