Re: [PATCH v3 3/3] interconnect: Add basic tracepoints

From: Georgi Djakov <georgi.djakov@linaro.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-pm@vger.kernel.org, mingo@redhat.com,
	bjorn.andersson@linaro.org, vincent.guittot@linaro.org,
	daidavid1@codeaurora.org, okukatla@codeaurora.org,
	evgreen@chromium.org, mka@chromium.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] interconnect: Add basic tracepoints
Date: Fri, 22 Nov 2019 23:10:39 +0200	[thread overview]
Message-ID: <da40050f-dbc5-e95f-29bb-edaa6ae565dc@linaro.org> (raw)
In-Reply-To: <20191122120746.62027203@oasis.local.home>

Hi Steven,

On 22.11.19 19:07, Steven Rostedt wrote:
> On Fri, 22 Nov 2019 17:27:12 +0200
> Georgi Djakov <georgi.djakov@linaro.org> wrote:
>> index 28f2ab0824d5..725029ae7a2c 100644
>> --- a/drivers/interconnect/Makefile
>> +++ b/drivers/interconnect/Makefile
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>> +CFLAGS_core.o				:= -I$(src)
>>  icc-core-objs				:= core.o
>>  
>>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 1ddad8ef3cf0..b218a2609f6b 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -19,6 +19,9 @@
>>  #include <linux/of.h>
>>  #include <linux/overflow.h>
>>  
>> +#define CREATE_TRACE_POINTS
>> +#include "trace.h"
>> +
> 
> You may want to move this below the include of internal.h, as you don't
> want CREATE_TRACE_POINTS defined when including any other header, or it
> can cause issues if that header has some tracepoint header inside it.
> 
> It may not be the case now, but could cause for headaches in the
> future, if other headers get included in internal.h.
> 
>>  #include "internal.h"
>>  
>>  static DEFINE_IDR(icc_idr);
>> @@ -435,6 +438,8 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>>  
>>  		/* aggregate requests for this node */
>>  		aggregate_requests(node);
>> +
>> +		trace_icc_set_bw(path, node, i, avg_bw, peak_bw);
>>  	}
>>  
>>  	ret = apply_constraints(path);
>> @@ -453,6 +458,8 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>>  
>>  	mutex_unlock(&icc_lock);
>>  
>> +	trace_icc_set_bw_end(path, ret);
>> +
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(icc_set_bw);
>> diff --git a/drivers/interconnect/trace.h b/drivers/interconnect/trace.h
>> new file mode 100644
>> index 000000000000..d2421bf7b389
>> --- /dev/null
>> +++ b/drivers/interconnect/trace.h
>> @@ -0,0 +1,90 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Interconnect framework tracepoints
>> + * Copyright (c) 2019, Linaro Ltd.
>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
>> + */
>> +
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM interconnect
>> +
>> +#if !defined(_TRACE_INTERCONNECT_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_INTERCONNECT_H
>> +
>> +#include <linux/interconnect.h>
>> +#include <linux/tracepoint.h>
>> +
>> +#include "internal.h"
> 
> And you include it here too, perhaps it is best not to have it here,
> and then just have it before trace.h is called?

Sure! Thanks a lot for reviewing!

BR,
Georgi

> 
> The rest looks good. Besides the comments above:
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> -- Steve