Re: [RFC 1/4] coresight: tmc-etr: Introduce CS_MODE_API and prepare the ground for it

Good day Mike,

On Tue, Sep 04, 2018 at 01:45:45PM +0300, mike@perception-point.io wrote:

From: Mike Bazov mike@perception-point.io

Introducing a new mode: CS_MODE_API. This mode shall be used by the etm-api framework to expose coresight functionality to kernel mode clients. This commit makes tmc-etr aware of it.

The set_buffer()/alloc_buffer()/free_buffer() sink operations are now turned to be mode-oriented. Up to this commit they were perf-specific, because only the perf framework used it. They are now extended to also be used with CS_MODE_API, in the hope of giving a kernel mode client the power of allocating and using a coresight buffer.

tmc-etr forbids using the sink for CS_MODE_API mode concurrently with other session modes(perf/sysfs). The implementation doesn't forbid using the sink multiple times if the requested mode is CS_MODE_API.

This commit also makes the perf framework aware of the sink operations function signature changes.

This patch is doing way too many things and needs to be broken down if we are to follow the API approach (more on that later).

Signed-off-by: Mike Bazov mike@perception-point.io

drivers/hwtracing/coresight/coresight-etb10.c | 13 ++- drivers/hwtracing/coresight/coresight-etm-perf.c | 7 +- drivers/hwtracing/coresight/coresight-priv.h | 1 + drivers/hwtracing/coresight/coresight-tmc-etf.c | 16 ++- drivers/hwtracing/coresight/coresight-tmc-etr.c | 124 +++++++++++++++++++---- drivers/hwtracing/coresight/coresight-tmc.h | 3 +- include/linux/coresight.h | 10 +- 7 files changed, 139 insertions(+), 35 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 08fa660098f8..4fbc8af7851a 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -323,12 +323,15 @@ static void etb_disable(struct coresight_device *csdev) dev_dbg(drvdata->dev, "ETB disabled\n"); } -static void *etb_alloc_buffer(struct coresight_device *csdev, int cpu, +static void *etb_alloc_buffer(struct coresight_device *csdev, u32 mode, int cpu, void **pages, int nr_pages, bool overwrite) { int node; struct cs_buffers *buf;

• if (mode != CS_MODE_PERF)

• return NULL;
  

• if (cpu == -1) cpu = smp_processor_id(); node = cpu_to_node(cpu);

@@ -344,11 +347,13 @@ static void *etb_alloc_buffer(struct coresight_device *csdev, int cpu, return buf; } -static void etb_free_buffer(void *config) +static void etb_free_buffer(u32 mode, void *config) {

• struct cs_buffers *buf = config;

• if (mode != CS_MODE_API) {

• struct cs_buffers *buf = config;
  

• kfree(buf);

• kfree(buf);
  

• }

See below on how to do this better.

} static int etb_set_buffer(struct coresight_device *csdev, diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index abe8249b893b..563f4b47e977 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -119,7 +119,8 @@ static void free_event_data(struct work_struct *work) cpu = cpumask_first(mask); sink = coresight_get_sink(etm_event_cpu_path(event_data, cpu)); if (sink_ops(sink)->free_buffer)

• 	sink_ops(sink)->free_buffer(event_data->snk_config);
  

• 	sink_ops(sink)->free_buffer(CS_MODE_PERF,
  

• 				    event_data->snk_config);
  

  }

for_each_cpu(cpu, mask) { @@ -250,7 +251,8 @@ static void *etm_setup_aux(int event_cpu, void **pages, /* Allocate the sink buffer for this session */ event_data->snk_config =

• 	sink_ops(sink)->alloc_buffer(sink, cpu, pages,
  

• 	sink_ops(sink)->alloc_buffer(sink, CS_MODE_PERF,
  

• 				     cpu, pages,
  			     nr_pages, overwrite);
  

  if (!event_data->snk_config) goto err;

@@ -284,6 +286,7 @@ static void etm_event_start(struct perf_event *event, int flags) goto fail; path = etm_event_cpu_path(event_data, cpu);

Extra cosmetic repair

/* We need a sink, no need to continue without one */ sink = coresight_get_sink(path); if (WARN_ON_ONCE(!sink)) diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index c11da5564a67..174e6f1fab3e 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -72,6 +72,7 @@ enum cs_mode { CS_MODE_DISABLED, CS_MODE_SYSFS, CS_MODE_PERF,

• CS_MODE_API

}; /** diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 4156c95ce1bb..897dfc1bcbaa 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -307,12 +307,16 @@ static void tmc_disable_etf_link(struct coresight_device *csdev, dev_dbg(drvdata->dev, "TMC-ETF disabled\n"); } -static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, int cpu,

• 		  void **pages, int nr_pages, bool overwrite)
  

+static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, u32 mode,

• 		  int cpu, void **pages, int nr_pages,
  

• 		  bool overwrite)
  

{ int node; struct cs_buffers *buf;

• if (mode == CS_MODE_API)

• return NULL;
  

• if (cpu == -1) cpu = smp_processor_id(); node = cpu_to_node(cpu);

@@ -329,11 +333,13 @@ static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, int cpu, return buf; } -static void tmc_free_etf_buffer(void *config) +static void tmc_free_etf_buffer(u32 mode, void *config) {

• struct cs_buffers *buf = config;

• if (mode != CS_MODE_API) {

• struct cs_buffers *buf = config;
  

• kfree(buf);

• kfree(buf);
  

• }

struct cs_buffers *buf = config;

if (mode == CS_MODE_API) return;

kfree(buf);

} static int tmc_set_etf_buffer(struct coresight_device *csdev, diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 56fea4ff947e..6ffcaaf76680 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1176,32 +1176,35 @@ tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages, return etr_perf; }

-static void *tmc_alloc_etr_buffer(struct coresight_device *csdev,

• 		  int cpu, void **pages, int nr_pages,
  

• 		  bool snapshot)
  

+static void *tmc_etr_alloc_api_buffer(struct tmc_drvdata *drvdata, int cpu,

• 		      int nr_pages, void **pages, bool snapshot)
  

{

• struct etr_perf_buffer *etr_perf;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

• ssize_t size = 0;

• if (cpu == -1)

• cpu = smp_processor_id();
  

• size = nr_pages << PAGE_SHIFT;

• etr_perf = tmc_etr_setup_perf_buf(drvdata, cpu_to_node(cpu),

• 			  nr_pages, pages, snapshot);
  

• if (IS_ERR(etr_perf)) {

• dev_dbg(drvdata->dev, "Unable to allocate ETR buffer\n");
  

• return NULL;
  

• }

• return tmc_alloc_etr_buf(drvdata, size, 0, cpu_to_node(0), pages);

+}

• etr_perf->snapshot = snapshot;
• etr_perf->nr_pages = nr_pages;
• etr_perf->pages = pages;

+void *tmc_alloc_etr_buffer(struct coresight_device *csdev, u32 mode,

• 	   int cpu, void **pages, int nr_pages, bool snapshot)
  

+{

• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

• return etr_perf;

• switch (mode) {
• case CS_MODE_PERF:

• return tmc_etr_setup_perf_buf(drvdata, cpu, nr_pages,
  

• 			      pages, snapshot);
  

Here an etr_perf buffer is returned but setting the snapshot, nr_pages and pages isn't done. The check for cpu == -1 and IS_ERR(etr_per) have also been clobbered.

I tried running a perf session after applying your patches and the kernel crashed on me. I'm pretty sure (one) of the problem is right here.

• case CS_MODE_API:

• return tmc_etr_alloc_api_buffer(drvdata, cpu, nr_pages,
  

• 				pages, snapshot);
  

• default:

• /* We shouldn't get here. */
  

• return ERR_PTR(-EINVAL);
  

• }

} -static void tmc_free_etr_buffer(void *config) +static void tmc_etr_free_perf_buffer(void *config) { struct etr_perf_buffer *etr_perf = config; @@ -1210,6 +1213,61 @@ static void tmc_free_etr_buffer(void *config) kfree(etr_perf); } +static void tmc_etr_free_api_buffer(void *buffer) +{

• if (buffer)

• tmc_free_etr_buf(buffer);
  

+}

+static void tmc_free_etr_buffer(u32 mode, void *buffer) +{

• switch (mode) {
• case CS_MODE_PERF:

• tmc_etr_free_perf_buffer(buffer);
  

• break;
  

• case CS_MODE_API:

• tmc_etr_free_api_buffer(buffer);
  

• default:

• break;
  

• }

+}

+static int tmc_etr_set_api_buffer(struct coresight_device *csdev, void *buffer) +{

• struct etr_buf *etr_buf = buffer;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• unsigned long flags;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• /*

• * Disable the etr if it's running. The user must pause any playing
  

• * sources if it doesn't want to lose data.
  

• */
  

• if (drvdata->mode == CS_MODE_API && drvdata->api_buf != NULL)

• tmc_etr_disable_hw(drvdata);
  

• drvdata->api_buf = etr_buf;
• if (drvdata->mode == CS_MODE_API && drvdata->api_buf != NULL)

• tmc_etr_enable_hw(drvdata, drvdata->api_buf);
  

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return 0;

+}

+static int tmc_etr_set_buffer(struct coresight_device *csdev, u32 mode,

• 	      struct perf_output_handle *handle, void *buffer)
  

+{

• switch (mode) {
• case CS_MODE_API:

• return tmc_etr_set_api_buffer(csdev, buffer);
  

• default:

• return -EINVAL;
  

• }

+}

If fail to see why the set and enable functions can't be rolled out as one.

/*

• tmc_etr_sync_perf_buffer: Copy the actual trace data from the hardware
• buffer to the perf ring buffer.

@@ -1350,6 +1408,29 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) return rc; } +static int tmc_enable_etr_sink_api(struct coresight_device *csdev) +{

• int rc = 0;
• unsigned long flags;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• spin_lock_irqsave(&drvdata->spinlock, flags);
• /*

• * If the sink is already being used by someone, we cannot interfere.
  

• */
  

• if (drvdata->mode != CS_MODE_DISABLED &&

•    drvdata->mode != CS_MODE_API) {
  

• rc = -EBUSY;
  

• goto out;
  

• }
• drvdata->mode = CS_MODE_API;

+out:

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return rc;

+}

static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode, void *data) { @@ -1358,13 +1439,15 @@ static int tmc_enable_etr_sink(struct coresight_device *csdev, return tmc_enable_etr_sink_sysfs(csdev); case CS_MODE_PERF: return tmc_enable_etr_sink_perf(csdev, data);

• case CS_MODE_API:

• return tmc_enable_etr_sink_api(csdev);
  

Why can't the sink data be communicated at enable time just like it is the case in perf? Then it would simply be a matter of dealing with the type of data that is communicated to the sink.

} /* We shouldn't be here */ return -EINVAL; } -static void tmc_disable_etr_sink(struct coresight_device *csdev) +void tmc_disable_etr_sink(struct coresight_device *csdev) { unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -1391,6 +1474,7 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = { .disable = tmc_disable_etr_sink, .alloc_buffer = tmc_alloc_etr_buffer, .update_buffer = tmc_update_etr_buffer,

• .set_buffer = tmc_etr_set_buffer, .free_buffer = tmc_free_etr_buffer,

}; diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 487c53701e9c..39c6646c82d3 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -172,6 +172,7 @@ struct etr_buf {

• device configuration register (DEVID)
  

• @perf_data: PERF buffer for ETR.
• @sysfs_data: SYSFS buffer for ETR.

• • @api_buf: API buffer for ETR.

Tabulation problem

*/ struct tmc_drvdata { void __iomem *base; @@ -193,6 +194,7 @@ struct tmc_drvdata { u32 etr_caps; struct etr_buf *sysfs_buf; void *perf_data;

• struct etr_buf *api_buf;

Here too

}; struct etr_buf_operations { @@ -257,7 +259,6 @@ extern const struct coresight_ops tmc_etr_cs_ops; ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata, loff_t pos, size_t len, char **bufpp);

Extra cosmetic repair

#define TMC_REG_PAIR(name, lo_off, hi_off) \ static inline u64 \ tmc_read_##name(struct tmc_drvdata *drvdata) \ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 53535821dc25..8f7f98570afd 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -183,16 +183,20 @@ struct coresight_device {

• Operations available for sinks
• @enable: enables the sink.
• @disable: disables the sink.

• • @alloc_buffer: initialises perf's ring buffer for trace collection.

• • @alloc_buffer: allocates the trace buffer.
  • @free_buffer: release memory allocated in @get_config.
• • @set_buffer: set the trace buffer.

Set the trace buffer for what? I wouldn't be looking for a 5 line descrition but something a little more intuitive would be appreciated.

• @update_buffer: update buffer pointers after a trace session.

*/ struct coresight_ops_sink { int (*enable)(struct coresight_device *csdev, u32 mode, void *data); void (*disable)(struct coresight_device *csdev);

• void *(*alloc_buffer)(struct coresight_device *csdev, int cpu,

• void *(*alloc_buffer)(struct coresight_device *csdev, u32 mode, int cpu, void **pages, int nr_pages, bool overwrite);

• void (*free_buffer)(void *config);

• void (*free_buffer)(u32 mode, void *buffer);
• int (*set_buffer)(struct coresight_device *csdev, u32 mode,

• 	  struct perf_output_handle *handle,
  

• 	  void *buffer);
  

  unsigned long (*update_buffer)(struct coresight_device *csdev, struct perf_output_handle *handle, void *sink_config);

-- 2.16.2