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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 22 Nov 2019 23:10:39 +0200
From:   Georgi Djakov <georgi.djakov@...aro.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-pm@...r.kernel.org, mingo@...hat.com,
        bjorn.andersson@...aro.org, vincent.guittot@...aro.org,
        daidavid1@...eaurora.org, okukatla@...eaurora.org,
        evgreen@...omium.org, mka@...omium.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] interconnect: Add basic tracepoints

Hi Steven,

On 22.11.19 19:07, Steven Rostedt wrote:
> On Fri, 22 Nov 2019 17:27:12 +0200
> Georgi Djakov <georgi.djakov@...aro.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@...aro.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@...dmis.org>
> 
> -- Steve