Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure

[Alexander Shishkin <alexander.shishkin@linux.intel.com>]

kbuild test robot <lkp@intel.com> writes:

> Hi Alexander,
>
> [auto build test ERROR on tip/perf/core]
> [also build test ERROR on v4.10-rc5 next-20170125]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Alexander-Shishkin/perf-Updates-for-address-filters/20170126-175256
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
>
> All errors (new ones prefixed by >>):
>
>    drivers/hwtracing/coresight/coresight-etm-perf.c: In function 'etm_addr_filters_sync':
>>> drivers/hwtracing/coresight/coresight-etm-perf.c:426:13: error: 'struct perf_addr_filter' has no member named 'range'
>       if (filter->range == 1) {
>                 ^~

Right. Now it should also compile.

>From f50111d892a63721bef66eae39feadcd0194b163 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Mon, 23 Jan 2017 17:29:20 +0200
Subject: [PATCH v1] perf, pt, coresight: Clean up address filter structure

This is a cosmetic patch that deals with the address filter structure's
ambiguous fields 'filter' and 'range'. The former stands to mean that the
filter's *action* should be to filter the traces to its address range if
it's set or stop tracing if it's unset. This is confusing and hard on the
eyes, so this patch replaces it with 'action' enum. The 'range' field is
completely redundant (meaning that the filter is an address range as
opposed to a single address trigger), as we can use zero size to mean the
same thing.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 arch/x86/events/intel/pt.c                       |  7 +++--
 drivers/hwtracing/coresight/coresight-etm-perf.c | 35 ++++++------------------
 include/linux/perf_event.h                       | 14 ++++++----
 kernel/events/core.c                             | 12 ++++----
 4 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 5dba5468f8..5ac0911ac6 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1141,7 +1141,7 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
 
 	list_for_each_entry(filter, filters, entry) {
 		/* PT doesn't support single address triggers */
-		if (!filter->range || !filter->size)
+		if (!filter->size)
 			return -EOPNOTSUPP;
 
 		if (!filter->inode) {
@@ -1181,7 +1181,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
 
 		filters->filter[range].msr_a  = msr_a;
 		filters->filter[range].msr_b  = msr_b;
-		filters->filter[range].config = filter->filter ? 1 : 2;
+		if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
+			filters->filter[range].config = 1;
+		else
+			filters->filter[range].config = 2;
 		range++;
 	}
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 1774196902..f14bc5c73f 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -392,35 +392,18 @@ static int etm_addr_filters_validate(struct list_head *filters)
 		if (++index > ETM_ADDR_CMP_MAX)
 			return -EOPNOTSUPP;
 
+		/* filter::size==0 means single address trigger */
+		if (filter->size)
+			range = true;
+		else
+			address = true;
+
 		/*
-		 * As taken from the struct perf_addr_filter documentation:
-		 *	@range:	1: range, 0: address
-		 *
 		 * At this time we don't allow range and start/stop filtering
 		 * to cohabitate, they have to be mutually exclusive.
 		 */
-		if ((filter->range == 1) && address)
+		if (range && address)
 			return -EOPNOTSUPP;
-
-		if ((filter->range == 0) && range)
-			return -EOPNOTSUPP;
-
-		/*
-		 * For range filtering, the second address in the address
-		 * range comparator needs to be higher than the first.
-		 * Invalid otherwise.
-		 */
-		if (filter->range && filter->size == 0)
-			return -EINVAL;
-
-		/*
-		 * Everything checks out with this filter, record what we've
-		 * received before moving on to the next one.
-		 */
-		if (filter->range)
-			range = true;
-		else
-			address = true;
 	}
 
 	return 0;
@@ -440,12 +423,12 @@ static void etm_addr_filters_sync(struct perf_event *event)
 		stop = start + filter->size;
 		etm_filter = &filters->etm_filter[i];
 
-		if (filter->range == 1) {
+		if (filter->size) {
 			etm_filter->start_addr = start;
 			etm_filter->stop_addr = stop;
 			etm_filter->type = ETM_ADDR_TYPE_RANGE;
 		} else {
-			if (filter->filter == 1) {
+			if (filter->action == PERF_ADDR_FILTER_ACTION_START) {
 				etm_filter->start_addr = start;
 				etm_filter->type = ETM_ADDR_TYPE_START;
 			} else {
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 214c81588e..fcb37c81ca 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -457,14 +457,19 @@ struct pmu {
 	int (*filter_match)		(struct perf_event *event); /* optional */
 };
 
+enum perf_addr_filter_action_t {
+	PERF_ADDR_FILTER_ACTION_STOP = 0,
+	PERF_ADDR_FILTER_ACTION_START,
+	PERF_ADDR_FILTER_ACTION_FILTER = PERF_ADDR_FILTER_ACTION_START,
+};
+
 /**
  * struct perf_addr_filter - address range filter definition
  * @entry:	event's filter list linkage
  * @inode:	object file's inode for file-based filters
  * @offset:	filter range offset
- * @size:	filter range size
- * @range:	1: range, 0: address
- * @filter:	1: filter/start, 0: stop
+ * @size:	filter range size (size==0 means single address trigger)
+ * @action:	filter/start/stop
  *
  * This is a hardware-agnostic filter configuration as specified by the user.
  */
@@ -473,8 +478,7 @@ struct perf_addr_filter {
 	struct inode		*inode;
 	unsigned long		offset;
 	unsigned long		size;
-	unsigned int		range	: 1,
-				filter	: 1;
+	enum perf_addr_filter_action_t	action;
 };
 
 /**
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 458ff624d6..b422b5feee 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8141,7 +8141,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
  *  * for kernel addresses: <start address>[/<size>]
  *  * for object files:     <start address>[/<size>]@</path/to/object/file>
  *
- * if <size> is not specified, the range is treated as a single address.
+ * if <size> is not specified or is zero, the range is treated as a single
+ * address.
  */
 enum {
 	IF_ACT_NONE = -1,
@@ -8207,7 +8208,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 		switch (token) {
 		case IF_ACT_FILTER:
 		case IF_ACT_START:
-			filter->filter = 1;
+			filter->action = PERF_ADDR_FILTER_ACTION_FILTER;
 
 		case IF_ACT_STOP:
 			if (state != IF_STATE_ACTION)
@@ -8225,15 +8226,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			if (state != IF_STATE_SOURCE)
 				goto fail;
 
-			if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
-				filter->range = 1;
-
 			*args[0].to = 0;
 			ret = kstrtoul(args[0].from, 0, &filter->offset);
 			if (ret)
 				goto fail;
 
-			if (filter->range) {
+			if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
 				*args[1].to = 0;
 				ret = kstrtoul(args[1].from, 0, &filter->size);
 				if (ret)
@@ -8241,7 +8239,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 			}
 
 			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
-				int fpos = filter->range ? 2 : 1;
+				int fpos = token == IF_SRC_FILE ? 2 : 1;
 
 				filename = match_strdup(&args[fpos]);
 				if (!filename) {
-- 
2.11.0