[PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Mathieu Poirier]

This set adds the capability to communiate event specific configuration
to the PMU kernel driver using an ioctl().  The functionatlity is made
generic enough for anyone to use but is targeted at the identification
of CoreSight sinks when operating in CPU-wide trace scenarios.

Applies cleanly on v4.18-rc5.

Thanks,
Mathieu

---
Changes for V3:
. Return an error for CPU-wide scenarios while the feature is being
  implemented (Kim)
. Proper initialisation for event::hw::drv_config::lock (Kim)

Changes for V2:
. Fixed s390 problem reported by buildbot.
. Removed uneeded check in perf_event_process_drv_config() (Jiri)
. Reordered data copy in perf_event_set_drv_config() (Jiri)
. Went from 2 to 1 step driver configuration process (Alex)
. Moved structure name "perf_drv_config" to "pmu_drv_config".

V1: https://lkml.org/lkml/2018/7/2/1008

Mathieu Poirier (7):
  perf: Introduce ioctl to communicate driver configuration to kernel
  perf/core: Use ioctl to communicate driver configuration to kernel
  perf/aux: Make perf_event accessible to setup_aux()
  coresight: Use PMU driver configuration for sink selection
  perf tools: Use ioctl to communicate driver configuration to kernel
  perf tools: Make perf_evsel accessible to PMU driver configuration
    code
  perf tools: Use ioctl function to send sink configuration to kernel

 arch/s390/kernel/perf_cpum_sf.c                  |   6 +-
 arch/x86/events/intel/bts.c                      |   4 +-
 arch/x86/events/intel/pt.c                       |   5 +-
 drivers/hwtracing/coresight/coresight-etm-perf.c | 140 ++++++++++++++++++++---
 drivers/hwtracing/coresight/coresight-etm-perf.h |   4 +
 drivers/perf/arm_spe_pmu.c                       |   6 +-
 include/linux/perf_event.h                       |  47 +++++++-
 include/uapi/linux/perf_event.h                  |   1 +
 kernel/events/core.c                             |  78 +++++++++++++
 kernel/events/ring_buffer.c                      |   2 +-
 tools/include/uapi/linux/perf_event.h            |   1 +
 tools/perf/arch/arm/util/cs-etm.c                |  60 +++-------
 tools/perf/arch/arm/util/cs-etm.h                |   3 +-
 tools/perf/util/drv_configs.c                    |  30 ++---
 tools/perf/util/evsel.c                          |   7 ++
 tools/perf/util/evsel.h                          |   1 +
 tools/perf/util/pmu.h                            |   3 +-
 17 files changed, 305 insertions(+), 93 deletions(-)

-- 
2.7.4

[PATCH v3 1/7] perf: Introduce ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

