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 3/7] devfreq: add clearing transitions stats in sysfs
To:     Chanwoo Choi <cw00.choi@samsung.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc:     Krzysztof Kozlowski <krzk@kernel.org>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>
From:   Kamil Konieczny <k.konieczny@samsung.com>
Message-ID: <0f8becc4-a63a-5ac9-99e4-ad43ab43e157@samsung.com>
Date:   Fri, 15 Nov 2019 15:35:00 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
        Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <22eda7a2-3c3d-fd3e-fe5f-169c17a50a0b@samsung.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20191113091336.5218-1-k.konieczny@samsung.com>
        <CGME20191113091352eucas1p2c30c8a73a8362aff872e3cd9312eb24b@eucas1p2.samsung.com>
        <20191113091336.5218-4-k.konieczny@samsung.com>
        <3a5096c3-03d2-fd5d-852a-6a19c5b262a5@samsung.com>
        <95813188-be4f-d3b3-459a-597be775f035@samsung.com>
        <22eda7a2-3c3d-fd3e-fe5f-169c17a50a0b@samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Chanwoo,

On 15.11.2019 03:47, Chanwoo Choi wrote:
> Hi Bartlomiej,
> 
> On 11/15/19 3:23 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Chanwoo,
>>
>> On 11/13/19 10:41 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> If user only want to use the transitions stats information
>>> from now, the user just read the sysfs twice and then
>>> can calculate them between the read data. It is enough
>>
>> IOW you are suggesting that user should invest his valuable time
>> into actually doing such calculations (or implementing some extra
>> script to do the data parsing and calculations automatically)
>> instead of doing simple write to special sysfs file?
> 
> Actually, I wouldn't like to add the new node for the debugging.
> If there are some requirement for the debugging, we better to
> add the debugfs for devfreq (devfreq has not yet developed
> the debugfs. I'll do that). If some node is necessary for the devfreq
> core/device operation, Surely, I'll agree.
> 
> On the initial version of devfreq, it contained the 'trans_stat'.
> In order to keep the compatibility, just could not move 'trans_stat'
> to under debugfs path.
> 
> If there are any requirement for resetting for transition stats,
> I prefer to use the following style without any extra debugging node.
> 
> 	For resetting for transition stats as following,
> 	echo 0 > /sys/class/devfreq/devfreq0/trans_stat

ok, I will change the stats reset to this.

>> Also similar patch for cpufreq has been applied not so long ago:
>>
>> commit ee7930ee27fe5240398cc302fa8eb4454725f188
>> Author: Markus Mayer <mmayer@broadcom.com>
>> Date:   Mon Nov 7 10:02:23 2016 -0800
>>
>>     cpufreq: stats: New sysfs attribute for clearing statistics
>>     
>>     Allow CPUfreq statistics to be cleared by writing anything to
>>     /sys/.../cpufreq/stats/reset.
>>     
>>     Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>     Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ...
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>> to get the existing sysfs information. 
>>>
>>> And I don't know the any other reason. So, I can't agree this patch.
>>> So, Not ack.
>>>
>>> Regards,
>>> Chanwoo Choi
>>>
>>>
>>> On 11/13/19 6:13 PM, Kamil Konieczny wrote:
>>>> Add new function trans_reset in sysfs for clearing transition
>>>> table and time in states devfreq statistics.> 
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@samsung.com>
>>>> ---
>>>>  drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index ac04b5baef70..0a88055d1362 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -1445,6 +1445,31 @@ static ssize_t trans_stat_show(struct device *dev,
>>>>  }
>>>>  static DEVICE_ATTR_RO(trans_stat);
>>>>  
>>>> +static void defvreq_stats_clear_table(struct devfreq *devfreq)
>>>> +{
>>>> +	unsigned int count = devfreq->profile->max_state;
>>>> +
>>>> +	spin_lock(&devfreq->stats_lock);
>>>> +	memset(devfreq->time_in_state, 0, count * sizeof(u64));
>>>> +	memset(devfreq->trans_table, 0, count * count * sizeof(int));
>>>> +	devfreq->last_stat_updated = get_jiffies_64();
>>>> +	devfreq->total_trans = 0;
>>>> +	spin_unlock(&devfreq->stats_lock);
>>>> +}
>>>> +
>>>> +static ssize_t trans_reset_store(struct device *dev,
>>>> +				 struct device_attribute *attr,
>>>> +				 const char *buf,
>>>> +				 size_t count)
>>>> +{
>>>> +	struct devfreq *devfreq = to_devfreq(dev);
>>>> +
>>>> +	defvreq_stats_clear_table(devfreq);
>>>> +
>>>> +	return count;
>>>> +}
>>>> +static DEVICE_ATTR_WO(trans_reset);
>>>> +
>>>>  static struct attribute *devfreq_attrs[] = {
>>>>  	&dev_attr_governor.attr,
>>>>  	&dev_attr_available_governors.attr,
>>>> @@ -1455,6 +1480,7 @@ static struct attribute *devfreq_attrs[] = {
>>>>  	&dev_attr_min_freq.attr,
>>>>  	&dev_attr_max_freq.attr,
>>>>  	&dev_attr_trans_stat.attr,
>>>> +	&dev_attr_trans_reset.attr,
>>>>  	NULL,
>>>>  };
>>>>  ATTRIBUTE_GROUPS(devfreq);

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland