On 19. 9. 25. 오전 4:37, Matthias Kaehlcke wrote:
> On Fri, Sep 20, 2019 at 10:15:57AM +0900, Chanwoo Choi wrote:
>> Hi,
>
> sorry for the delayed response, you message got buried in my
> mailbox.
>
>> On 19. 9. 20. 오전 2:44, Matthias Kaehlcke wrote:
>>> Add a tracepoint for frequency changes of devfreq devices and
>>> use it.
>>>
>>> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>>> ---
>>> (sending v2 without much delay wrt v1, since the change in devfreq
>>> probably isn't controversial, and I'll be offline a few days)
>>>
>>> Changes in v2:
>>> - included trace_devfreq_frequency_enabled() in the condition
>>> to avoid unnecessary evaluation when the trace point is
>>> disabled
>>> ---
>>> drivers/devfreq/devfreq.c | 3 +++
>>> include/trace/events/devfreq.h | 18 ++++++++++++++++++
>>> 2 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index ab22bf8a12d6..e9f04dcafb01 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq *devfreq,
>>> unsigned long new_freq,
>>>
>>> devfreq->previous_freq = new_freq;
>>>
>>> + if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
>>> + trace_devfreq_frequency(devfreq, new_freq);
>>
>> You can change as following without 'new_freq' variable
>> because devfreq->previous_freq is the new frequency.
>> trace_devfreq_frequency(devfreq);
>
> In general that sounds good.
>
> devfreq essentially uses df->previous_freq as df->cur_freq, I think
> most code using it would be clearer if we renamed it accordingly.
> I'll send a separate patch for this.
Actually, according to reference time of the 'df->previous_freq',
'previous_freq' is proper or 'cur_freq is proper.
But, In the comment of 'struct devfreq',
it means the configured time as following:
* @previous_freq: previously configured frequency value.
I think that it it not big problem to keep the name.
>
>>> +
>>> if (devfreq->suspend_freq)
>>> devfreq->resume_freq = cur_freq;
>>>
>>> diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
>>> index cf5b8772175d..a62d32fe3c33 100644
>>> --- a/include/trace/events/devfreq.h
>>> +++ b/include/trace/events/devfreq.h
>>> @@ -8,6 +8,24 @@
>>> #include <linux/devfreq.h>
>>> #include <linux/tracepoint.h>
>>>
>>> +TRACE_EVENT(devfreq_frequency,
>>> + TP_PROTO(struct devfreq *devfreq, unsigned long freq),
>>
>> 'unsigned long freq' parameter is not necessary.
>>
>>> +
>>> + TP_ARGS(devfreq, freq),
>>> +
>>> + TP_STRUCT__entry(
>>> + __string(dev_name, dev_name(&devfreq->dev))
>>> + __field(unsigned long, freq)
>>> + ),
>>> +
>>> + TP_fast_assign(
>>> + __assign_str(dev_name, dev_name(&devfreq->dev));
>>> + __entry->freq = freq;
>>
>> Initialize the new frequency with 'devfreq->previous_freq' as following:
>>
>> __entry->freq = devfreq->previous_freq;
>>
>>> + ),
>>> +
>>> + TP_printk("dev_name=%s freq=%lu", __get_str(dev_name), __entry->freq)
>>> +);
>>> +
>>> TRACE_EVENT(devfreq_monitor,
>>> TP_PROTO(struct devfreq *devfreq),
>>>
>>>
>>
>>
>> --
>> Best Regards,
>> Chanwoo Choi
>> Samsung Electronics
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