Adding a new IOCTL command to communicate PMU specific configuration to
PMU kernel drivers.  This can be anything a PMU might need for
configuration that doesn't fit in the perf_event_attr structure, such
as the CoreSight sink to use for a session.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/uapi/linux/perf_event.h       | 1 +
 tools/include/uapi/linux/perf_event.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index b8e288a1f740..b5b3241877df 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -460,6 +460,7 @@ struct perf_event_query_bpf {
 #define PERF_EVENT_IOC_PAUSE_OUTPUT		_IOW('$', 9, __u32)
 #define PERF_EVENT_IOC_QUERY_BPF		_IOWR('$', 10, struct perf_event_query_bpf *)
 #define PERF_EVENT_IOC_MODIFY_ATTRIBUTES	_IOW('$', 11, struct perf_event_attr *)
+#define PERF_EVENT_IOC_SET_DRV_CONFIG		_IOW('$', 12, char *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index b8e288a1f740..b5b3241877df 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -460,6 +460,7 @@ struct perf_event_query_bpf {
 #define PERF_EVENT_IOC_PAUSE_OUTPUT		_IOW('$', 9, __u32)
 #define PERF_EVENT_IOC_QUERY_BPF		_IOWR('$', 10, struct perf_event_query_bpf *)
 #define PERF_EVENT_IOC_MODIFY_ATTRIBUTES	_IOW('$', 11, struct perf_event_attr *)
+#define PERF_EVENT_IOC_SET_DRV_CONFIG		_IOW('$', 12, char *)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
-- 
2.7.4

[PATCH v3 2/7] perf/core: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

This patch adds the mechanic needed for user space to send PMU specific
configuration to the kernel driver using an ioctl() command.  That way
events can keep track of options that don't fit in the perf_event_attr
structure like the selection of a CoreSight sink to use for the session.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/linux/perf_event.h | 45 ++++++++++++++++++++++++++
 kernel/events/core.c       | 78 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1fa12887ec02..7064b513ca2b 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -114,6 +114,14 @@ struct hw_perf_event_extra {
 	int		idx;	/* index in shared_regs->regs[] */
 };
 
+/*
+ * PMU driver configuration
+ */
+struct pmu_drv_config {
+	void		*config;
+	raw_spinlock_t	lock;
+};
+
 /**
  * struct hw_perf_event - performance event hardware details:
  */
@@ -178,6 +186,9 @@ struct hw_perf_event {
 	/* Last sync'ed generation of filters */
 	unsigned long			addr_filters_gen;
 
+	/* PMU driver configuration */
+	struct pmu_drv_config		drv_config;
+
 /*
  * hw_perf_event::state flags; used to track the PERF_EF_* state.
  */
@@ -447,6 +458,23 @@ struct pmu {
 	 * Filter events for PMU-specific reasons.
 	 */
 	int (*filter_match)		(struct perf_event *event); /* optional */
+
+	/*
+	 * Valiate complex PMU configuration that don't fit in the
+	 * perf_event_attr struct.  Returns a PMU specific pointer or an error
+	 * value < 0.
+	 *
+	 * As with addr_filters_validate(), runs in the context of the ioctl()
+	 * process and is not serialized with the rest of the PMU callbacks.
+	 */
+	void *(*drv_config_validate)	(struct perf_event *event,
+					 char *config_str);
+
+	/*
+	 * Release PMU specific configuration acquired by
+	 * drv_config_validate()
+	 */
+	void (*drv_config_free)		(void *drv_data);
 };
 
 enum perf_addr_filter_action_t {
@@ -1234,6 +1262,12 @@ static inline bool has_addr_filter(struct perf_event *event)
 	return event->pmu->nr_addr_filters;
 }
 
+static inline bool has_drv_config(struct perf_event *event)
+{
+	return event->pmu->drv_config_validate &&
+	       event->pmu->drv_config_free;
+}
+
 /*
  * An inherited event uses parent's filters
  */
@@ -1248,6 +1282,17 @@ perf_event_addr_filters(struct perf_event *event)
 	return ifh;
 }
 
+static inline struct pmu_drv_config *
+perf_event_get_drv_config(struct perf_event *event)
+{
+	struct pmu_drv_config *cfg = &event->hw.drv_config;
+
+	if (event->parent)
+		cfg = &event->parent->hw.drv_config;
+
+	return cfg;
+}
+
 extern void perf_event_addr_filters_sync(struct perf_event *event);
 
 extern int perf_output_begin(struct perf_output_handle *handle,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index a56f10b1e13b..4892246c3af2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4410,6 +4410,7 @@ static bool exclusive_event_installable(struct perf_event *event,
 
 static void perf_addr_filters_splice(struct perf_event *event,
 				       struct list_head *head);
+static void perf_drv_config_replace(struct perf_event *event, void *drv_data);
 
 static void _free_event(struct perf_event *event)
 {
@@ -4440,6 +4441,7 @@ static void _free_event(struct perf_event *event)
 	perf_event_free_bpf_prog(event);
 	perf_addr_filters_splice(event, NULL);
 	kfree(event->addr_filters_offs);
+	perf_drv_config_replace(event, NULL);
 
 	if (event->destroy)
 		event->destroy(event);
@@ -5002,6 +5004,8 @@ static inline int perf_fget_light(int fd, struct fd *p)
 static int perf_event_set_output(struct perf_event *event,
 				 struct perf_event *output_event);
 static int perf_event_set_filter(struct perf_event *event, void __user *arg);
+static int perf_event_set_drv_config(struct perf_event *event,
+				     void __user *arg);
 static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd);
 static int perf_copy_attr(struct perf_event_attr __user *uattr,
 			  struct perf_event_attr *attr);
@@ -5088,6 +5092,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
 
 		return perf_event_modify_attr(event,  &new_attr);
 	}
+
+	case PERF_EVENT_IOC_SET_DRV_CONFIG:
+		return perf_event_set_drv_config(event, (void __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
@@ -9090,6 +9098,75 @@ static int perf_event_set_filter(struct perf_event *event, void __user *arg)
 	return ret;
 }
 
+static void perf_drv_config_replace(struct perf_event *event, void *drv_data)
+{
+	unsigned long flags;
+	void *old_drv_data;
+	struct pmu_drv_config *drv_config = &event->hw.drv_config;
+
+	if (!has_drv_config(event))
+		return;
+
+	/* Children take their configuration from their parent */
+	if (event->parent)
+		return;
+
+	/* Make sure the PMU doesn't get a handle on the data */
+	raw_spin_lock_irqsave(&drv_config->lock, flags);
+
+	old_drv_data = drv_config->config;
+	drv_config->config = drv_data;
+
+	raw_spin_unlock_irqrestore(&drv_config->lock, flags);
+
+	/* Free PMU private data allocated by pmu::drv_config_validate() */
+	event->pmu->drv_config_free(old_drv_data);
+}
+
+static int
+perf_event_process_drv_config(struct perf_event *event, char *config_str)
+{
+	int ret = -EINVAL;
+	void *drv_data;
+
+	/* Make sure ctx.mutex is held */
+	lockdep_assert_held(&event->ctx->mutex);
+
+	/* Children take their configuration from their parent */
+	if (WARN_ON_ONCE(event->parent))
+		goto out;
+
+	drv_data = event->pmu->drv_config_validate(event, config_str);
+	if (IS_ERR(drv_data)) {
+		ret = PTR_ERR(drv_data);
+		goto out;
+	}
+
+	perf_drv_config_replace(event, drv_data);
+
+	ret = 0;
+out:
+	return ret;
+}
+
+static int perf_event_set_drv_config(struct perf_event *event, void __user *arg)
+{
+	int ret = -EINVAL;
+	char *config_str;
+
+	if (!has_drv_config(event))
+		return ret;
+
+	config_str = strndup_user(arg, PAGE_SIZE);
+	if (IS_ERR(config_str))
+		return PTR_ERR(config_str);
+
+	ret = perf_event_process_drv_config(event, config_str);
+
+	kfree(config_str);
+	return ret;
+}
+
 /*
  * hrtimer based swevent callback
  */
@@ -10019,6 +10096,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	if (attr->freq && attr->sample_freq)
 		hwc->sample_period = 1;
 	hwc->last_period = hwc->sample_period;
+	raw_spin_lock_init(&hwc->drv_config.lock);
 
 	local64_set(&hwc->period_left, hwc->sample_period);
 
-- 
2.7.4

[PATCH v3 3/7] perf/aux: Make perf_event accessible to setup_aux()

[Mathieu Poirier]

When pmu::setup_aux() is called the coresight PMU needs to know which
sink to use for the session by looking up the information in the
drv_config field of the hw_perf_event structure.

As such simply replace the cpu information by the complete perf_event
structure and change all affected customers.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 arch/s390/kernel/perf_cpum_sf.c                  | 6 +++---
 arch/x86/events/intel/bts.c                      | 4 +++-
 arch/x86/events/intel/pt.c                       | 5 +++--
 drivers/hwtracing/coresight/coresight-etm-perf.c | 6 +++---
 drivers/perf/arm_spe_pmu.c                       | 6 +++---
 include/linux/perf_event.h                       | 2 +-
 kernel/events/ring_buffer.c                      | 2 +-
 7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index 0292d68e7dde..2d55fc2bb185 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -1589,7 +1589,7 @@ static void aux_buffer_free(void *data)
 
 /*
  * aux_buffer_setup() - Setup AUX buffer for diagnostic mode sampling
- * @cpu:	On which to allocate, -1 means current
+ * @event:	Event the buffer is setup for, event->cpu == -1 means current
  * @pages:	Array of pointers to buffer pages passed from perf core
  * @nr_pages:	Total pages
  * @snapshot:	Flag for snapshot mode
@@ -1601,8 +1601,8 @@ static void aux_buffer_free(void *data)
  *
  * Return the private AUX buffer structure if success or NULL if fails.
  */
-static void *aux_buffer_setup(int cpu, void **pages, int nr_pages,
-			      bool snapshot)
+static void *aux_buffer_setup(struct perf_event *event, void **pages,
+			      int nr_pages, bool snapshot)
 {
 	struct sf_buffer *sfb;
 	struct aux_buffer *aux;
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 24ffa1e88cf9..7139f6bf27ad 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -77,10 +77,12 @@ static size_t buf_size(struct page *page)
 }
 
 static void *
-bts_buffer_setup_aux(int cpu, void **pages, int nr_pages, bool overwrite)
+bts_buffer_setup_aux(struct perf_event *event, void **pages,
+		     int nr_pages, bool overwrite)
 {
 	struct bts_buffer *buf;
 	struct page *page;
+	int cpu = event->cpu;
 	int node = (cpu == -1) ? cpu : cpu_to_node(cpu);
 	unsigned long offset;
 	size_t size = nr_pages << PAGE_SHIFT;
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 8d016ce5b80d..8f4c98fdd03c 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1104,10 +1104,11 @@ static int pt_buffer_init_topa(struct pt_buffer *buf, unsigned long nr_pages,
  * Return:	Our private PT buffer structure.
  */
 static void *
-pt_buffer_setup_aux(int cpu, void **pages, int nr_pages, bool snapshot)
+pt_buffer_setup_aux(struct perf_event *event, void **pages,
+		    int nr_pages, bool snapshot)
 {
 	struct pt_buffer *buf;
-	int node, ret;
+	int node, ret, cpu = event->cpu;
 
 	if (!nr_pages)
 		return NULL;
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 16b9c87d9d00..2bbd3240ff72 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -184,15 +184,15 @@ static void etm_free_aux(void *data)
 	schedule_work(&event_data->work);
 }
 
-static void *etm_setup_aux(int event_cpu, void **pages,
+static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
-	int cpu;
+	int cpu = event->cpu;
 	cpumask_t *mask;
 	struct coresight_device *sink;
 	struct etm_event_data *event_data = NULL;
 
-	event_data = alloc_event_data(event_cpu);
+	event_data = alloc_event_data(cpu);
 	if (!event_data)
 		return NULL;
 	INIT_WORK(&event_data->work, free_event_data);
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 54ec278d2fc4..4dcd7bf14dcc 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -824,10 +824,10 @@ static void arm_spe_pmu_read(struct perf_event *event)
 {
 }
 
-static void *arm_spe_pmu_setup_aux(int cpu, void **pages, int nr_pages,
-				   bool snapshot)
+static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
+				   int nr_pages, bool snapshot)
 {
-	int i;
+	int i, cpu = event->cpu;
 	struct page **pglist;
 	struct arm_spe_pmu_buf *buf;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 7064b513ca2b..d67074f81c23 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -420,7 +420,7 @@ struct pmu {
 	/*
 	 * Set up pmu-private data structures for an AUX area
 	 */
-	void *(*setup_aux)		(int cpu, void **pages,
+	void *(*setup_aux)		(struct perf_event *event, void **pages,
 					 int nr_pages, bool overwrite);
 					/* optional */
 
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 5d3cf407e374..4c96c7575224 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -648,7 +648,7 @@ int rb_alloc_aux(struct ring_buffer *rb, struct perf_event *event,
 			goto out;
 	}
 
-	rb->aux_priv = event->pmu->setup_aux(event->cpu, rb->aux_pages, nr_pages,
+	rb->aux_priv = event->pmu->setup_aux(event, rb->aux_pages, nr_pages,
 					     overwrite);
 	if (!rb->aux_priv)
 		goto out;
-- 
2.7.4

[PATCH v3 4/7] coresight: Use PMU driver configuration for sink selection

[Mathieu Poirier]

This patch uses the PMU driver configuration held in event::hw::drv_config
to select a sink for each event that is created (the old sysFS way of
working is kept around for backward compatibility).

By proceeding in this way a sink can be used by multiple sessions
without having to play games with entries in sysFS.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 134 +++++++++++++++++++++--
 drivers/hwtracing/coresight/coresight-etm-perf.h |   4 +
 2 files changed, 126 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 2bbd3240ff72..5c23b35a8ad0 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -11,6 +11,7 @@
 #include <linux/list.h>
 #include <linux/mm.h>
 #include <linux/init.h>
+#include <linux/parser.h>
 #include <linux/perf_event.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -22,6 +23,8 @@
 static struct pmu etm_pmu;
 static bool etm_perf_up;
 
+#define CORESIGHT_DEVICE_MAX_NAME_LEN	256
+
 /**
  * struct etm_event_data - Coresight specifics associated to an event
  * @work:		Handle to free allocated memory outside IRQ context.
@@ -184,6 +187,78 @@ static void etm_free_aux(void *data)
 	schedule_work(&event_data->work);
 }
 
+static char *etm_drv_config_sync(struct perf_event *event)
+{
+	char *sink;
+	int node = event->cpu == -1 ? -1 : cpu_to_node(event->cpu);
+	struct pmu_drv_config *drv_config = perf_event_get_drv_config(event);
+
+	sink = kmalloc_node(CORESIGHT_DEVICE_MAX_NAME_LEN, GFP_KERNEL, node);
+	if (!sink)
+		return NULL;
+
+	/*
+	 * Make sure we don't race with perf_drv_config_replace() in
+	 * kernel/events/core.c.
+	 */
+	raw_spin_lock(&drv_config->lock);
+	memcpy(sink, drv_config->config, CORESIGHT_DEVICE_MAX_NAME_LEN);
+	raw_spin_unlock(&drv_config->lock);
+
+	return sink;
+}
+
+static struct coresight_device *etm_event_get_sink(struct perf_event *event)
+{
+	struct pmu_drv_config *drv_config = perf_event_get_drv_config(event);
+
+	/* CPU-wide scenarios aren't supported yet */
+	if (event->cpu != -1)
+		return NULL;
+
+	/*
+	 * Try the preferred method first, i.e getting the sink information
+	 * using the ioctl() method.
+	 */
+	if (drv_config->config) {
+		char *drv_config;
+		struct device *dev;
+		struct coresight_device *sink;
+
+		/*
+		 * Get sink from event->hw.drv_config.config - see
+		 * _perf_ioctl() _SET_DRV_CONFIG.
+		 */
+		drv_config = etm_drv_config_sync(event);
+		if (!drv_config)
+			goto out;
+
+		/* Look for the device of that name on the CS bus. */
+		dev = bus_find_device_by_name(&coresight_bustype, NULL,
+					      drv_config);
+		kfree(drv_config);
+
+		if (dev) {
+			sink = to_coresight_device(dev);
+			/* Put reference from 'bus_find_device()' */
+			put_device(dev);
+			return sink;
+		}
+	}
+
+	/*
+	 * No luck with the above method, so we are working with an older user
+	 * space.  See if a sink has been set using sysFS.  If this is the case
+	 * CPU-wide session will only be able to use a single sink.
+	 *
+	 * When operated from sysFS users are responsible to enable the sink
+	 * while from perf, the perf tools will do it based on the choice made
+	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
+	 */
+out:
+	return coresight_get_enabled_sink(true);
+}
+
 static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
@@ -197,18 +272,8 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		return NULL;
 	INIT_WORK(&event_data->work, free_event_data);
 
-	/*
-	 * In theory nothing prevent tracers in a trace session from being
-	 * associated with different sinks, nor having a sink per tracer.  But
-	 * until we have HW with this kind of topology we need to assume tracers
-	 * in a trace session are using the same sink.  Therefore go through
-	 * the coresight bus and pick the first enabled sink.
-	 *
-	 * When operated from sysFS users are responsible to enable the sink
-	 * while from perf, the perf tools will do it based on the choice made
-	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
-	 */
-	sink = coresight_get_enabled_sink(true);
+	/* First get the sink to use for this event */
+	sink = etm_event_get_sink(event);
 	if (!sink)
 		goto err;
 
@@ -445,6 +510,49 @@ static void etm_addr_filters_sync(struct perf_event *event)
 	filters->nr_filters = i;
 }
 
+static const match_table_t config_tokens = {
+	{ ETM_CFG_SINK,		"%u.%s" },
+	{ ETM_CFG_NONE,		NULL },
+};
+
+static void *etm_drv_config_validate(struct perf_event *event, char *cstr)
+{
+	char *str, *to_parse, *sink = NULL;
+	int token, ret = -EINVAL;
+	substring_t args[MAX_OPT_ARGS];
+
+	to_parse = kstrdup(cstr, GFP_KERNEL);
+	if (!to_parse)
+		return ERR_PTR(-ENOMEM);
+
+	while ((str = strsep(&to_parse, " ,\n")) != NULL) {
+		if (!*str)
+			continue;
+
+		token = match_token(str, config_tokens, args);
+		switch (token) {
+		case ETM_CFG_SINK:
+			sink = kstrdup(str, GFP_KERNEL);
+			if (!sink) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			break;
+		default:
+			goto out;
+		}
+	}
+
+out:
+	kfree(to_parse);
+	return sink ? sink : ERR_PTR(ret);
+}
+
+static void etm_drv_config_free(void *drv_data)
+{
+	kfree(drv_data);
+}
+
 int etm_perf_symlink(struct coresight_device *csdev, bool link)
 {
 	char entry[sizeof("cpu9999999")];
@@ -489,6 +597,8 @@ static int __init etm_perf_init(void)
 	etm_pmu.addr_filters_sync	= etm_addr_filters_sync;
 	etm_pmu.addr_filters_validate	= etm_addr_filters_validate;
 	etm_pmu.nr_addr_filters		= ETM_ADDR_CMP_MAX;
+	etm_pmu.drv_config_validate	= etm_drv_config_validate;
+	etm_pmu.drv_config_free		= etm_drv_config_free;
 
 	ret = perf_pmu_register(&etm_pmu, CORESIGHT_ETM_PMU_NAME, -1);
 	if (ret == 0)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 4197df4faf5e..37a98230fc34 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -42,6 +42,10 @@ struct etm_filters {
 	bool			ssstatus;
 };
 
+enum etm_config_elem_type {
+	ETM_CFG_NONE = -1,
+	ETM_CFG_SINK,
+};
 
 #ifdef CONFIG_CORESIGHT
 int etm_perf_symlink(struct coresight_device *csdev, bool link);
-- 
2.7.4

[PATCH v3 5/7] perf tools: Use ioctl to communicate driver configuration to kernel

[Mathieu Poirier]

Following in the footsteps of what was done for filters, adding the
necessary mechanic needed to push down driver specific configuration
to the kernel using an ioctl. By proceeding this way PMU specific
configuration such as CoreSight sink specification can be communicated
to each event.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/util/evsel.c | 7 +++++++
 tools/perf/util/evsel.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 94fce4f537e9..534aca4c642c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1162,6 +1162,13 @@ static int perf_evsel__append_filter(struct perf_evsel *evsel,
 	return -1;
 }
 
+int perf_evsel__apply_drv_config(struct perf_evsel *evsel, const char *config)
+{
+	return perf_evsel__run_ioctl(evsel,
+				     PERF_EVENT_IOC_SET_DRV_CONFIG,
+				     (void *)config);
+}
+
 int perf_evsel__append_tp_filter(struct perf_evsel *evsel, const char *filter)
 {
 	return perf_evsel__append_filter(evsel, "(%s) && (%s)", filter);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d277930b19a1..0f671bd2a988 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -271,6 +271,7 @@ int perf_evsel__append_tp_filter(struct perf_evsel *evsel, const char *filter);
 int perf_evsel__append_addr_filter(struct perf_evsel *evsel,
 				   const char *filter);
 int perf_evsel__apply_filter(struct perf_evsel *evsel, const char *filter);
+int perf_evsel__apply_drv_config(struct perf_evsel *evsel, const char *config);
 int perf_evsel__enable(struct perf_evsel *evsel);
 int perf_evsel__disable(struct perf_evsel *evsel);
 
-- 
2.7.4

[PATCH v3 6/7] perf tools: Make perf_evsel accessible to PMU driver configuration code

[Mathieu Poirier]

Make structure perf_evsel available to the PMU driver configuration code.
That way function perf_evsel__apply_drv_config() can be used from within
that code and information pertaining to the 'perf_evsel_config_term' is
still available.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 22 +++++++++++++++++++++-
 tools/perf/arch/arm/util/cs-etm.h |  3 ++-
 tools/perf/util/drv_configs.c     | 30 +++++++-----------------------
 tools/perf/util/pmu.h             |  3 ++-
 4 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 2f595cd73da6..d8081c2e6d44 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -635,7 +635,7 @@ static int __printf(2, 3) cs_device__print_file(const char *name, const char *fm
 	return ret;
 }
 
-int cs_etm_set_drv_config(struct perf_evsel_config_term *term)
+static int cs_etm_set_drv_config_term(struct perf_evsel_config_term *term)
 {
 	int ret;
 	char enable_sink[ENABLE_SINK_MAX];
@@ -649,3 +649,23 @@ int cs_etm_set_drv_config(struct perf_evsel_config_term *term)
 
 	return 0;
 }
+
+int cs_etm_set_drv_config(struct perf_evsel *evsel,
+			  struct perf_evsel_config_term **err_term)
+{
+	int err = 0;
+	struct perf_evsel_config_term *term;
+
+	list_for_each_entry(term, &evsel->config_terms, list) {
+		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
+			continue;
+
+		err = cs_etm_set_drv_config_term(term);
+		if (err) {
+			*err_term = term;
+			break;
+		}
+	}
+
+	return err;
+}
diff --git a/tools/perf/arch/arm/util/cs-etm.h b/tools/perf/arch/arm/util/cs-etm.h
index 1a12e64f5127..a3f8dde6ccef 100644
--- a/tools/perf/arch/arm/util/cs-etm.h
+++ b/tools/perf/arch/arm/util/cs-etm.h
@@ -10,6 +10,7 @@
 #include "../../util/evsel.h"
 
 struct auxtrace_record *cs_etm_record_init(int *err);
-int cs_etm_set_drv_config(struct perf_evsel_config_term *term);
+int cs_etm_set_drv_config(struct perf_evsel *evsel,
+			  struct perf_evsel_config_term **err_term);
 
 #endif
diff --git a/tools/perf/util/drv_configs.c b/tools/perf/util/drv_configs.c
index eec754243f4d..f7c1bcf08549 100644
--- a/tools/perf/util/drv_configs.c
+++ b/tools/perf/util/drv_configs.c
@@ -25,7 +25,6 @@ perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
 {
 	bool found = false;
 	int err = 0;
-	struct perf_evsel_config_term *term;
 	struct perf_pmu *pmu = NULL;
 
 	while ((pmu = perf_pmu__scan(pmu)) != NULL)
@@ -34,29 +33,14 @@ perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
 			break;
 		}
 
-	list_for_each_entry(term, &evsel->config_terms, list) {
-		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
-			continue;
+	/*
+	 * No need to continue if we didn't get a match or if there is no
+	 * driver configuration function for this PMU.
+	 */
+	if (!found || !pmu->set_drv_config)
+		return err;
 
-		/*
-		 * We have a configuration term, report an error if we
-		 * can't find the PMU or if the PMU driver doesn't support
-		 * cmd line driver configuration.
-		 */
-		if (!found || !pmu->set_drv_config) {
-			err = -EINVAL;
-			*err_term = term;
-			break;
-		}
-
-		err = pmu->set_drv_config(term);
-		if (err) {
-			*err_term = term;
-			break;
-		}
-	}
-
-	return err;
+	return pmu->set_drv_config(evsel, err_term);
 }
 
 int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 76fecec7b3f9..47f44394042b 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -29,7 +29,8 @@ struct perf_pmu {
 	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
 	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
 	struct list_head list;    /* ELEM */
-	int (*set_drv_config)	(struct perf_evsel_config_term *term);
+	int (*set_drv_config)	(struct perf_evsel *evsel,
+				 struct perf_evsel_config_term **err_term);
 };
 
 struct perf_pmu_info {
-- 
2.7.4

[PATCH v3 7/7] perf tools: Use ioctl function to send sink configuration to kernel

[Mathieu Poirier]

Using sysFS to communicate sink information for a trace session doesn't
work when more than one CPU is involved in the scenario.  As such
communicate the sink information to each event by using the SET_DRV_CONFIG
ioctl command.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 54 ++++-----------------------------------
 1 file changed, 5 insertions(+), 49 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index d8081c2e6d44..cb0978eb7181 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -599,55 +599,10 @@ struct auxtrace_record *cs_etm_record_init(int *err)
 	return NULL;
 }
 
-static FILE *cs_device__open_file(const char *name)
+static int cs_etm_set_drv_config_term(struct perf_evsel *evsel,
+				      const char *term)
 {
-	struct stat st;
-	char path[PATH_MAX];
-	const char *sysfs;
-
-	sysfs = sysfs__mountpoint();
-	if (!sysfs)
-		return NULL;
-
-	snprintf(path, PATH_MAX,
-		 "%s" CS_BUS_DEVICE_PATH "%s", sysfs, name);
-
-	if (stat(path, &st) < 0)
-		return NULL;
-
-	return fopen(path, "w");
-
-}
-
-static int __printf(2, 3) cs_device__print_file(const char *name, const char *fmt, ...)
-{
-	va_list args;
-	FILE *file;
-	int ret = -EINVAL;
-
-	va_start(args, fmt);
-	file = cs_device__open_file(name);
-	if (file) {
-		ret = vfprintf(file, fmt, args);
-		fclose(file);
-	}
-	va_end(args);
-	return ret;
-}
-
-static int cs_etm_set_drv_config_term(struct perf_evsel_config_term *term)
-{
-	int ret;
-	char enable_sink[ENABLE_SINK_MAX];
-
-	snprintf(enable_sink, ENABLE_SINK_MAX, "%s/%s",
-		 term->val.drv_cfg, "enable_sink");
-
-	ret = cs_device__print_file(enable_sink, "%d", 1);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return perf_evsel__apply_drv_config(evsel, term);
 }
 
 int cs_etm_set_drv_config(struct perf_evsel *evsel,
@@ -660,7 +615,8 @@ int cs_etm_set_drv_config(struct perf_evsel *evsel,
 		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
 			continue;
 
-		err = cs_etm_set_drv_config_term(term);
+		err = cs_etm_set_drv_config_term(evsel,
+						 term->val.drv_cfg);
 		if (err) {
 			*err_term = term;
 			break;
-- 
2.7.4

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Wed, 18 Jul 2018 15:48:00 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> This set adds the capability to communiate event specific configuration
> to the PMU kernel driver using an ioctl().  The functionatlity is made
> generic enough for anyone to use but is targeted at the identification
> of CoreSight sinks when operating in CPU-wide trace scenarios.
> 
> ---
> Changes for V3:
> . Return an error for CPU-wide scenarios while the feature is being
>   implemented (Kim)

Hi, I'm giving this series a short test-drive on Juno.

It yields success for the --per-thread case..:

$ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a
Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux
[ perf record: Woken up 0 times to write data ]
Warning:
AUX data lost 1 times out of 2!

[ perf record: Captured and wrote 0.067 MB perf.data ]
$ 

..but not for CPU-wide?:

$ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a
failed to mmap with 12 (Cannot allocate memory)
$ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a
failed to mmap with 12 (Cannot allocate memory)
$ 

Thanks,

Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Mathieu Poirier]

On Mon, 13 Aug 2018 at 11:46, Kim Phillips <kim.phillips@arm.com> wrote:
>
> On Wed, 18 Jul 2018 15:48:00 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> > This set adds the capability to communiate event specific configuration
> > to the PMU kernel driver using an ioctl().  The functionatlity is made
> > generic enough for anyone to use but is targeted at the identification
> > of CoreSight sinks when operating in CPU-wide trace scenarios.
> >
> > ---
> > Changes for V3:
> > . Return an error for CPU-wide scenarios while the feature is being
> >   implemented (Kim)
>
> Hi, I'm giving this series a short test-drive on Juno.

Hi,

>
> It yields success for the --per-thread case..:
>
> $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a
> Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux
> [ perf record: Woken up 0 times to write data ]
> Warning:
> AUX data lost 1 times out of 2!
>
> [ perf record: Captured and wrote 0.067 MB perf.data ]
> $
>
> ..but not for CPU-wide?:
>
> $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a
> failed to mmap with 12 (Cannot allocate memory)
> $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a
> failed to mmap with 12 (Cannot allocate memory)
> $

This patchset is getting very old and a fair amount of things have
changed since then.  I'm hoping to be coming out with a new one
shortly.  Nonetheless the above is returning an error in CPU-wide
scenarios while the feature is being implemented.  Isn't what you
requested or have I misunderstood your comment?

>
> Thanks,
>
> Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Tue, 14 Aug 2018 10:15:56 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On Mon, 13 Aug 2018 at 11:46, Kim Phillips <kim.phillips@arm.com> wrote:
> > It yields success for the --per-thread case..:
> >
> > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a
> > Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux
> > [ perf record: Woken up 0 times to write data ]
> > Warning:
> > AUX data lost 1 times out of 2!
> >
> > [ perf record: Captured and wrote 0.067 MB perf.data ]
> > $
> >
> > ..but not for CPU-wide?:
> >
> > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a
> > failed to mmap with 12 (Cannot allocate memory)
> > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a
> > failed to mmap with 12 (Cannot allocate memory)
> > $
> 
> This patchset is getting very old and a fair amount of things have
> changed since then.  I'm hoping to be coming out with a new one
> shortly.  Nonetheless the above is returning an error in CPU-wide
> scenarios while the feature is being implemented.  Isn't what you
> requested or have I misunderstood your comment?

No, sigh, I just automatically assumed the patchset would include
CPU-wide support again.  If it were being done that way, we'd all know
that the feature(s) this patchset adds would be doing the right thing
for that purpose, guaranteed.

The other thing that's going on here is that I'm becoming numb to the
loathsome "failed to mmap with 12 (Cannot allocate memory)" being
returned no matter what the error is/was.  E.g., an error that would
indicate a sense of non-implementation would be much better
appreciated than presumably what the above is doing, i.e., returning
-ENOMEM.  That, backed up with specific details in the form of human
readable text in dmesg would be *most* welcome.

Thanks,

Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Mathieu Poirier]

On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
>
> On Tue, 14 Aug 2018 10:15:56 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> > On Mon, 13 Aug 2018 at 11:46, Kim Phillips <kim.phillips@arm.com> wrote:
> > > It yields success for the --per-thread case..:
> > >
> > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a
> > > Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux
> > > [ perf record: Woken up 0 times to write data ]
> > > Warning:
> > > AUX data lost 1 times out of 2!
> > >
> > > [ perf record: Captured and wrote 0.067 MB perf.data ]
> > > $
> > >
> > > ..but not for CPU-wide?:
> > >
> > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a
> > > failed to mmap with 12 (Cannot allocate memory)
> > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a
> > > failed to mmap with 12 (Cannot allocate memory)
> > > $
> >
> > This patchset is getting very old and a fair amount of things have
> > changed since then.  I'm hoping to be coming out with a new one
> > shortly.  Nonetheless the above is returning an error in CPU-wide
> > scenarios while the feature is being implemented.  Isn't what you
> > requested or have I misunderstood your comment?
>
> No, sigh, I just automatically assumed the patchset would include
> CPU-wide support again.  If it were being done that way, we'd all know
> that the feature(s) this patchset adds would be doing the right thing
> for that purpose, guaranteed.

The patchset published on this list never had support for CPU-wide scenarios.

This is only a preparatory step, the first one in a few more to come.
Sending the whole thing in one go would be way too heavy and is not
realistic.

>
> The other thing that's going on here is that I'm becoming numb to the
> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> returned no matter what the error is/was. E.g., an error that would
> indicate a sense of non-implementation would be much better
> appreciated than presumably what the above is doing, i.e., returning
> -ENOMEM.  That, backed up with specific details in the form of human
> readable text in dmesg would be *most* welcome.

As part of the refactoring of the code to support CPU-wide scenarios I
intend to emit better diagnostic messages from the driver.  Modifying
rb_alloc_aux() to propagate the error message generated by the
architecture specific PMUs doesn't look hard either and I _may_ get to
it as part of this work.

Mathieu

>
> Thanks,
>
> Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Will Deacon]

On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> > The other thing that's going on here is that I'm becoming numb to the
> > loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> > returned no matter what the error is/was. E.g., an error that would
> > indicate a sense of non-implementation would be much better
> > appreciated than presumably what the above is doing, i.e., returning
> > -ENOMEM.  That, backed up with specific details in the form of human
> > readable text in dmesg would be *most* welcome.
> 
> As part of the refactoring of the code to support CPU-wide scenarios I
> intend to emit better diagnostic messages from the driver.  Modifying
> rb_alloc_aux() to propagate the error message generated by the
> architecture specific PMUs doesn't look hard either and I _may_ get to
> it as part of this work.

For the record, I will continue to oppose PMU drivers that dump diagnostics
about user-controlled input into dmesg, but the coresight drivers are yours
so it's up to you and I won't get in the way!

Will

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Wed, 15 Aug 2018 10:39:13 +0100
Will Deacon <will.deacon@arm.com> wrote:

> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> > On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> > > The other thing that's going on here is that I'm becoming numb to the
> > > loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> > > returned no matter what the error is/was. E.g., an error that would
> > > indicate a sense of non-implementation would be much better
> > > appreciated than presumably what the above is doing, i.e., returning
> > > -ENOMEM.  That, backed up with specific details in the form of human
> > > readable text in dmesg would be *most* welcome.
> > 
> > As part of the refactoring of the code to support CPU-wide scenarios I
> > intend to emit better diagnostic messages from the driver.  Modifying
> > rb_alloc_aux() to propagate the error message generated by the
> > architecture specific PMUs doesn't look hard either and I _may_ get to
> > it as part of this work.
> 
> For the record, I will continue to oppose PMU drivers that dump diagnostics
> about user-controlled input into dmesg, but the coresight drivers are yours
> so it's up to you and I won't get in the way!

That sounds technically self-contradicting to me.  Why shouldn't
coresight share the same policies as those used for PMU drivers?  Or
why not allow the individual vendor PMU driver authors control the
level of user-friendliness of their own drivers?

That being said, Matheiu, would you accept patches that make coresight
more verbose in dmesg?

Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Mathieu Poirier]

On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
>
> On Wed, 15 Aug 2018 10:39:13 +0100
> Will Deacon <will.deacon@arm.com> wrote:
>
> > On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> > > On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> > > > The other thing that's going on here is that I'm becoming numb to the
> > > > loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> > > > returned no matter what the error is/was. E.g., an error that would
> > > > indicate a sense of non-implementation would be much better
> > > > appreciated than presumably what the above is doing, i.e., returning
> > > > -ENOMEM.  That, backed up with specific details in the form of human
> > > > readable text in dmesg would be *most* welcome.
> > >
> > > As part of the refactoring of the code to support CPU-wide scenarios I
> > > intend to emit better diagnostic messages from the driver.  Modifying
> > > rb_alloc_aux() to propagate the error message generated by the
> > > architecture specific PMUs doesn't look hard either and I _may_ get to
> > > it as part of this work.
> >
> > For the record, I will continue to oppose PMU drivers that dump diagnostics
> > about user-controlled input into dmesg, but the coresight drivers are yours
> > so it's up to you and I won't get in the way!
>
> That sounds technically self-contradicting to me.  Why shouldn't
> coresight share the same policies as those used for PMU drivers?  Or
> why not allow the individual vendor PMU driver authors control the
> level of user-friendliness of their own drivers?
>
> That being said, Matheiu, would you accept patches that make coresight
> more verbose in dmesg?

It depends on the issue you're hoping to address.  I'd rather see the
root cause of the problem fixed than adding temporary code.  Suzuki
added the ETR perf API and I'm currently working on CPU-wide
scenarios.  From there and with regards to what can happen in
setup_aux(), we should have things covered.

>
> Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Suzuki K Poulose]

On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
>>
>> On Wed, 15 Aug 2018 10:39:13 +0100
>> Will Deacon <will.deacon@arm.com> wrote:
>>
>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
>>>>> The other thing that's going on here is that I'm becoming numb to the
>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
>>>>> returned no matter what the error is/was. E.g., an error that would
>>>>> indicate a sense of non-implementation would be much better
>>>>> appreciated than presumably what the above is doing, i.e., returning
>>>>> -ENOMEM.  That, backed up with specific details in the form of human
>>>>> readable text in dmesg would be *most* welcome.
>>>>
>>>> As part of the refactoring of the code to support CPU-wide scenarios I
>>>> intend to emit better diagnostic messages from the driver.  Modifying
>>>> rb_alloc_aux() to propagate the error message generated by the
>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
>>>> it as part of this work.
>>>
>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
>>> about user-controlled input into dmesg, but the coresight drivers are yours
>>> so it's up to you and I won't get in the way!
>>
>> That sounds technically self-contradicting to me.  Why shouldn't
>> coresight share the same policies as those used for PMU drivers?  Or
>> why not allow the individual vendor PMU driver authors control the
>> level of user-friendliness of their own drivers?
>>
>> That being said, Matheiu, would you accept patches that make coresight
>> more verbose in dmesg?
> 
> It depends on the issue you're hoping to address.  I'd rather see the
> root cause of the problem fixed than adding temporary code.  Suzuki
> added the ETR perf API and I'm currently working on CPU-wide
> scenarios.  From there and with regards to what can happen in
> setup_aux(), we should have things covered.

I think the main issue is the lack of error code propagation from
setup_aux() back to the perf_aux_output_handle_begin(), which always
return -ENOMEM. If we fix that, we could get better idea of whats
wrong.


If someone is planning to add verbose messages, they may do so by adding
dev_dbg() / pr_debug(), which can be turned on as and when needed.

Suzuki

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Mon, 20 Aug 2018 11:03:03 +0100
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:

> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
> > On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
> >>
> >> On Wed, 15 Aug 2018 10:39:13 +0100
> >> Will Deacon <will.deacon@arm.com> wrote:
> >>
> >>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> >>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>> The other thing that's going on here is that I'm becoming numb to the
> >>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> >>>>> returned no matter what the error is/was. E.g., an error that would
> >>>>> indicate a sense of non-implementation would be much better
> >>>>> appreciated than presumably what the above is doing, i.e., returning
> >>>>> -ENOMEM.  That, backed up with specific details in the form of human
> >>>>> readable text in dmesg would be *most* welcome.
> >>>>
> >>>> As part of the refactoring of the code to support CPU-wide scenarios I
> >>>> intend to emit better diagnostic messages from the driver.  Modifying
> >>>> rb_alloc_aux() to propagate the error message generated by the
> >>>> architecture specific PMUs doesn't look hard either and I _may_ get to
> >>>> it as part of this work.
> >>>
> >>> For the record, I will continue to oppose PMU drivers that dump diagnostics
> >>> about user-controlled input into dmesg, but the coresight drivers are yours
> >>> so it's up to you and I won't get in the way!
> >>
> >> That sounds technically self-contradicting to me.  Why shouldn't
> >> coresight share the same policies as those used for PMU drivers?  Or
> >> why not allow the individual vendor PMU driver authors control the
> >> level of user-friendliness of their own drivers?
> >>
> >> That being said, Matheiu, would you accept patches that make coresight
> >> more verbose in dmesg?
> > 
> > It depends on the issue you're hoping to address.  I'd rather see the
> > root cause of the problem fixed than adding temporary code.  Suzuki
> > added the ETR perf API and I'm currently working on CPU-wide
> > scenarios.  From there and with regards to what can happen in
> > setup_aux(), we should have things covered.
> 
> I think the main issue is the lack of error code propagation from
> setup_aux() back to the perf_aux_output_handle_begin(), which always
> return -ENOMEM. If we fix that, we could get better idea of whats
> wrong.

Why get a better idea when we can get the exact details?

> If someone is planning to add verbose messages, they may do so by adding
> dev_dbg() / pr_debug(), which can be turned on as and when needed.

I disagree:  that just adds another usage and kernel configuration
obstacle.  Why not use pr_err straight up?

Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Suzuki K Poulose]

On 08/20/2018 03:22 PM, Kim Phillips wrote:
> On Mon, 20 Aug 2018 11:03:03 +0100
> Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
>> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
>>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
>>>>
>>>> On Wed, 15 Aug 2018 10:39:13 +0100
>>>> Will Deacon <will.deacon@arm.com> wrote:
>>>>
>>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
>>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
>>>>>>> The other thing that's going on here is that I'm becoming numb to the
>>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
>>>>>>> returned no matter what the error is/was. E.g., an error that would
>>>>>>> indicate a sense of non-implementation would be much better
>>>>>>> appreciated than presumably what the above is doing, i.e., returning
>>>>>>> -ENOMEM.  That, backed up with specific details in the form of human
>>>>>>> readable text in dmesg would be *most* welcome.
>>>>>>
>>>>>> As part of the refactoring of the code to support CPU-wide scenarios I
>>>>>> intend to emit better diagnostic messages from the driver.  Modifying
>>>>>> rb_alloc_aux() to propagate the error message generated by the
>>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
>>>>>> it as part of this work.
>>>>>
>>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
>>>>> about user-controlled input into dmesg, but the coresight drivers are yours
>>>>> so it's up to you and I won't get in the way!
>>>>
>>>> That sounds technically self-contradicting to me.  Why shouldn't
>>>> coresight share the same policies as those used for PMU drivers?  Or
>>>> why not allow the individual vendor PMU driver authors control the
>>>> level of user-friendliness of their own drivers?
>>>>
>>>> That being said, Matheiu, would you accept patches that make coresight
>>>> more verbose in dmesg?
>>>
>>> It depends on the issue you're hoping to address.  I'd rather see the
>>> root cause of the problem fixed than adding temporary code.  Suzuki
>>> added the ETR perf API and I'm currently working on CPU-wide
>>> scenarios.  From there and with regards to what can happen in
>>> setup_aux(), we should have things covered.
>>
>> I think the main issue is the lack of error code propagation from
>> setup_aux() back to the perf_aux_output_handle_begin(), which always
>> return -ENOMEM. If we fix that, we could get better idea of whats
>> wrong.
> 
> Why get a better idea when we can get the exact details?

The different values for error numbers are there for a reason...

> 
>> If someone is planning to add verbose messages, they may do so by adding
>> dev_dbg() / pr_debug(), which can be turned on as and when needed.
> 
> I disagree:  that just adds another usage and kernel configuration
> obstacle.  Why not use pr_err straight up?

I personally don't agree to usage of pr_err() in paths which are easily
triggered from user input. Also, we are moving all the "debugging"
messages to the dynamic debug, to prevent lockdep splats.

Suzuki

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Mon, 20 Aug 2018 15:36:47 +0100
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:

> On 08/20/2018 03:22 PM, Kim Phillips wrote:
> > On Mon, 20 Aug 2018 11:03:03 +0100
> > Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > 
> >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
> >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>
> >>>> On Wed, 15 Aug 2018 10:39:13 +0100
> >>>> Will Deacon <will.deacon@arm.com> wrote:
> >>>>
> >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>>>> The other thing that's going on here is that I'm becoming numb to the
> >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> >>>>>>> returned no matter what the error is/was. E.g., an error that would
> >>>>>>> indicate a sense of non-implementation would be much better
> >>>>>>> appreciated than presumably what the above is doing, i.e., returning
> >>>>>>> -ENOMEM.  That, backed up with specific details in the form of human
> >>>>>>> readable text in dmesg would be *most* welcome.
> >>>>>>
> >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I
> >>>>>> intend to emit better diagnostic messages from the driver.  Modifying
> >>>>>> rb_alloc_aux() to propagate the error message generated by the
> >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
> >>>>>> it as part of this work.
> >>>>>
> >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
> >>>>> about user-controlled input into dmesg, but the coresight drivers are yours
> >>>>> so it's up to you and I won't get in the way!
> >>>>
> >>>> That sounds technically self-contradicting to me.  Why shouldn't
> >>>> coresight share the same policies as those used for PMU drivers?  Or
> >>>> why not allow the individual vendor PMU driver authors control the
> >>>> level of user-friendliness of their own drivers?
> >>>>
> >>>> That being said, Matheiu, would you accept patches that make coresight
> >>>> more verbose in dmesg?
> >>>
> >>> It depends on the issue you're hoping to address.  I'd rather see the
> >>> root cause of the problem fixed than adding temporary code.  Suzuki
> >>> added the ETR perf API and I'm currently working on CPU-wide
> >>> scenarios.  From there and with regards to what can happen in
> >>> setup_aux(), we should have things covered.
> >>
> >> I think the main issue is the lack of error code propagation from
> >> setup_aux() back to the perf_aux_output_handle_begin(), which always
> >> return -ENOMEM. If we fix that, we could get better idea of whats
> >> wrong.
> > 
> > Why get a better idea when we can get the exact details?
> 
> The different values for error numbers are there for a reason...

But the same error number, e.g., EINVAL, can be returned for different
reasons.

> >> If someone is planning to add verbose messages, they may do so by adding
> >> dev_dbg() / pr_debug(), which can be turned on as and when needed.
> > 
> > I disagree:  that just adds another usage and kernel configuration
> > obstacle.  Why not use pr_err straight up?
> 
> I personally don't agree to usage of pr_err() in paths which are easily
> triggered from user input.

Why not?  pr_* are ratelimited.

> Also, we are moving all the "debugging"
> messages to the dynamic debug, to prevent lockdep splats.

Are you referring to this?:

https://lkml.org/lkml/2018/5/1/73

Re-reading the thread, AFAICT, it was never proven that the splats
occurred due to the dev_infos, and there's no dev_info in this
stacktrace:

https://lkml.org/lkml/2018/5/10/269

But even if it were, wouldn't the splats also occur with dev_dbg?

Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Suzuki K Poulose]

On 08/20/2018 04:25 PM, Kim Phillips wrote:
> On Mon, 20 Aug 2018 15:36:47 +0100
> Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
>> On 08/20/2018 03:22 PM, Kim Phillips wrote:
>>> On Mon, 20 Aug 2018 11:03:03 +0100
>>> Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>>
>>>> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
>>>>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
>>>>>>
>>>>>> On Wed, 15 Aug 2018 10:39:13 +0100
>>>>>> Will Deacon <will.deacon@arm.com> wrote:
>>>>>>
>>>>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
>>>>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
>>>>>>>>> The other thing that's going on here is that I'm becoming numb to the
>>>>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
>>>>>>>>> returned no matter what the error is/was. E.g., an error that would
>>>>>>>>> indicate a sense of non-implementation would be much better
>>>>>>>>> appreciated than presumably what the above is doing, i.e., returning
>>>>>>>>> -ENOMEM.  That, backed up with specific details in the form of human
>>>>>>>>> readable text in dmesg would be *most* welcome.
>>>>>>>>
>>>>>>>> As part of the refactoring of the code to support CPU-wide scenarios I
>>>>>>>> intend to emit better diagnostic messages from the driver.  Modifying
>>>>>>>> rb_alloc_aux() to propagate the error message generated by the
>>>>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
>>>>>>>> it as part of this work.
>>>>>>>
>>>>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
>>>>>>> about user-controlled input into dmesg, but the coresight drivers are yours
>>>>>>> so it's up to you and I won't get in the way!
>>>>>>
>>>>>> That sounds technically self-contradicting to me.  Why shouldn't
>>>>>> coresight share the same policies as those used for PMU drivers?  Or
>>>>>> why not allow the individual vendor PMU driver authors control the
>>>>>> level of user-friendliness of their own drivers?
>>>>>>
>>>>>> That being said, Matheiu, would you accept patches that make coresight
>>>>>> more verbose in dmesg?
>>>>>
>>>>> It depends on the issue you're hoping to address.  I'd rather see the
>>>>> root cause of the problem fixed than adding temporary code.  Suzuki
>>>>> added the ETR perf API and I'm currently working on CPU-wide
>>>>> scenarios.  From there and with regards to what can happen in
>>>>> setup_aux(), we should have things covered.
>>>>
>>>> I think the main issue is the lack of error code propagation from
>>>> setup_aux() back to the perf_aux_output_handle_begin(), which always
>>>> return -ENOMEM. If we fix that, we could get better idea of whats
>>>> wrong.
>>>
>>> Why get a better idea when we can get the exact details?
>>
>> The different values for error numbers are there for a reason...
> 
> But the same error number, e.g., EINVAL, can be returned for different
> reasons.

True, but then you can narrow it down by tuning it. We do that for
several syscalls without printing any useful messages to debug. Why
should the perf syscalls be any different ?

> 
>>>> If someone is planning to add verbose messages, they may do so by adding
>>>> dev_dbg() / pr_debug(), which can be turned on as and when needed.
>>>
>>> I disagree:  that just adds another usage and kernel configuration
>>> obstacle.  Why not use pr_err straight up?
>>
>> I personally don't agree to usage of pr_err() in paths which are easily
>> triggered from user input.
> 
> Why not?  pr_* are ratelimited.
> 
>> Also, we are moving all the "debugging"
>> messages to the dynamic debug, to prevent lockdep splats.
> 
> Are you referring to this?:
> 
> https://lkml.org/lkml/2018/5/1/7

Definitely not that thread.

> 
> Re-reading the thread, AFAICT, it was never proven that the splats
> occurred due to the dev_infos, and there's no dev_info in this
> stacktrace:
> 
> https://lkml.org/lkml/2018/5/10/269

That doesn't have the stack trace. Mathieu was also able to reproduce
the lockdep splat involving console lock lately. Unfortunately none of
these were captured here.

> 
> But even if it were, wouldn't the splats also occur with dev_dbg?

Not normally. dev_dbg() has to be turned on manually and the user knows
what he is doing. That allows the normal user to use the system without
any trouble.

Suzuki

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Tue, 21 Aug 2018 09:39:22 +0100
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:

> On 08/20/2018 04:25 PM, Kim Phillips wrote:
> > On Mon, 20 Aug 2018 15:36:47 +0100
> > Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > 
> >> On 08/20/2018 03:22 PM, Kim Phillips wrote:
> >>> On Mon, 20 Aug 2018 11:03:03 +0100
> >>> Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> >>>
> >>>> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
> >>>>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>>>
> >>>>>> On Wed, 15 Aug 2018 10:39:13 +0100
> >>>>>> Will Deacon <will.deacon@arm.com> wrote:
> >>>>>>
> >>>>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> >>>>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>>>>>> The other thing that's going on here is that I'm becoming numb to the
> >>>>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> >>>>>>>>> returned no matter what the error is/was. E.g., an error that would
> >>>>>>>>> indicate a sense of non-implementation would be much better
> >>>>>>>>> appreciated than presumably what the above is doing, i.e., returning
> >>>>>>>>> -ENOMEM.  That, backed up with specific details in the form of human
> >>>>>>>>> readable text in dmesg would be *most* welcome.
> >>>>>>>>
> >>>>>>>> As part of the refactoring of the code to support CPU-wide scenarios I
> >>>>>>>> intend to emit better diagnostic messages from the driver.  Modifying
> >>>>>>>> rb_alloc_aux() to propagate the error message generated by the
> >>>>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
> >>>>>>>> it as part of this work.
> >>>>>>>
> >>>>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
> >>>>>>> about user-controlled input into dmesg, but the coresight drivers are yours
> >>>>>>> so it's up to you and I won't get in the way!
> >>>>>>
> >>>>>> That sounds technically self-contradicting to me.  Why shouldn't
> >>>>>> coresight share the same policies as those used for PMU drivers?  Or
> >>>>>> why not allow the individual vendor PMU driver authors control the
> >>>>>> level of user-friendliness of their own drivers?
> >>>>>>
> >>>>>> That being said, Matheiu, would you accept patches that make coresight
> >>>>>> more verbose in dmesg?
> >>>>>
> >>>>> It depends on the issue you're hoping to address.  I'd rather see the
> >>>>> root cause of the problem fixed than adding temporary code.  Suzuki
> >>>>> added the ETR perf API and I'm currently working on CPU-wide
> >>>>> scenarios.  From there and with regards to what can happen in
> >>>>> setup_aux(), we should have things covered.
> >>>>
> >>>> I think the main issue is the lack of error code propagation from
> >>>> setup_aux() back to the perf_aux_output_handle_begin(), which always
> >>>> return -ENOMEM. If we fix that, we could get better idea of whats
> >>>> wrong.
> >>>
> >>> Why get a better idea when we can get the exact details?
> >>
> >> The different values for error numbers are there for a reason...
> > 
> > But the same error number, e.g., EINVAL, can be returned for different
> > reasons.
> 
> True, but then you can narrow it down by tuning it.

I'm trying to avoid having our users have to do that every time
something isn't quite right :)

> We do that for
> several syscalls without printing any useful messages to debug. Why
> should the perf syscalls be any different ?

Not sure what syscalls you kernel hackers are talking about, nor
whether the work was being done on behalf of an end-user, but things
like mount have the same problem as perf, and perf is notorious for it.

> >>>> If someone is planning to add verbose messages, they may do so by adding
> >>>> dev_dbg() / pr_debug(), which can be turned on as and when needed.
> >>>
> >>> I disagree:  that just adds another usage and kernel configuration
> >>> obstacle.  Why not use pr_err straight up?
> >>
> >> I personally don't agree to usage of pr_err() in paths which are easily
> >> triggered from user input.
> > 
> > Why not?  pr_* are ratelimited.
> > 
> >> Also, we are moving all the "debugging"
> >> messages to the dynamic debug, to prevent lockdep splats.
> > 
> > Are you referring to this?:
> > 
> > https://lkml.org/lkml/2018/5/1/7
> 
> Definitely not that thread.

bah, sorry, apparently the trailing 3 was removed somehow.  Here you go:

https://lkml.org/lkml/2018/5/1/73

> > Re-reading the thread, AFAICT, it was never proven that the splats
> > occurred due to the dev_infos, and there's no dev_info in this
> > stacktrace:
> > 
> > https://lkml.org/lkml/2018/5/10/269
> 
> That doesn't have the stack trace. Mathieu was also able to reproduce
> the lockdep splat involving console lock lately. Unfortunately none of
> these were captured here.

OK, I'd rather we see and properly debug and fix the so-called lockdep
splats rather than hiding them by lowering their loglevel.

> > But even if it were, wouldn't the splats also occur with dev_dbg?
> 
> Not normally. dev_dbg() has to be turned on manually and the user knows
> what he is doing. That allows the normal user to use the system without
> any trouble.

I thought it was obvious that I was talking about when dev_dbg _is_
enabled.  There, the problem still occurs, so you can't say 'we are
moving all the "debugging" messages to the dynamic debug, to prevent
lockdep splats.'

Please let's see the lockdep splat and the proper fix for it instead of
paper-taping over it via dev_dbg, thanks.

Kim

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Mathieu Poirier]

On Mon, 20 Aug 2018 at 08:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 08/20/2018 03:22 PM, Kim Phillips wrote:
> > On Mon, 20 Aug 2018 11:03:03 +0100
> > Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> >
> >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
> >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>
> >>>> On Wed, 15 Aug 2018 10:39:13 +0100
> >>>> Will Deacon <will.deacon@arm.com> wrote:
> >>>>
> >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> >>>>>>> The other thing that's going on here is that I'm becoming numb to the
> >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> >>>>>>> returned no matter what the error is/was. E.g., an error that would
> >>>>>>> indicate a sense of non-implementation would be much better
> >>>>>>> appreciated than presumably what the above is doing, i.e., returning
> >>>>>>> -ENOMEM.  That, backed up with specific details in the form of human
> >>>>>>> readable text in dmesg would be *most* welcome.
> >>>>>>
> >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I
> >>>>>> intend to emit better diagnostic messages from the driver.  Modifying
> >>>>>> rb_alloc_aux() to propagate the error message generated by the
> >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
> >>>>>> it as part of this work.
> >>>>>
> >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
> >>>>> about user-controlled input into dmesg, but the coresight drivers are yours
> >>>>> so it's up to you and I won't get in the way!
> >>>>
> >>>> That sounds technically self-contradicting to me.  Why shouldn't
> >>>> coresight share the same policies as those used for PMU drivers?  Or
> >>>> why not allow the individual vendor PMU driver authors control the
> >>>> level of user-friendliness of their own drivers?
> >>>>
> >>>> That being said, Matheiu, would you accept patches that make coresight
> >>>> more verbose in dmesg?
> >>>
> >>> It depends on the issue you're hoping to address.  I'd rather see the
> >>> root cause of the problem fixed than adding temporary code.  Suzuki
> >>> added the ETR perf API and I'm currently working on CPU-wide
> >>> scenarios.  From there and with regards to what can happen in
> >>> setup_aux(), we should have things covered.
> >>
> >> I think the main issue is the lack of error code propagation from
> >> setup_aux() back to the perf_aux_output_handle_begin(), which always
> >> return -ENOMEM. If we fix that, we could get better idea of whats
> >> wrong.
> >
> > Why get a better idea when we can get the exact details?
>
> The different values for error numbers are there for a reason...
>
> >
> >> If someone is planning to add verbose messages, they may do so by adding
> >> dev_dbg() / pr_debug(), which can be turned on as and when needed.
> >
> > I disagree:  that just adds another usage and kernel configuration
> > obstacle.  Why not use pr_err straight up?

I think everything on this topic has been said already.  As I remarked
earlier once I'm done with CPU-wide support we shouldn't need detailed
error reporting.  Everything should be handled via error code
propagation and that is what I'd like to see addressed in the first
place.  From there we can think about individual error cases as they
come up.

>
> I personally don't agree to usage of pr_err() in paths which are easily
> triggered from user input. Also, we are moving all the "debugging"
> messages to the dynamic debug, to prevent lockdep splats.

A slight correction here - we are moving most of the framework error
reporting to dynamic debug because they clog the log file and aren't
useful outside of a development context.  It is not a remedy for the
negative interaction between event locking and console access
generated by the framework's reporting of device status as a path is
built/enabled/disabled.

>
> Suzuki

Re: [PATCH v3 0/7] perf: Add ioctl for PMU driver configuration

[Kim Phillips]

On Tue, 21 Aug 2018 11:16:49 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On Mon, 20 Aug 2018 at 08:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> >
> > On 08/20/2018 03:22 PM, Kim Phillips wrote:
> > > On Mon, 20 Aug 2018 11:03:03 +0100
> > > Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > >
> > >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote:
> > >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote:
> > >>>>
> > >>>> On Wed, 15 Aug 2018 10:39:13 +0100
> > >>>> Will Deacon <will.deacon@arm.com> wrote:
> > >>>>
> > >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote:
> > >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote:
> > >>>>>>> The other thing that's going on here is that I'm becoming numb to the
> > >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being
> > >>>>>>> returned no matter what the error is/was. E.g., an error that would
> > >>>>>>> indicate a sense of non-implementation would be much better
> > >>>>>>> appreciated than presumably what the above is doing, i.e., returning
> > >>>>>>> -ENOMEM.  That, backed up with specific details in the form of human
> > >>>>>>> readable text in dmesg would be *most* welcome.
> > >>>>>>
> > >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I
> > >>>>>> intend to emit better diagnostic messages from the driver.  Modifying
> > >>>>>> rb_alloc_aux() to propagate the error message generated by the
> > >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to
> > >>>>>> it as part of this work.
> > >>>>>
> > >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics
> > >>>>> about user-controlled input into dmesg, but the coresight drivers are yours
> > >>>>> so it's up to you and I won't get in the way!
> > >>>>
> > >>>> That sounds technically self-contradicting to me.  Why shouldn't
> > >>>> coresight share the same policies as those used for PMU drivers?  Or
> > >>>> why not allow the individual vendor PMU driver authors control the
> > >>>> level of user-friendliness of their own drivers?
> > >>>>
> > >>>> That being said, Matheiu, would you accept patches that make coresight
> > >>>> more verbose in dmesg?
> > >>>
> > >>> It depends on the issue you're hoping to address.  I'd rather see the
> > >>> root cause of the problem fixed than adding temporary code.  Suzuki
> > >>> added the ETR perf API and I'm currently working on CPU-wide
> > >>> scenarios.  From there and with regards to what can happen in
> > >>> setup_aux(), we should have things covered.
> > >>
> > >> I think the main issue is the lack of error code propagation from
> > >> setup_aux() back to the perf_aux_output_handle_begin(), which always
> > >> return -ENOMEM. If we fix that, we could get better idea of whats
> > >> wrong.
> > >
> > > Why get a better idea when we can get the exact details?
> >
> > The different values for error numbers are there for a reason...
> >
> > >
> > >> If someone is planning to add verbose messages, they may do so by adding
> > >> dev_dbg() / pr_debug(), which can be turned on as and when needed.
> > >
> > > I disagree:  that just adds another usage and kernel configuration
> > > obstacle.  Why not use pr_err straight up?
> 
> I think everything on this topic has been said already.  As I remarked

Apparently not :)

> earlier once I'm done with CPU-wide support we shouldn't need detailed
> error reporting.  Everything should be handled via error code
> propagation and that is what I'd like to see addressed in the first
> place.  From there we can think about individual error cases as they
> come up.

FYI, there are a lot of -EINVAL cases:

$ git grep -- -EINVAL drivers/hwtracing/coresight/ | wc -l
142

> > I personally don't agree to usage of pr_err() in paths which are easily
> > triggered from user input. Also, we are moving all the "debugging"
> > messages to the dynamic debug, to prevent lockdep splats.
> 
> A slight correction here - we are moving most of the framework error
> reporting to dynamic debug because they clog the log file and aren't
> useful outside of a development context.  It is not a remedy for the

I didn't see anyone complain about any 'clogging', and I sure don't
mind seeing the extra messaging, esp. when using coresight in sysfs
mode: it's reinforcement to the user that it's doing work.

> negative interaction between event locking and console access
> generated by the framework's reporting of device status as a path is
> built/enabled/disabled.

I'd like to see this splat from this 'negative interaction', and it be
fixed before we allow code in that makes it silently go away...

Kim