Re: [PATCH v2] devfreq: Add tracepoint for frequency changes

[Matthias Kaehlcke]

On Wed, Sep 25, 2019 at 10:56:15AM +0900, Chanwoo Choi wrote:
> 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 <mka@xxxxxxxxxxxx>
> >>> ---
> >>> (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.

sure, I wouldn't expect the comment of a variable/field called 'previous_freq'
say that it's the current frequency.

> I think that it it not big problem to keep the name.

It's indeed not a big problem, because the code works either way, something
like 'cur_freq' would just be less confusing.

These are the functions that use 'previous_freq' and how they use it:

devfreq_set_target
devfreq_monitor_suspend
cur_freq_show
target_freq_show
trans_stat_show
devfreq_passive_notifier_call
devfreq_userspace_func
  cur freq

devfreq_update_status
  prev freq

More than 85% use the variable as current frequency, which seems like a
good argument to give it the proper name, instead of having folks wonder
why the previous frequency is used.