[PATCH v2 0/4] coresight: support panic dump functionality

[PATCH v2 0/4] coresight: support panic dump functionality

[Leo Yan]

### Introduction ###

Embedded Trace Buffer (ETB) provides on-chip storage of trace data,
usually has buffer size from 2KB to 8KB. These data has been used for
profiling and this has been well implemented in coresight driver.

This patch set is to explore ETB RAM data for postmortem debugging.
We could consider ETB RAM data is quite useful for postmortem debugging,
especially if the hardware design with local ETB buffer (ARM DDI 0461B)
chapter 1.2.7. 'Local ETF', with this kind design every CPU has one
dedicated ETB RAM. So it's quite handy that we can use alive CPU to help
dump the hang CPU ETB RAM. Then we can quickly get to know what's the
exact execution flow before its hang.

Due ETB RAM buffer has small size, if all CPUs shared one ETB buffer
then the trace data for causing error is easily to be overwritten by
other PEs; but even so sometimes we still have chance to go through the
trace data to assist debugging panic issues.

### Implementation ###

Firstly we need provide a unified APIs for panic dump functionality, so
it can be easily extended to enable panic dump for multiple drivers. This
is finished by patch 0001, it registers panic notifier, and provide the
general APIs {coresight_dump_add|coresight_dump_del} as helper functions
so any coresight device can add into dump list or delete itself
as needed.

Generally coresight devices can add itself into panic dump when
registration, if the coresight device wants to do dump it will set its
'panic_cb' in the ops structure. So patch 0002 is to add and delete panic
dump node for devices.

Patch 0003 and 0004 are to add panic callback functions for tmc and etm4x
drivers; so tmc dirver can save specific trace data for ETB/ETF when panic
happens, and etm4x driver can save metadata for offline analysis.

### Usage ###

Below are the example for how to use panic dump functionality on 96boards
Hikey, the brief flow is: when the panic happens the ETB panic callback
function saves trace data into memory, then relies on kdump to use
recovery kernel to save DDR content as kernel core dump file; after we
transfer kernel core dump file from board to host PC, use 'crash' tool +
extension program to extract trace data and generate 'perf' format
compatible file.

- Enable tracing on Hikey; in theory there have two methods to enable
  tracing:

  The first method is to use sysfs interface to enable coresight tracing:
  echo 1 > /sys/bus/coresight/devices/f6402000.etf/enable_sink
  echo 1 > /sys/bus/coresight/devices/f659c000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f659d000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f659e000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f659f000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f65dc000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f65dd000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f65de000.etm/enable_source
  echo 1 > /sys/bus/coresight/devices/f65df000.etm/enable_source

  The second method is to use tool 'perf' with snapshot method, this
  command is expected to enable tracing and wait for specific event happen
  and capture the snapshot trace data, this method also can be smoothly
  used for panic dump. This command currently is failure on Hikey due
  now coresight only support '--per-thread' method with perf tool:
  ./perf record --snapshot -S8196 -e cs_etm/@f6402000.etf/ -- sleep 1000 &

- Load recovery kernel for kdump:

  ARM64's kdump supports to use the same kernel image both for main
  kernel and dump-capture kernel; so we can simply to load dump-capture
  kernel with below command:
  ./kexec -p vmlinux --dtb=hi6220-hikey.dtb --append="root=/dev/mmcblk0p9
  rw  maxcpus=1 reset_devices earlycon=pl011,0xf7113000 nohlt
  initcall_debug console=tty0 console=ttyAMA3,115200 clk_ignore_unused"

- Download kernel dump file:

  After kernel panic happens, the kdump launches dump-capture kernel;
  so we need save kernel's dump file on target:
  cp /proc/vmcore ./vmcore

  Finally we can copy 'vmcore' file onto PC.

- Use 'crash' tool + csdump.so extension to extract trace data:

  After we download vmcore file from Hikey board to host PC, we can
  use 'crash' tool + csdump.so to generate 'perf.data' file:

  ./crash vmcore vmlinux
  crash> extend csdump.so
  crash> csdump output_dir

  We can see in the 'output_dir' there will generate out three files:
  output_dir/
  ??? cstrace.bin       -> trace raw data
  ??? metadata.bin      -> meta data
  ??? perf.data         -> 'perf' format compatible file

  The source code of 'csdump.so' will be sent to mailing list sepeartely.

- User 'perf' tool for offline analysis:

  On Hikey board:
  ./perf script -v -F cpu,event,ip -i perf_2.data -k vmlinux

  [001]         instructions:  ffff000008559ad0
  [001]         instructions:  ffff000008559230
  [001]         instructions:  ffff00000855924c
  [001]         instructions:  ffff000008559ae0
  [001]         instructions:  ffff000008559ad0
  [001]         instructions:  ffff000008559230
  [001]         instructions:  ffff00000855924c
  [001]         instructions:  ffff000008559ae0
  [001]         instructions:  ffff000008559ad0

Changes from v1:
* Add support to dump ETMv4 meta data.
* Wrote 'crash' extension csdump.so so rely on it to generate 'perf'
  format compatible file.
* Refactored panic dump driver to support pre & post panic dump.

Changes from RFC:
* Follow Mathieu's suggestion, use general framework to support dump
  functionality.
* Changed to use perf to analyse trace data.


Leo Yan (4):
  coresight: Support panic dump functionality
  coresight: Add and delete dump node for registration/unregistration
  coresight: tmc: Hook panic dump callback for ETB/ETF
  coresight: etm4x: Hook panic dump callback for etmv4

 drivers/hwtracing/coresight/Kconfig                |   9 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-etm4x.c      |  22 +++
 drivers/hwtracing/coresight/coresight-etm4x.h      |  15 ++
 drivers/hwtracing/coresight/coresight-panic-dump.c | 211 +++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-priv.h       |  17 ++
 drivers/hwtracing/coresight/coresight-tmc-etf.c    |  29 +++
 drivers/hwtracing/coresight/coresight.c            |   7 +
 include/linux/coresight.h                          |   7 +
 9 files changed, 318 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-panic-dump.c

-- 
2.7.4

[PATCH v2 1/4] coresight: Support panic dump functionality

[Leo Yan]

After kernel panic happens, coresight has many useful info can be used
for analysis.  For example, the trace info from ETB RAM can be used to
check the CPU execution flows before crash.  So we can save the tracing
data from sink devices, and rely on kdump to extract them from vmcore
file.

This patch is to add a simple framework to support panic dump
functionality; it registers panic notifier, and provide the general APIs
{coresight_dump_add|coresight_dump_del} as helper functions so any
coresight device can add itself into dump list or delete as needed;
these two functions can be used when coresight device registration or
unregistration.

At late initcall phase and kernel panic happens, the notifier iterates
dump list and calls callback function to dump device specific info.  The
late initcall is mainly used to dump device meta data due there have
per-CPU data so cannot wait to dump until panic happen due the panic CPU
might cannot handle IPI anymore.  The panic dump is mainly used to dump
trace data so we can get to know the execution flow before the panic
happens.  This driver provides helper function coresight_dump_update()
to update the dump buffer info.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/Kconfig                |   9 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-panic-dump.c | 211 +++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-priv.h       |  17 ++
 include/linux/coresight.h                          |   7 +
 5 files changed, 245 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-panic-dump.c

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index ef9cb3c..6745a43 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -103,4 +103,13 @@ config CORESIGHT_CPU_DEBUG
 	  properly, please refer Documentation/trace/coresight-cpu-debug.txt
 	  for detailed description and the example for usage.
 
+config CORESIGHT_PANIC_DUMP
+	bool "CoreSight Panic Dump driver"
+	depends on ARM || ARM64
+	help
+	  This driver provides panic dump functionality for CoreSight
+	  devices.  When a kernel panic happen a device supplied callback function
+	  is used to save trace data to memory. From there we rely on kdump to extract
+	  the trace data from kernel dump file.
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 5bae90ce..04dd600 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
 obj-$(CONFIG_CORESIGHT_DYNAMIC_REPLICATOR) += coresight-dynamic-replicator.o
 obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
 obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
+obj-$(CONFIG_CORESIGHT_PANIC_DUMP) += coresight-panic-dump.o
diff --git a/drivers/hwtracing/coresight/coresight-panic-dump.c b/drivers/hwtracing/coresight/coresight-panic-dump.c
new file mode 100644
index 0000000..966336a
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-panic-dump.c
@@ -0,0 +1,211 @@
+/*
+ * Copyright(C) 2017 Linaro Limited. All rights reserved.
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/coresight.h>
+#include <linux/coresight-pmu.h>
+#include <linux/cpumask.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/perf_event.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "coresight-priv.h"
+
+typedef void (*coresight_cb_t)(void *data);
+
+/**
+ * struct coresight_dump_node - Node information for dump
+ * @cpu:	The cpu this node is affined to.
+ * @type:	Dump type for pre or post panic dump.
+ * @csdev:	Handler for coresight device.
+ * @buf:	Pointer for dump buffer.
+ * @buf_size:	Length of dump buffer.
+ * @list:	Hook to the list.
+ */
+struct coresight_dump_node {
+	int cpu;
+	int type;
+	struct coresight_device *csdev;
+	char *buf;
+	unsigned int buf_size;
+	struct list_head list;
+};
+
+static DEFINE_MUTEX(coresight_dump_lock);
+static LIST_HEAD(coresight_dump_list);
+static struct notifier_block coresight_dump_nb;
+
+static coresight_cb_t
+coresight_dump_get_cb(struct coresight_device *csdev)
+{
+	coresight_cb_t cb = NULL;
+
+	switch (csdev->type) {
+	case CORESIGHT_DEV_TYPE_SINK:
+	case CORESIGHT_DEV_TYPE_LINKSINK:
+		cb = sink_ops(csdev)->panic_cb;
+		break;
+	case CORESIGHT_DEV_TYPE_SOURCE:
+		cb = source_ops(csdev)->panic_cb;
+		break;
+	case CORESIGHT_DEV_TYPE_LINK:
+		cb = link_ops(csdev)->panic_cb;
+		break;
+	default:
+		dev_info(&csdev->dev, "Unsupport panic dump\n");
+		break;
+	}
+
+	return cb;
+}
+
+/**
+ * coresight_dump - Invoke dump callbacks, this is the main function
+ * to fulfill the panic dump.  It distinguishs to two types: one is
+ * pre panic dump so it need send IPI to ask CPUs to dump CPU
+ * dedicated info, this is mainly used to dump meta data; another is
+ * post panic dump so directly invoke callback on alive CPU.
+ *
+ * @dump_type:	Dump type for pre/post panic
+ *
+ * Returns: 0 on success.
+ */
+static int coresight_dump(unsigned int dump_type)
+{
+	struct coresight_dump_node *node;
+	struct coresight_device *csdev;
+	coresight_cb_t cb;
+
+	mutex_lock(&coresight_dump_lock);
+
+	list_for_each_entry(node, &coresight_dump_list, list) {
+		csdev = node->csdev;
+
+		if (node->type != dump_type)
+			continue;
+
+		dev_info(&csdev->dev, "Invoke panic dump...\n");
+
+		cb = coresight_dump_get_cb(csdev);
+		if (node->type == CORESIGHT_DUMP_TYPE_PRE_PANIC)
+			smp_call_function_single(node->cpu, cb, csdev, 1);
+		else
+			cb(csdev);
+	}
+
+	mutex_unlock(&coresight_dump_lock);
+	return 0;
+}
+
+int coresight_dump_update(struct coresight_device *csdev, char *buf,
+			  unsigned int buf_size)
+{
+	struct coresight_dump_node *node = csdev->dump_node;
+
+	if (!node) {
+		dev_err(&csdev->dev, "Failed to update dump node.\n");
+		return -EINVAL;
+	}
+
+	node->buf = buf;
+	node->buf_size = buf_size;
+	return 0;
+}
+
+int coresight_dump_add(struct coresight_device *csdev, int cpu)
+{
+	struct coresight_dump_node *node;
+	coresight_cb_t cb;
+	unsigned int type;
+
+	/* Bail out when callback is NULL */
+	cb = coresight_dump_get_cb(csdev);
+	if (!cb)
+		return 0;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	/*
+	 * Since source devices are used to save meta data, these devices
+	 * usually are per-CPU device and after panic happen there has risk
+	 * for the panic CPU cannot handle IPI and dump log anymore; so
+	 * register these devices as PRE_PANIC type and their callback are
+	 * called at late_initcall phase.
+	 */
+	type = (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) ?
+		CORESIGHT_DUMP_TYPE_PRE_PANIC :
+		CORESIGHT_DUMP_TYPE_PANIC;
+
+	csdev->dump_node = (void *)node;
+	node->cpu = cpu;
+	node->type = type;
+	node->csdev = csdev;
+
+	mutex_lock(&coresight_dump_lock);
+	list_add_tail(&node->list, &coresight_dump_list);
+	mutex_unlock(&coresight_dump_lock);
+	return 0;
+}
+
+void coresight_dump_del(struct coresight_device *csdev)
+{
+	struct coresight_dump_node *node;
+	coresight_cb_t cb;
+
+	/* Bail out when callback is NULL */
+	cb = coresight_dump_get_cb(csdev);
+	if (!cb)
+		return;
+
+	mutex_lock(&coresight_dump_lock);
+	list_for_each_entry(node, &coresight_dump_list, list) {
+		if (node->csdev == csdev) {
+			list_del(&node->list);
+			kfree(node);
+			mutex_unlock(&coresight_dump_lock);
+			return;
+		}
+	}
+	mutex_unlock(&coresight_dump_lock);
+
+	dev_err(&csdev->dev, "Failed to find dump node.\n");
+}
+
+static int coresight_dump_notify(struct notifier_block *nb,
+				 unsigned long mode, void *_unused)
+{
+	return coresight_dump(CORESIGHT_DUMP_TYPE_PANIC);
+}
+
+static int __init coresight_dump_init(void)
+{
+	int ret;
+
+	coresight_dump_nb.notifier_call = coresight_dump_notify;
+	ret = atomic_notifier_chain_register(&panic_notifier_list,
+					     &coresight_dump_nb);
+	if (ret)
+		return ret;
+
+	return coresight_dump(CORESIGHT_DUMP_TYPE_PRE_PANIC);
+}
+late_initcall(coresight_dump_init);
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index f1d0e21d..f268dbc 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -151,4 +151,21 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
 static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
 #endif
 
+#define CORESIGHT_DUMP_TYPE_PRE_PANIC	0
+#define CORESIGHT_DUMP_TYPE_PANIC	1
+
+#ifdef CONFIG_CORESIGHT_PANIC_DUMP
+extern int coresight_dump_add(struct coresight_device *csdev, int cpu);
+extern void coresight_dump_del(struct coresight_device *csdev);
+extern int coresight_dump_update(struct coresight_device *csdev,
+				 char *buf, unsigned int buf_size);
+#else
+static inline int coresight_dump_add(struct coresight_device *csdev, int cpu)
+{ return 0; }
+static inline void coresight_dump_del(struct coresight_device *csdev)
+{ return; }
+extern int coresight_dump_update(struct coresight_device *csdev,
+				 char *buf, unsigned int buf_size);
+#endif
+
 #endif
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index d950dad..43e40fa 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -171,6 +171,7 @@ struct coresight_device {
 	bool orphan;
 	bool enable;	/* true only if configured as part of a path */
 	bool activated;	/* true only if a sink is part of a path */
+	void *dump_node;
 };
 
 #define to_coresight_device(d) container_of(d, struct coresight_device, dev)
@@ -189,6 +190,7 @@ struct coresight_device {
  * @set_buffer:		initialises buffer mechanic before a trace session.
  * @reset_buffer:	finalises buffer mechanic after a trace session.
  * @update_buffer:	update buffer pointers after a trace session.
+ * @panic_cb:		hook function for panic notifier.
  */
 struct coresight_ops_sink {
 	int (*enable)(struct coresight_device *csdev, u32 mode);
@@ -205,6 +207,7 @@ struct coresight_ops_sink {
 	void (*update_buffer)(struct coresight_device *csdev,
 			      struct perf_output_handle *handle,
 			      void *sink_config);
+	void (*panic_cb)(void *data);
 };
 
 /**
@@ -212,10 +215,12 @@ struct coresight_ops_sink {
  * Operations available for links.
  * @enable:	enables flow between iport and oport.
  * @disable:	disables flow between iport and oport.
+ * @panic_cb:	hook function for panic notifier.
  */
 struct coresight_ops_link {
 	int (*enable)(struct coresight_device *csdev, int iport, int oport);
 	void (*disable)(struct coresight_device *csdev, int iport, int oport);
+	void (*panic_cb)(void *data);
 };
 
 /**
@@ -227,6 +232,7 @@ struct coresight_ops_link {
  *		to the HW.
  * @enable:	enables tracing for a source.
  * @disable:	disables tracing for a source.
+ * @panic_cb:	hook function for panic notifier.
  */
 struct coresight_ops_source {
 	int (*cpu_id)(struct coresight_device *csdev);
@@ -235,6 +241,7 @@ struct coresight_ops_source {
 		      struct perf_event *event,  u32 mode);
 	void (*disable)(struct coresight_device *csdev,
 			struct perf_event *event);
+	void (*panic_cb)(void *data);
 };
 
 struct coresight_ops {
-- 
2.7.4

[PATCH v2 1/4] coresight: Support panic dump functionality

[Mathieu Poirier]

On Tue, Nov 21, 2017 at 11:08:41AM +0800, Leo Yan wrote:
> After kernel panic happens, coresight has many useful info can be used
> for analysis.  For example, the trace info from ETB RAM can be used to
> check the CPU execution flows before crash.  So we can save the tracing
> data from sink devices, and rely on kdump to extract them from vmcore
> file.
> 
> This patch is to add a simple framework to support panic dump
> functionality; it registers panic notifier, and provide the general APIs
> {coresight_dump_add|coresight_dump_del} as helper functions so any
> coresight device can add itself into dump list or delete as needed;
> these two functions can be used when coresight device registration or
> unregistration.
> 
> At late initcall phase and kernel panic happens, the notifier iterates
> dump list and calls callback function to dump device specific info.  The
> late initcall is mainly used to dump device meta data due there have
> per-CPU data so cannot wait to dump until panic happen due the panic CPU
> might cannot handle IPI anymore.  The panic dump is mainly used to dump
> trace data so we can get to know the execution flow before the panic
> happens.  This driver provides helper function coresight_dump_update()
> to update the dump buffer info.

Hi Leo,

> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/hwtracing/coresight/Kconfig                |   9 +
>  drivers/hwtracing/coresight/Makefile               |   1 +
>  drivers/hwtracing/coresight/coresight-panic-dump.c | 211 +++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-priv.h       |  17 ++
>  include/linux/coresight.h                          |   7 +
>  5 files changed, 245 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-panic-dump.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index ef9cb3c..6745a43 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -103,4 +103,13 @@ config CORESIGHT_CPU_DEBUG
>  	  properly, please refer Documentation/trace/coresight-cpu-debug.txt
>  	  for detailed description and the example for usage.
>  
> +config CORESIGHT_PANIC_DUMP
> +	bool "CoreSight Panic Dump driver"
> +	depends on ARM || ARM64
> +	help
> +	  This driver provides panic dump functionality for CoreSight
> +	  devices.  When a kernel panic happen a device supplied callback function
> +	  is used to save trace data to memory. From there we rely on kdump to extract
> +	  the trace data from kernel dump file.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 5bae90ce..04dd600 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>  obj-$(CONFIG_CORESIGHT_DYNAMIC_REPLICATOR) += coresight-dynamic-replicator.o
>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
>  obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> +obj-$(CONFIG_CORESIGHT_PANIC_DUMP) += coresight-panic-dump.o
> diff --git a/drivers/hwtracing/coresight/coresight-panic-dump.c b/drivers/hwtracing/coresight/coresight-panic-dump.c
> new file mode 100644
> index 0000000..966336a
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-panic-dump.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/coresight.h>
> +#include <linux/coresight-pmu.h>
> +#include <linux/cpumask.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/mm.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include "coresight-priv.h"
> +
> +typedef void (*coresight_cb_t)(void *data);
> +
> +/**
> + * struct coresight_dump_node - Node information for dump
> + * @cpu:	The cpu this node is affined to.
> + * @type:	Dump type for pre or post panic dump.
> + * @csdev:	Handler for coresight device.
> + * @buf:	Pointer for dump buffer.
> + * @buf_size:	Length of dump buffer.
> + * @list:	Hook to the list.
> + */
> +struct coresight_dump_node {
> +	int cpu;
> +	int type;
> +	struct coresight_device *csdev;
> +	char *buf;
> +	unsigned int buf_size;
> +	struct list_head list;
> +};
> +
> +static DEFINE_MUTEX(coresight_dump_lock);
> +static LIST_HEAD(coresight_dump_list);
> +static struct notifier_block coresight_dump_nb;
> +
> +static coresight_cb_t
> +coresight_dump_get_cb(struct coresight_device *csdev)
> +{
> +	coresight_cb_t cb = NULL;
> +
> +	switch (csdev->type) {
> +	case CORESIGHT_DEV_TYPE_SINK:
> +	case CORESIGHT_DEV_TYPE_LINKSINK:
> +		cb = sink_ops(csdev)->panic_cb;
> +		break;
> +	case CORESIGHT_DEV_TYPE_SOURCE:
> +		cb = source_ops(csdev)->panic_cb;
> +		break;
> +	case CORESIGHT_DEV_TYPE_LINK:
> +		cb = link_ops(csdev)->panic_cb;
> +		break;
> +	default:
> +		dev_info(&csdev->dev, "Unsupport panic dump\n");
> +		break;
> +	}
> +
> +	return cb;
> +}
> +
> +/**
> + * coresight_dump - Invoke dump callbacks, this is the main function
> + * to fulfill the panic dump.  It distinguishs to two types: one is
> + * pre panic dump so it need send IPI to ask CPUs to dump CPU
> + * dedicated info, this is mainly used to dump meta data; another is
> + * post panic dump so directly invoke callback on alive CPU.
> + *
> + * @dump_type:	Dump type for pre/post panic
> + *
> + * Returns: 0 on success.
> + */
> +static int coresight_dump(unsigned int dump_type)
> +{
> +	struct coresight_dump_node *node;
> +	struct coresight_device *csdev;
> +	coresight_cb_t cb;
> +
> +	mutex_lock(&coresight_dump_lock);
> +
> +	list_for_each_entry(node, &coresight_dump_list, list) {
> +		csdev = node->csdev;
> +
> +		if (node->type != dump_type)
> +			continue;
> +
> +		dev_info(&csdev->dev, "Invoke panic dump...\n");
> +
> +		cb = coresight_dump_get_cb(csdev);
> +		if (node->type == CORESIGHT_DUMP_TYPE_PRE_PANIC)
> +			smp_call_function_single(node->cpu, cb, csdev, 1);
> +		else
> +			cb(csdev);
> +	}
> +
> +	mutex_unlock(&coresight_dump_lock);
> +	return 0;
> +}
> +
> +int coresight_dump_update(struct coresight_device *csdev, char *buf,
> +			  unsigned int buf_size)
> +{
> +	struct coresight_dump_node *node = csdev->dump_node;
> +
> +	if (!node) {
> +		dev_err(&csdev->dev, "Failed to update dump node.\n");
> +		return -EINVAL;
> +	}
> +
> +	node->buf = buf;
> +	node->buf_size = buf_size;

How and where does this buffer get communicated to the kdump mechanic?

> +	return 0;
> +}
> +
> +int coresight_dump_add(struct coresight_device *csdev, int cpu)
> +{
> +	struct coresight_dump_node *node;
> +	coresight_cb_t cb;
> +	unsigned int type;

Filter on source or sink here, return for anything else.  That way we don't need
to clog the link structure with a callpack function that will never be used.
Doing so make sure we properly deal with the LINK_SINK type (based how I did
things in coresight.c it *may* be OK).  

> +
> +	/* Bail out when callback is NULL */
> +	cb = coresight_dump_get_cb(csdev);
> +	if (!cb)
> +		return 0;
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Since source devices are used to save meta data, these devices
> +	 * usually are per-CPU device and after panic happen there has risk
> +	 * for the panic CPU cannot handle IPI and dump log anymore; so
> +	 * register these devices as PRE_PANIC type and their callback are
> +	 * called at late_initcall phase.
> +	 */
> +	type = (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) ?
> +		CORESIGHT_DUMP_TYPE_PRE_PANIC :
> +		CORESIGHT_DUMP_TYPE_PANIC;
> +
> +	csdev->dump_node = (void *)node;
> +	node->cpu = cpu;
> +	node->type = type;
> +	node->csdev = csdev;
> +
> +	mutex_lock(&coresight_dump_lock);
> +	list_add_tail(&node->list, &coresight_dump_list);
> +	mutex_unlock(&coresight_dump_lock);
> +	return 0;
> +}
> +
> +void coresight_dump_del(struct coresight_device *csdev)
> +{
> +	struct coresight_dump_node *node;
> +	coresight_cb_t cb;
> +
> +	/* Bail out when callback is NULL */
> +	cb = coresight_dump_get_cb(csdev);
> +	if (!cb)
> +		return;
> +
> +	mutex_lock(&coresight_dump_lock);
> +	list_for_each_entry(node, &coresight_dump_list, list) {

list_for_each_entry_safe()

> +		if (node->csdev == csdev) {
> +			list_del(&node->list);
> +			kfree(node);

Just add a 'break' here and remove the error message as it serves little
purpose.

> +			mutex_unlock(&coresight_dump_lock);
> +			return;
> +		}
> +	}
> +	mutex_unlock(&coresight_dump_lock);
> +
> +	dev_err(&csdev->dev, "Failed to find dump node.\n");
> +}

Addition and delition should be done when a session start and ends, so that
only the devices involved in this session are taken into account by the dump
mechanic.  Speaking of which, would it make sense to replace all the
coresight_dump_xyz() with coresight_kdump_xyz()?

> +
> +static int coresight_dump_notify(struct notifier_block *nb,
> +				 unsigned long mode, void *_unused)
> +{
> +	return coresight_dump(CORESIGHT_DUMP_TYPE_PANIC);
> +}
> +
> +static int __init coresight_dump_init(void)
> +{
> +	int ret;
> +
> +	coresight_dump_nb.notifier_call = coresight_dump_notify;
> +	ret = atomic_notifier_chain_register(&panic_notifier_list,
> +					     &coresight_dump_nb);
> +	if (ret)
> +		return ret;
> +
> +	return coresight_dump(CORESIGHT_DUMP_TYPE_PRE_PANIC);

I'm not sure why we need to dump the information at boot time.  All this work
has to be done when a panic happens.  That would also give us the advantage of
not having to carry a 'type'.

> +}
> +late_initcall(coresight_dump_init);
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index f1d0e21d..f268dbc 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -151,4 +151,21 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
>  static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
>  #endif
>  
> +#define CORESIGHT_DUMP_TYPE_PRE_PANIC	0
> +#define CORESIGHT_DUMP_TYPE_PANIC	1
> +
> +#ifdef CONFIG_CORESIGHT_PANIC_DUMP
> +extern int coresight_dump_add(struct coresight_device *csdev, int cpu);
> +extern void coresight_dump_del(struct coresight_device *csdev);
> +extern int coresight_dump_update(struct coresight_device *csdev,
> +				 char *buf, unsigned int buf_size);
> +#else
> +static inline int coresight_dump_add(struct coresight_device *csdev, int cpu)
> +{ return 0; }

static inline int
coresight_dump_add(struct coresight_device *csdev, int cpu) { return 0; }

> +static inline void coresight_dump_del(struct coresight_device *csdev)
> +{ return; }

static inline void coresight_dump_del(struct coresight_device *csdev) {}

> +extern int coresight_dump_update(struct coresight_device *csdev,
> +				 char *buf, unsigned int buf_size);
> +#endif
> +
>  #endif
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index d950dad..43e40fa 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -171,6 +171,7 @@ struct coresight_device {
>  	bool orphan;
>  	bool enable;	/* true only if configured as part of a path */
>  	bool activated;	/* true only if a sink is part of a path */
> +	void *dump_node;
>  };
>  
>  #define to_coresight_device(d) container_of(d, struct coresight_device, dev)
> @@ -189,6 +190,7 @@ struct coresight_device {
>   * @set_buffer:		initialises buffer mechanic before a trace session.
>   * @reset_buffer:	finalises buffer mechanic after a trace session.
>   * @update_buffer:	update buffer pointers after a trace session.
> + * @panic_cb:		hook function for panic notifier.
>   */
>  struct coresight_ops_sink {
>  	int (*enable)(struct coresight_device *csdev, u32 mode);
> @@ -205,6 +207,7 @@ struct coresight_ops_sink {
>  	void (*update_buffer)(struct coresight_device *csdev,
>  			      struct perf_output_handle *handle,
>  			      void *sink_config);
> +	void (*panic_cb)(void *data);
>  };
>  
>  /**
> @@ -212,10 +215,12 @@ struct coresight_ops_sink {
>   * Operations available for links.
>   * @enable:	enables flow between iport and oport.
>   * @disable:	disables flow between iport and oport.
> + * @panic_cb:	hook function for panic notifier.
>   */
>  struct coresight_ops_link {
>  	int (*enable)(struct coresight_device *csdev, int iport, int oport);
>  	void (*disable)(struct coresight_device *csdev, int iport, int oport);
> +	void (*panic_cb)(void *data);
>  };
>  
>  /**
> @@ -227,6 +232,7 @@ struct coresight_ops_link {
>   *		to the HW.
>   * @enable:	enables tracing for a source.
>   * @disable:	disables tracing for a source.
> + * @panic_cb:	hook function for panic notifier.
>   */
>  struct coresight_ops_source {
>  	int (*cpu_id)(struct coresight_device *csdev);
> @@ -235,6 +241,7 @@ struct coresight_ops_source {
>  		      struct perf_event *event,  u32 mode);
>  	void (*disable)(struct coresight_device *csdev,
>  			struct perf_event *event);
> +	void (*panic_cb)(void *data);
>  };
>  
>  struct coresight_ops {
> -- 
> 2.7.4
> 

[PATCH v2 1/4] coresight: Support panic dump functionality

[Leo Yan]

On Wed, Nov 22, 2017 at 11:58:28AM -0700, Mathieu Poirier wrote:
> On Tue, Nov 21, 2017 at 11:08:41AM +0800, Leo Yan wrote:
> > After kernel panic happens, coresight has many useful info can be used
> > for analysis.  For example, the trace info from ETB RAM can be used to
> > check the CPU execution flows before crash.  So we can save the tracing
> > data from sink devices, and rely on kdump to extract them from vmcore
> > file.
> > 
> > This patch is to add a simple framework to support panic dump
> > functionality; it registers panic notifier, and provide the general APIs
> > {coresight_dump_add|coresight_dump_del} as helper functions so any
> > coresight device can add itself into dump list or delete as needed;
> > these two functions can be used when coresight device registration or
> > unregistration.
> > 
> > At late initcall phase and kernel panic happens, the notifier iterates
> > dump list and calls callback function to dump device specific info.  The
> > late initcall is mainly used to dump device meta data due there have
> > per-CPU data so cannot wait to dump until panic happen due the panic CPU
> > might cannot handle IPI anymore.  The panic dump is mainly used to dump
> > trace data so we can get to know the execution flow before the panic
> > happens.  This driver provides helper function coresight_dump_update()
> > to update the dump buffer info.
> 
> Hi Leo,
> 
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  drivers/hwtracing/coresight/Kconfig                |   9 +
> >  drivers/hwtracing/coresight/Makefile               |   1 +
> >  drivers/hwtracing/coresight/coresight-panic-dump.c | 211 +++++++++++++++++++++
> >  drivers/hwtracing/coresight/coresight-priv.h       |  17 ++
> >  include/linux/coresight.h                          |   7 +
> >  5 files changed, 245 insertions(+)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-panic-dump.c
> > 
> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > index ef9cb3c..6745a43 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -103,4 +103,13 @@ config CORESIGHT_CPU_DEBUG
> >  	  properly, please refer Documentation/trace/coresight-cpu-debug.txt
> >  	  for detailed description and the example for usage.
> >  
> > +config CORESIGHT_PANIC_DUMP
> > +	bool "CoreSight Panic Dump driver"
> > +	depends on ARM || ARM64
> > +	help
> > +	  This driver provides panic dump functionality for CoreSight
> > +	  devices.  When a kernel panic happen a device supplied callback function
> > +	  is used to save trace data to memory. From there we rely on kdump to extract
> > +	  the trace data from kernel dump file.
> > +
> >  endif
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index 5bae90ce..04dd600 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -17,3 +17,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> >  obj-$(CONFIG_CORESIGHT_DYNAMIC_REPLICATOR) += coresight-dynamic-replicator.o
> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> >  obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> > +obj-$(CONFIG_CORESIGHT_PANIC_DUMP) += coresight-panic-dump.o
> > diff --git a/drivers/hwtracing/coresight/coresight-panic-dump.c b/drivers/hwtracing/coresight/coresight-panic-dump.c
> > new file mode 100644
> > index 0000000..966336a
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-panic-dump.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> > + * Author: Leo Yan <leo.yan@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/coresight.h>
> > +#include <linux/coresight-pmu.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/list.h>
> > +#include <linux/mm.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include "coresight-priv.h"
> > +
> > +typedef void (*coresight_cb_t)(void *data);
> > +
> > +/**
> > + * struct coresight_dump_node - Node information for dump
> > + * @cpu:	The cpu this node is affined to.
> > + * @type:	Dump type for pre or post panic dump.
> > + * @csdev:	Handler for coresight device.
> > + * @buf:	Pointer for dump buffer.
> > + * @buf_size:	Length of dump buffer.
> > + * @list:	Hook to the list.
> > + */
> > +struct coresight_dump_node {
> > +	int cpu;
> > +	int type;
> > +	struct coresight_device *csdev;
> > +	char *buf;
> > +	unsigned int buf_size;
> > +	struct list_head list;
> > +};
> > +
> > +static DEFINE_MUTEX(coresight_dump_lock);
> > +static LIST_HEAD(coresight_dump_list);
> > +static struct notifier_block coresight_dump_nb;
> > +
> > +static coresight_cb_t
> > +coresight_dump_get_cb(struct coresight_device *csdev)
> > +{
> > +	coresight_cb_t cb = NULL;
> > +
> > +	switch (csdev->type) {
> > +	case CORESIGHT_DEV_TYPE_SINK:
> > +	case CORESIGHT_DEV_TYPE_LINKSINK:
> > +		cb = sink_ops(csdev)->panic_cb;
> > +		break;
> > +	case CORESIGHT_DEV_TYPE_SOURCE:
> > +		cb = source_ops(csdev)->panic_cb;
> > +		break;
> > +	case CORESIGHT_DEV_TYPE_LINK:
> > +		cb = link_ops(csdev)->panic_cb;
> > +		break;
> > +	default:
> > +		dev_info(&csdev->dev, "Unsupport panic dump\n");
> > +		break;
> > +	}
> > +
> > +	return cb;
> > +}
> > +
> > +/**
> > + * coresight_dump - Invoke dump callbacks, this is the main function
> > + * to fulfill the panic dump.  It distinguishs to two types: one is
> > + * pre panic dump so it need send IPI to ask CPUs to dump CPU
> > + * dedicated info, this is mainly used to dump meta data; another is
> > + * post panic dump so directly invoke callback on alive CPU.
> > + *
> > + * @dump_type:	Dump type for pre/post panic
> > + *
> > + * Returns: 0 on success.
> > + */
> > +static int coresight_dump(unsigned int dump_type)
> > +{
> > +	struct coresight_dump_node *node;
> > +	struct coresight_device *csdev;
> > +	coresight_cb_t cb;
> > +
> > +	mutex_lock(&coresight_dump_lock);
> > +
> > +	list_for_each_entry(node, &coresight_dump_list, list) {
> > +		csdev = node->csdev;
> > +
> > +		if (node->type != dump_type)
> > +			continue;
> > +
> > +		dev_info(&csdev->dev, "Invoke panic dump...\n");
> > +
> > +		cb = coresight_dump_get_cb(csdev);
> > +		if (node->type == CORESIGHT_DUMP_TYPE_PRE_PANIC)
> > +			smp_call_function_single(node->cpu, cb, csdev, 1);
> > +		else
> > +			cb(csdev);
> > +	}
> > +
> > +	mutex_unlock(&coresight_dump_lock);
> > +	return 0;
> > +}
> > +
> > +int coresight_dump_update(struct coresight_device *csdev, char *buf,
> > +			  unsigned int buf_size)
> > +{
> > +	struct coresight_dump_node *node = csdev->dump_node;
> > +
> > +	if (!node) {
> > +		dev_err(&csdev->dev, "Failed to update dump node.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	node->buf = buf;
> > +	node->buf_size = buf_size;
> 
> How and where does this buffer get communicated to the kdump mechanic?

Thanks for reviewing, Mathieu.

Kdump can firstly find list head 'coresight_dump_list' by its global
symbol address; then kdump can iterate every node to retrive buffer
pointer and buffer size.

> > +	return 0;
> > +}
> > +
> > +int coresight_dump_add(struct coresight_device *csdev, int cpu)
> > +{
> > +	struct coresight_dump_node *node;
> > +	coresight_cb_t cb;
> > +	unsigned int type;
> 
> Filter on source or sink here, return for anything else.  That way we don't need
> to clog the link structure with a callpack function that will never be used.
> Doing so make sure we properly deal with the LINK_SINK type (based how I did
> things in coresight.c it *may* be OK).  

Will do this.  For LINK_SINK type handling, I have no confidence I have
completely understood your suggestion, do you suggest code as below?

	if (type == CORESIGHT_DEV_TYPE_LINKSINK)
		type = (csdev == coresight_get_sink(path)) ?
				CORESIGHT_DEV_TYPE_SINK :
				CORESIGHT_DEV_TYPE_LINK;

> > +
> > +	/* Bail out when callback is NULL */
> > +	cb = coresight_dump_get_cb(csdev);
> > +	if (!cb)
> > +		return 0;
> > +
> > +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> > +	if (!node)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Since source devices are used to save meta data, these devices
> > +	 * usually are per-CPU device and after panic happen there has risk
> > +	 * for the panic CPU cannot handle IPI and dump log anymore; so
> > +	 * register these devices as PRE_PANIC type and their callback are
> > +	 * called at late_initcall phase.
> > +	 */
> > +	type = (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) ?
> > +		CORESIGHT_DUMP_TYPE_PRE_PANIC :
> > +		CORESIGHT_DUMP_TYPE_PANIC;
> > +
> > +	csdev->dump_node = (void *)node;
> > +	node->cpu = cpu;
> > +	node->type = type;
> > +	node->csdev = csdev;
> > +
> > +	mutex_lock(&coresight_dump_lock);
> > +	list_add_tail(&node->list, &coresight_dump_list);
> > +	mutex_unlock(&coresight_dump_lock);
> > +	return 0;
> > +}
> > +
> > +void coresight_dump_del(struct coresight_device *csdev)
> > +{
> > +	struct coresight_dump_node *node;
> > +	coresight_cb_t cb;
> > +
> > +	/* Bail out when callback is NULL */
> > +	cb = coresight_dump_get_cb(csdev);
> > +	if (!cb)
> > +		return;
> > +
> > +	mutex_lock(&coresight_dump_lock);
> > +	list_for_each_entry(node, &coresight_dump_list, list) {
> 
> list_for_each_entry_safe()

Will fix.

> > +		if (node->csdev == csdev) {
> > +			list_del(&node->list);
> > +			kfree(node);
> 
> Just add a 'break' here and remove the error message as it serves little
> purpose.

Will fix.

> > +			mutex_unlock(&coresight_dump_lock);
> > +			return;
> > +		}
> > +	}
> > +	mutex_unlock(&coresight_dump_lock);
> > +
> > +	dev_err(&csdev->dev, "Failed to find dump node.\n");
> > +}
> 
> Addition and delition should be done when a session start and ends, so that
> only the devices involved in this session are taken into account by the dump
> mechanic.

In this version patch set addition and delition are called from
coresight_register() and coresight_unregister(), and every device
panic callback can check if the device has enabled or disabled. If the
device is disabled then it can skip the panic dump.

The implementation in patch v2 is following comment from patch set v1 [1],
sorry I sent patch set v2 with very long interval from v1 and I might
misunderstand the comments from you and Suzuki.

Could you confirm which option is better? One is addition and delition
called from coresight_register() and coresight_unregister()? Another
is from session start and end?

[1] http://archive.armlinux.org.uk/lurker/message/20170608.181301.8adf6ec5.en.html

> Speaking of which, would it make sense to replace all the
> coresight_dump_xyz() with coresight_kdump_xyz()?

I am fine to replace coresight_dump_xyz() with coresight_kdump_xyz().

> > +
> > +static int coresight_dump_notify(struct notifier_block *nb,
> > +				 unsigned long mode, void *_unused)
> > +{
> > +	return coresight_dump(CORESIGHT_DUMP_TYPE_PANIC);
> > +}
> > +
> > +static int __init coresight_dump_init(void)
> > +{
> > +	int ret;
> > +
> > +	coresight_dump_nb.notifier_call = coresight_dump_notify;
> > +	ret = atomic_notifier_chain_register(&panic_notifier_list,
> > +					     &coresight_dump_nb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return coresight_dump(CORESIGHT_DUMP_TYPE_PRE_PANIC);
> 
> I'm not sure why we need to dump the information at boot time.  All this work
> has to be done when a panic happens.  That would also give us the advantage of
> not having to carry a 'type'.

Yes, but if we want to dump ETM per-CPU meta data, it's safe to dump
them before panic, otherwise the panic CPU might cannot handle IPI
when panic happens. This is main reason I add "PRE_PANIC" type.

Another benefit for "PRE_PANIC" dump is for hang case, if we want to
debug hang case with coresight dump, the "PRE_PANIC" can dump meta
data in system initilization phase and after the system hang we can
rely on system rebooting (like watchdog) + bootloader to dump trace
data (ETB/ETF), finally kdump also can easily extract them out from
'vmcore' dump file.

I saw you have another email for ETM dump patch, will reply for that
email for related discussion.

> > +}
> > +late_initcall(coresight_dump_init);
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index f1d0e21d..f268dbc 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -151,4 +151,21 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
> >  static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
> >  #endif
> >  
> > +#define CORESIGHT_DUMP_TYPE_PRE_PANIC	0
> > +#define CORESIGHT_DUMP_TYPE_PANIC	1
> > +
> > +#ifdef CONFIG_CORESIGHT_PANIC_DUMP
> > +extern int coresight_dump_add(struct coresight_device *csdev, int cpu);
> > +extern void coresight_dump_del(struct coresight_device *csdev);
> > +extern int coresight_dump_update(struct coresight_device *csdev,
> > +				 char *buf, unsigned int buf_size);
> > +#else
> > +static inline int coresight_dump_add(struct coresight_device *csdev, int cpu)
> > +{ return 0; }
> 
> static inline int
> coresight_dump_add(struct coresight_device *csdev, int cpu) { return 0; }
> 
> > +static inline void coresight_dump_del(struct coresight_device *csdev)
> > +{ return; }
> 
> static inline void coresight_dump_del(struct coresight_device *csdev) {}

Will fix them.

[...]

Thanks,
Leo Yan

[PATCH v2 1/4] coresight: Support panic dump functionality

[Mathieu Poirier]

On 23 November 2017 at 18:54, Leo Yan <leo.yan@linaro.org> wrote:
> On Wed, Nov 22, 2017 at 11:58:28AM -0700, Mathieu Poirier wrote:
>> On Tue, Nov 21, 2017 at 11:08:41AM +0800, Leo Yan wrote:
>> > After kernel panic happens, coresight has many useful info can be used
>> > for analysis.  For example, the trace info from ETB RAM can be used to
>> > check the CPU execution flows before crash.  So we can save the tracing
>> > data from sink devices, and rely on kdump to extract them from vmcore
>> > file.
>> >
>> > This patch is to add a simple framework to support panic dump
>> > functionality; it registers panic notifier, and provide the general APIs
>> > {coresight_dump_add|coresight_dump_del} as helper functions so any
>> > coresight device can add itself into dump list or delete as needed;
>> > these two functions can be used when coresight device registration or
>> > unregistration.
>> >
>> > At late initcall phase and kernel panic happens, the notifier iterates
>> > dump list and calls callback function to dump device specific info.  The
>> > late initcall is mainly used to dump device meta data due there have
>> > per-CPU data so cannot wait to dump until panic happen due the panic CPU
>> > might cannot handle IPI anymore.  The panic dump is mainly used to dump
>> > trace data so we can get to know the execution flow before the panic
>> > happens.  This driver provides helper function coresight_dump_update()
>> > to update the dump buffer info.
>>
>> Hi Leo,
>>
>> >
>> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> > ---
>> >  drivers/hwtracing/coresight/Kconfig                |   9 +
>> >  drivers/hwtracing/coresight/Makefile               |   1 +
>> >  drivers/hwtracing/coresight/coresight-panic-dump.c | 211 +++++++++++++++++++++
>> >  drivers/hwtracing/coresight/coresight-priv.h       |  17 ++
>> >  include/linux/coresight.h                          |   7 +
>> >  5 files changed, 245 insertions(+)
>> >  create mode 100644 drivers/hwtracing/coresight/coresight-panic-dump.c
>> >
>> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> > index ef9cb3c..6745a43 100644
>> > --- a/drivers/hwtracing/coresight/Kconfig
>> > +++ b/drivers/hwtracing/coresight/Kconfig
>> > @@ -103,4 +103,13 @@ config CORESIGHT_CPU_DEBUG
>> >       properly, please refer Documentation/trace/coresight-cpu-debug.txt
>> >       for detailed description and the example for usage.
>> >
>> > +config CORESIGHT_PANIC_DUMP
>> > +   bool "CoreSight Panic Dump driver"
>> > +   depends on ARM || ARM64
>> > +   help
>> > +     This driver provides panic dump functionality for CoreSight
>> > +     devices.  When a kernel panic happen a device supplied callback function
>> > +     is used to save trace data to memory. From there we rely on kdump to extract
>> > +     the trace data from kernel dump file.
>> > +
>> >  endif
>> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> > index 5bae90ce..04dd600 100644
>> > --- a/drivers/hwtracing/coresight/Makefile
>> > +++ b/drivers/hwtracing/coresight/Makefile
>> > @@ -17,3 +17,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>> >  obj-$(CONFIG_CORESIGHT_DYNAMIC_REPLICATOR) += coresight-dynamic-replicator.o
>> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
>> >  obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
>> > +obj-$(CONFIG_CORESIGHT_PANIC_DUMP) += coresight-panic-dump.o
>> > diff --git a/drivers/hwtracing/coresight/coresight-panic-dump.c b/drivers/hwtracing/coresight/coresight-panic-dump.c
>> > new file mode 100644
>> > index 0000000..966336a
>> > --- /dev/null
>> > +++ b/drivers/hwtracing/coresight/coresight-panic-dump.c
>> > @@ -0,0 +1,211 @@
>> > +/*
>> > + * Copyright(C) 2017 Linaro Limited. All rights reserved.
>> > + * Author: Leo Yan <leo.yan@linaro.org>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify it
>> > + * under the terms of the GNU General Public License version 2 as published by
>> > + * the Free Software Foundation.
>> > + *
>> > + * This program is distributed in the hope that it will be useful, but WITHOUT
>> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> > + * more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License along with
>> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> > + */
>> > +
>> > +#include <linux/coresight.h>
>> > +#include <linux/coresight-pmu.h>
>> > +#include <linux/cpumask.h>
>> > +#include <linux/device.h>
>> > +#include <linux/init.h>
>> > +#include <linux/list.h>
>> > +#include <linux/mm.h>
>> > +#include <linux/perf_event.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/types.h>
>> > +
>> > +#include "coresight-priv.h"
>> > +
>> > +typedef void (*coresight_cb_t)(void *data);
>> > +
>> > +/**
>> > + * struct coresight_dump_node - Node information for dump
>> > + * @cpu:   The cpu this node is affined to.
>> > + * @type:  Dump type for pre or post panic dump.
>> > + * @csdev: Handler for coresight device.
>> > + * @buf:   Pointer for dump buffer.
>> > + * @buf_size:      Length of dump buffer.
>> > + * @list:  Hook to the list.
>> > + */
>> > +struct coresight_dump_node {
>> > +   int cpu;
>> > +   int type;
>> > +   struct coresight_device *csdev;
>> > +   char *buf;
>> > +   unsigned int buf_size;
>> > +   struct list_head list;
>> > +};
>> > +
>> > +static DEFINE_MUTEX(coresight_dump_lock);
>> > +static LIST_HEAD(coresight_dump_list);
>> > +static struct notifier_block coresight_dump_nb;
>> > +
>> > +static coresight_cb_t
>> > +coresight_dump_get_cb(struct coresight_device *csdev)
>> > +{
>> > +   coresight_cb_t cb = NULL;
>> > +
>> > +   switch (csdev->type) {
>> > +   case CORESIGHT_DEV_TYPE_SINK:
>> > +   case CORESIGHT_DEV_TYPE_LINKSINK:
>> > +           cb = sink_ops(csdev)->panic_cb;
>> > +           break;
>> > +   case CORESIGHT_DEV_TYPE_SOURCE:
>> > +           cb = source_ops(csdev)->panic_cb;
>> > +           break;
>> > +   case CORESIGHT_DEV_TYPE_LINK:
>> > +           cb = link_ops(csdev)->panic_cb;
>> > +           break;
>> > +   default:
>> > +           dev_info(&csdev->dev, "Unsupport panic dump\n");
>> > +           break;
>> > +   }
>> > +
>> > +   return cb;
>> > +}
>> > +
>> > +/**
>> > + * coresight_dump - Invoke dump callbacks, this is the main function
>> > + * to fulfill the panic dump.  It distinguishs to two types: one is
>> > + * pre panic dump so it need send IPI to ask CPUs to dump CPU
>> > + * dedicated info, this is mainly used to dump meta data; another is
>> > + * post panic dump so directly invoke callback on alive CPU.
>> > + *
>> > + * @dump_type:     Dump type for pre/post panic
>> > + *
>> > + * Returns: 0 on success.
>> > + */
>> > +static int coresight_dump(unsigned int dump_type)
>> > +{
>> > +   struct coresight_dump_node *node;
>> > +   struct coresight_device *csdev;
>> > +   coresight_cb_t cb;
>> > +
>> > +   mutex_lock(&coresight_dump_lock);
>> > +
>> > +   list_for_each_entry(node, &coresight_dump_list, list) {
>> > +           csdev = node->csdev;
>> > +
>> > +           if (node->type != dump_type)
>> > +                   continue;
>> > +
>> > +           dev_info(&csdev->dev, "Invoke panic dump...\n");
>> > +
>> > +           cb = coresight_dump_get_cb(csdev);
>> > +           if (node->type == CORESIGHT_DUMP_TYPE_PRE_PANIC)
>> > +                   smp_call_function_single(node->cpu, cb, csdev, 1);
>> > +           else
>> > +                   cb(csdev);
>> > +   }
>> > +
>> > +   mutex_unlock(&coresight_dump_lock);
>> > +   return 0;
>> > +}
>> > +
>> > +int coresight_dump_update(struct coresight_device *csdev, char *buf,
>> > +                     unsigned int buf_size)
>> > +{
>> > +   struct coresight_dump_node *node = csdev->dump_node;
>> > +
>> > +   if (!node) {
>> > +           dev_err(&csdev->dev, "Failed to update dump node.\n");
>> > +           return -EINVAL;
>> > +   }
>> > +
>> > +   node->buf = buf;
>> > +   node->buf_size = buf_size;
>>
>> How and where does this buffer get communicated to the kdump mechanic?
>
> Thanks for reviewing, Mathieu.
>
> Kdump can firstly find list head 'coresight_dump_list' by its global
> symbol address; then kdump can iterate every node to retrive buffer
> pointer and buffer size.

I'd like to understand how the whole solution works - can you point me
to where that is done?

>
>> > +   return 0;
>> > +}
>> > +
>> > +int coresight_dump_add(struct coresight_device *csdev, int cpu)
>> > +{
>> > +   struct coresight_dump_node *node;
>> > +   coresight_cb_t cb;
>> > +   unsigned int type;
>>
>> Filter on source or sink here, return for anything else.  That way we don't need
>> to clog the link structure with a callpack function that will never be used.
>> Doing so make sure we properly deal with the LINK_SINK type (based how I did
>> things in coresight.c it *may* be OK).
>
> Will do this.  For LINK_SINK type handling, I have no confidence I have
> completely understood your suggestion, do you suggest code as below?
>
>         if (type == CORESIGHT_DEV_TYPE_LINKSINK)
>                 type = (csdev == coresight_get_sink(path)) ?
>                                 CORESIGHT_DEV_TYPE_SINK :
>                                 CORESIGHT_DEV_TYPE_LINK;


Thinking further on this we have a problem.  An ETF can be a link or a
sink based on trace scenario specifics and we don't want to have 2
different ways to deal with sinks (one for ETB and another one for
ETF).  As such I'm suggesting to do the add/remove operation in the
sink's enable/disable functions.

>
>> > +
>> > +   /* Bail out when callback is NULL */
>> > +   cb = coresight_dump_get_cb(csdev);
>> > +   if (!cb)
>> > +           return 0;
>> > +
>> > +   node = kzalloc(sizeof(*node), GFP_KERNEL);
>> > +   if (!node)
>> > +           return -ENOMEM;
>> > +
>> > +   /*
>> > +    * Since source devices are used to save meta data, these devices
>> > +    * usually are per-CPU device and after panic happen there has risk
>> > +    * for the panic CPU cannot handle IPI and dump log anymore; so
>> > +    * register these devices as PRE_PANIC type and their callback are
>> > +    * called at late_initcall phase.
>> > +    */
>> > +   type = (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) ?
>> > +           CORESIGHT_DUMP_TYPE_PRE_PANIC :
>> > +           CORESIGHT_DUMP_TYPE_PANIC;
>> > +
>> > +   csdev->dump_node = (void *)node;
>> > +   node->cpu = cpu;
>> > +   node->type = type;
>> > +   node->csdev = csdev;
>> > +
>> > +   mutex_lock(&coresight_dump_lock);
>> > +   list_add_tail(&node->list, &coresight_dump_list);
>> > +   mutex_unlock(&coresight_dump_lock);
>> > +   return 0;
>> > +}
>> > +
>> > +void coresight_dump_del(struct coresight_device *csdev)
>> > +{
>> > +   struct coresight_dump_node *node;
>> > +   coresight_cb_t cb;
>> > +
>> > +   /* Bail out when callback is NULL */
>> > +   cb = coresight_dump_get_cb(csdev);
>> > +   if (!cb)
>> > +           return;
>> > +
>> > +   mutex_lock(&coresight_dump_lock);
>> > +   list_for_each_entry(node, &coresight_dump_list, list) {
>>
>> list_for_each_entry_safe()
>
> Will fix.
>
>> > +           if (node->csdev == csdev) {
>> > +                   list_del(&node->list);
>> > +                   kfree(node);
>>
>> Just add a 'break' here and remove the error message as it serves little
>> purpose.
>
> Will fix.
>
>> > +                   mutex_unlock(&coresight_dump_lock);
>> > +                   return;
>> > +           }
>> > +   }
>> > +   mutex_unlock(&coresight_dump_lock);
>> > +
>> > +   dev_err(&csdev->dev, "Failed to find dump node.\n");
>> > +}
>>
>> Addition and delition should be done when a session start and ends, so that
>> only the devices involved in this session are taken into account by the dump
>> mechanic.
>
> In this version patch set addition and delition are called from
> coresight_register() and coresight_unregister(), and every device
> panic callback can check if the device has enabled or disabled. If the
> device is disabled then it can skip the panic dump.
>
> The implementation in patch v2 is following comment from patch set v1 [1],
> sorry I sent patch set v2 with very long interval from v1 and I might
> misunderstand the comments from you and Suzuki.

Right, but this patchset is significantly different than your previous
one, as such is it normal that comments from the previous one my not
apply fully.  But that's Ok, this is work in progress.

>
> Could you confirm which option is better? One is addition and delition
> called from coresight_register() and coresight_unregister()? Another
> is from session start and end?

Adding a tracer to the list at registration time is useless, see
explanation below.

>
> [1] http://archive.armlinux.org.uk/lurker/message/20170608.181301.8adf6ec5.en.html
>
>> Speaking of which, would it make sense to replace all the
>> coresight_dump_xyz() with coresight_kdump_xyz()?
>
> I am fine to replace coresight_dump_xyz() with coresight_kdump_xyz().
>
>> > +
>> > +static int coresight_dump_notify(struct notifier_block *nb,
>> > +                            unsigned long mode, void *_unused)
>> > +{
>> > +   return coresight_dump(CORESIGHT_DUMP_TYPE_PANIC);
>> > +}
>> > +
>> > +static int __init coresight_dump_init(void)
>> > +{
>> > +   int ret;
>> > +
>> > +   coresight_dump_nb.notifier_call = coresight_dump_notify;
>> > +   ret = atomic_notifier_chain_register(&panic_notifier_list,
>> > +                                        &coresight_dump_nb);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   return coresight_dump(CORESIGHT_DUMP_TYPE_PRE_PANIC);
>>
>> I'm not sure why we need to dump the information at boot time.  All this work
>> has to be done when a panic happens.  That would also give us the advantage of
>> not having to carry a 'type'.
>
> Yes, but if we want to dump ETM per-CPU meta data, it's safe to dump
> them before panic, otherwise the panic CPU might cannot handle IPI
> when panic happens. This is main reason I add "PRE_PANIC" type.
>
> Another benefit for "PRE_PANIC" dump is for hang case, if we want to
> debug hang case with coresight dump, the "PRE_PANIC" can dump meta
> data in system initilization phase and after the system hang we can
> rely on system rebooting (like watchdog) + bootloader to dump trace
> data (ETB/ETF), finally kdump also can easily extract them out from
> 'vmcore' dump file.

The problem is that trace data can't be decoded if the tracer
configuration (i.e metadata) isn't correct.  At boot time tracers get
a default configuration that can later be modified by users to fit
trace scenarios.  The real tracer configuration isn't known until the
session starts, hence my comment in the ETM dump patch.

>
> I saw you have another email for ETM dump patch, will reply for that
> email for related discussion.
>
>> > +}
>> > +late_initcall(coresight_dump_init);
>> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>> > index f1d0e21d..f268dbc 100644
>> > --- a/drivers/hwtracing/coresight/coresight-priv.h
>> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> > @@ -151,4 +151,21 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
>> >  static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
>> >  #endif
>> >
>> > +#define CORESIGHT_DUMP_TYPE_PRE_PANIC      0
>> > +#define CORESIGHT_DUMP_TYPE_PANIC  1
>> > +
>> > +#ifdef CONFIG_CORESIGHT_PANIC_DUMP
>> > +extern int coresight_dump_add(struct coresight_device *csdev, int cpu);
>> > +extern void coresight_dump_del(struct coresight_device *csdev);
>> > +extern int coresight_dump_update(struct coresight_device *csdev,
>> > +                            char *buf, unsigned int buf_size);
>> > +#else
>> > +static inline int coresight_dump_add(struct coresight_device *csdev, int cpu)
>> > +{ return 0; }
>>
>> static inline int
>> coresight_dump_add(struct coresight_device *csdev, int cpu) { return 0; }
>>
>> > +static inline void coresight_dump_del(struct coresight_device *csdev)
>> > +{ return; }
>>
>> static inline void coresight_dump_del(struct coresight_device *csdev) {}
>
> Will fix them.

There is a lot of variables to take into account and it is quite
possible (and normal) that we adjust the solution as things go along,
based on new things we find.  This is work in progress and I think
it's coming a long well.

>
> [...]
>
> Thanks,
> Leo Yan

[PATCH v2 1/4] coresight: Support panic dump functionality

[Leo Yan]

On Fri, Nov 24, 2017 at 09:29:24AM -0700, Mathieu Poirier wrote:

[...]

> >> > +int coresight_dump_update(struct coresight_device *csdev, char *buf,
> >> > +                     unsigned int buf_size)
> >> > +{
> >> > +   struct coresight_dump_node *node = csdev->dump_node;
> >> > +
> >> > +   if (!node) {
> >> > +           dev_err(&csdev->dev, "Failed to update dump node.\n");
> >> > +           return -EINVAL;
> >> > +   }
> >> > +
> >> > +   node->buf = buf;
> >> > +   node->buf_size = buf_size;
> >>
> >> How and where does this buffer get communicated to the kdump mechanic?
> >
> > Thanks for reviewing, Mathieu.
> >
> > Kdump can firstly find list head 'coresight_dump_list' by its global
> > symbol address; then kdump can iterate every node to retrive buffer
> > pointer and buffer size.
> 
> I'd like to understand how the whole solution works - can you point me
> to where that is done?

Sure, I have sent coresight dump for 'crash' extension [1], you could
see in function csdump_prepare() it searchs symbol
'coresight_dump_list' and then create list for dump nodes:

static int csdump_prepare(void)
{
	[...]

 	/* Get pointer to dump list */
 	sym_dump_list = symbol_search("coresight_dump_list");
 	if (!sym_dump_list) {
 		fprintf(fp, "symbol coresight_dump_list is not found\n");
 		return -1;
 	}

 	cs_dump_list_head = (void *)sym_dump_list->value;
 	fprintf(fp, "cs_dump_list_head = 0x%p\n", cs_dump_list_head);

 	BZERO(&list_data, sizeof(struct list_data));
 	list_data.start = (ulong)cs_dump_list_head;
 	list_data.end = (ulong)cs_dump_list_head;
 	list_data.flags = LIST_ALLOCATE;
 	instance_count = do_list(&list_data);

	[...]
}

> >> > +   return 0;
> >> > +}
> >> > +
> >> > +int coresight_dump_add(struct coresight_device *csdev, int cpu)
> >> > +{
> >> > +   struct coresight_dump_node *node;
> >> > +   coresight_cb_t cb;
> >> > +   unsigned int type;
> >>
> >> Filter on source or sink here, return for anything else.  That way we don't need
> >> to clog the link structure with a callpack function that will never be used.
> >> Doing so make sure we properly deal with the LINK_SINK type (based how I did
> >> things in coresight.c it *may* be OK).
> >
> > Will do this.  For LINK_SINK type handling, I have no confidence I have
> > completely understood your suggestion, do you suggest code as below?
> >
> >         if (type == CORESIGHT_DEV_TYPE_LINKSINK)
> >                 type = (csdev == coresight_get_sink(path)) ?
> >                                 CORESIGHT_DEV_TYPE_SINK :
> >                                 CORESIGHT_DEV_TYPE_LINK;
> 
> 
> Thinking further on this we have a problem.  An ETF can be a link or a
> sink based on trace scenario specifics and we don't want to have 2
> different ways to deal with sinks (one for ETB and another one for
> ETF).  As such I'm suggesting to do the add/remove operation in the
> sink's enable/disable functions.

Will do this.

Just want to check one thing: for coresight kdump, do we only need to
dump tracers & sinks and skip to dump links? I want to get clear for
the requirement, even the ETF is used as a link, should we still dump
for it?

> >> > +
> >> > +   /* Bail out when callback is NULL */
> >> > +   cb = coresight_dump_get_cb(csdev);
> >> > +   if (!cb)
> >> > +           return 0;
> >> > +
> >> > +   node = kzalloc(sizeof(*node), GFP_KERNEL);
> >> > +   if (!node)
> >> > +           return -ENOMEM;
> >> > +
> >> > +   /*
> >> > +    * Since source devices are used to save meta data, these devices
> >> > +    * usually are per-CPU device and after panic happen there has risk
> >> > +    * for the panic CPU cannot handle IPI and dump log anymore; so
> >> > +    * register these devices as PRE_PANIC type and their callback are
> >> > +    * called at late_initcall phase.
> >> > +    */
> >> > +   type = (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) ?
> >> > +           CORESIGHT_DUMP_TYPE_PRE_PANIC :
> >> > +           CORESIGHT_DUMP_TYPE_PANIC;
> >> > +
> >> > +   csdev->dump_node = (void *)node;
> >> > +   node->cpu = cpu;
> >> > +   node->type = type;
> >> > +   node->csdev = csdev;
> >> > +
> >> > +   mutex_lock(&coresight_dump_lock);
> >> > +   list_add_tail(&node->list, &coresight_dump_list);
> >> > +   mutex_unlock(&coresight_dump_lock);
> >> > +   return 0;
> >> > +}
> >> > +
> >> > +void coresight_dump_del(struct coresight_device *csdev)
> >> > +{
> >> > +   struct coresight_dump_node *node;
> >> > +   coresight_cb_t cb;
> >> > +
> >> > +   /* Bail out when callback is NULL */
> >> > +   cb = coresight_dump_get_cb(csdev);
> >> > +   if (!cb)
> >> > +           return;
> >> > +
> >> > +   mutex_lock(&coresight_dump_lock);
> >> > +   list_for_each_entry(node, &coresight_dump_list, list) {
> >>
> >> list_for_each_entry_safe()
> >
> > Will fix.
> >
> >> > +           if (node->csdev == csdev) {
> >> > +                   list_del(&node->list);
> >> > +                   kfree(node);
> >>
> >> Just add a 'break' here and remove the error message as it serves little
> >> purpose.
> >
> > Will fix.
> >
> >> > +                   mutex_unlock(&coresight_dump_lock);
> >> > +                   return;
> >> > +           }
> >> > +   }
> >> > +   mutex_unlock(&coresight_dump_lock);
> >> > +
> >> > +   dev_err(&csdev->dev, "Failed to find dump node.\n");
> >> > +}
> >>
> >> Addition and delition should be done when a session start and ends, so that
> >> only the devices involved in this session are taken into account by the dump
> >> mechanic.
> >
> > In this version patch set addition and delition are called from
> > coresight_register() and coresight_unregister(), and every device
> > panic callback can check if the device has enabled or disabled. If the
> > device is disabled then it can skip the panic dump.
> >
> > The implementation in patch v2 is following comment from patch set v1 [1],
> > sorry I sent patch set v2 with very long interval from v1 and I might
> > misunderstand the comments from you and Suzuki.
> 
> Right, but this patchset is significantly different than your previous
> one, as such is it normal that comments from the previous one my not
> apply fully.  But that's Ok, this is work in progress.

Agree, thanks for clarification.

> > Could you confirm which option is better? One is addition and delition
> > called from coresight_register() and coresight_unregister()? Another
> > is from session start and end?
> 
> Adding a tracer to the list at registration time is useless, see
> explanation below.
> 
> >
> > [1] http://archive.armlinux.org.uk/lurker/message/20170608.181301.8adf6ec5.en.html
> >
> >> Speaking of which, would it make sense to replace all the
> >> coresight_dump_xyz() with coresight_kdump_xyz()?
> >
> > I am fine to replace coresight_dump_xyz() with coresight_kdump_xyz().
> >
> >> > +
> >> > +static int coresight_dump_notify(struct notifier_block *nb,
> >> > +                            unsigned long mode, void *_unused)
> >> > +{
> >> > +   return coresight_dump(CORESIGHT_DUMP_TYPE_PANIC);
> >> > +}
> >> > +
> >> > +static int __init coresight_dump_init(void)
> >> > +{
> >> > +   int ret;
> >> > +
> >> > +   coresight_dump_nb.notifier_call = coresight_dump_notify;
> >> > +   ret = atomic_notifier_chain_register(&panic_notifier_list,
> >> > +                                        &coresight_dump_nb);
> >> > +   if (ret)
> >> > +           return ret;
> >> > +
> >> > +   return coresight_dump(CORESIGHT_DUMP_TYPE_PRE_PANIC);
> >>
> >> I'm not sure why we need to dump the information at boot time.  All this work
> >> has to be done when a panic happens.  That would also give us the advantage of
> >> not having to carry a 'type'.
> >
> > Yes, but if we want to dump ETM per-CPU meta data, it's safe to dump
> > them before panic, otherwise the panic CPU might cannot handle IPI
> > when panic happens. This is main reason I add "PRE_PANIC" type.
> >
> > Another benefit for "PRE_PANIC" dump is for hang case, if we want to
> > debug hang case with coresight dump, the "PRE_PANIC" can dump meta
> > data in system initilization phase and after the system hang we can
> > rely on system rebooting (like watchdog) + bootloader to dump trace
> > data (ETB/ETF), finally kdump also can easily extract them out from
> > 'vmcore' dump file.
> 
> The problem is that trace data can't be decoded if the tracer
> configuration (i.e metadata) isn't correct.  At boot time tracers get
> a default configuration that can later be modified by users to fit
> trace scenarios.  The real tracer configuration isn't known until the
> session starts, hence my comment in the ETM dump patch.

Thanks for detailed guidance for this point.

I have read your suggestions fro ETM dump patch in another email, now
it's more clear for me. I will follow the suggestion for next version
patch.

> > I saw you have another email for ETM dump patch, will reply for that
> > email for related discussion.
> >
> >> > +}
> >> > +late_initcall(coresight_dump_init);
> >> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> >> > index f1d0e21d..f268dbc 100644
> >> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> >> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> >> > @@ -151,4 +151,21 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
> >> >  static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
> >> >  #endif
> >> >
> >> > +#define CORESIGHT_DUMP_TYPE_PRE_PANIC      0
> >> > +#define CORESIGHT_DUMP_TYPE_PANIC  1
> >> > +
> >> > +#ifdef CONFIG_CORESIGHT_PANIC_DUMP
> >> > +extern int coresight_dump_add(struct coresight_device *csdev, int cpu);
> >> > +extern void coresight_dump_del(struct coresight_device *csdev);
> >> > +extern int coresight_dump_update(struct coresight_device *csdev,
> >> > +                            char *buf, unsigned int buf_size);
> >> > +#else
> >> > +static inline int coresight_dump_add(struct coresight_device *csdev, int cpu)
> >> > +{ return 0; }
> >>
> >> static inline int
> >> coresight_dump_add(struct coresight_device *csdev, int cpu) { return 0; }
> >>
> >> > +static inline void coresight_dump_del(struct coresight_device *csdev)
> >> > +{ return; }
> >>
> >> static inline void coresight_dump_del(struct coresight_device *csdev) {}
> >
> > Will fix them.
> 
> There is a lot of variables to take into account and it is quite
> possible (and normal) that we adjust the solution as things go along,
> based on new things we find.  This is work in progress and I think
> it's coming a long well.

Good to know this ahead.  Will mature the patch set step by step
according to comments and suggestions.

Thanks,
Leo Yan

[PATCH v2 1/4] coresight: Support panic dump functionality

[Mathieu Poirier]

On 26 November 2017 at 18:57, Leo Yan <leo.yan@linaro.org> wrote:
> On Fri, Nov 24, 2017 at 09:29:24AM -0700, Mathieu Poirier wrote:
>
> [...]
>
>> >> > +int coresight_dump_update(struct coresight_device *csdev, char *buf,
>> >> > +                     unsigned int buf_size)
>> >> > +{
>> >> > +   struct coresight_dump_node *node = csdev->dump_node;
>> >> > +
>> >> > +   if (!node) {
>> >> > +           dev_err(&csdev->dev, "Failed to update dump node.\n");
>> >> > +           return -EINVAL;
>> >> > +   }
>> >> > +
>> >> > +   node->buf = buf;
>> >> > +   node->buf_size = buf_size;
>> >>
>> >> How and where does this buffer get communicated to the kdump mechanic?
>> >
>> > Thanks for reviewing, Mathieu.
>> >
>> > Kdump can firstly find list head 'coresight_dump_list' by its global
>> > symbol address; then kdump can iterate every node to retrive buffer
>> > pointer and buffer size.
>>
>> I'd like to understand how the whole solution works - can you point me
>> to where that is done?
>
> Sure, I have sent coresight dump for 'crash' extension [1], you could
> see in function csdump_prepare() it searchs symbol
> 'coresight_dump_list' and then create list for dump nodes:
>
> static int csdump_prepare(void)
> {
>         [...]
>
>         /* Get pointer to dump list */
>         sym_dump_list = symbol_search("coresight_dump_list");
>         if (!sym_dump_list) {
>                 fprintf(fp, "symbol coresight_dump_list is not found\n");
>                 return -1;
>         }
>
>         cs_dump_list_head = (void *)sym_dump_list->value;
>         fprintf(fp, "cs_dump_list_head = 0x%p\n", cs_dump_list_head);
>
>         BZERO(&list_data, sizeof(struct list_data));
>         list_data.start = (ulong)cs_dump_list_head;
>         list_data.end = (ulong)cs_dump_list_head;
>         list_data.flags = LIST_ALLOCATE;
>         instance_count = do_list(&list_data);
>
>         [...]
> }

Thanks for pointing that out - I haven't looked at the user space yet.

>
>> >> > +   return 0;
>> >> > +}
>> >> > +
>> >> > +int coresight_dump_add(struct coresight_device *csdev, int cpu)
>> >> > +{
>> >> > +   struct coresight_dump_node *node;
>> >> > +   coresight_cb_t cb;
>> >> > +   unsigned int type;
>> >>
>> >> Filter on source or sink here, return for anything else.  That way we don't need
>> >> to clog the link structure with a callpack function that will never be used.
>> >> Doing so make sure we properly deal with the LINK_SINK type (based how I did
>> >> things in coresight.c it *may* be OK).
>> >
>> > Will do this.  For LINK_SINK type handling, I have no confidence I have
>> > completely understood your suggestion, do you suggest code as below?
>> >
>> >         if (type == CORESIGHT_DEV_TYPE_LINKSINK)
>> >                 type = (csdev == coresight_get_sink(path)) ?
>> >                                 CORESIGHT_DEV_TYPE_SINK :
>> >                                 CORESIGHT_DEV_TYPE_LINK;
>>
>>
>> Thinking further on this we have a problem.  An ETF can be a link or a
>> sink based on trace scenario specifics and we don't want to have 2
>> different ways to deal with sinks (one for ETB and another one for
>> ETF).  As such I'm suggesting to do the add/remove operation in the
>> sink's enable/disable functions.
>
> Will do this.
>
> Just want to check one thing: for coresight kdump, do we only need to
> dump tracers & sinks and skip to dump links? I want to get clear for
> the requirement, even the ETF is used as a link, should we still dump
> for it?
>

There isn't any useful information conveyed by the links.  If ETF is
used as a sink then it needs to be dump'ed (otherwise were will the
trace data come from).  Nothing needs to be done if used as a link.

>> >> > +
>> >> > +   /* Bail out when callback is NULL */
>> >> > +   cb = coresight_dump_get_cb(csdev);
>> >> > +   if (!cb)
>> >> > +           return 0;
>> >> > +
>> >> > +   node = kzalloc(sizeof(*node), GFP_KERNEL);
>> >> > +   if (!node)
>> >> > +           return -ENOMEM;
>> >> > +
>> >> > +   /*
>> >> > +    * Since source devices are used to save meta data, these devices
>> >> > +    * usually are per-CPU device and after panic happen there has risk
>> >> > +    * for the panic CPU cannot handle IPI and dump log anymore; so
>> >> > +    * register these devices as PRE_PANIC type and their callback are
>> >> > +    * called at late_initcall phase.
>> >> > +    */
>> >> > +   type = (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) ?
>> >> > +           CORESIGHT_DUMP_TYPE_PRE_PANIC :
>> >> > +           CORESIGHT_DUMP_TYPE_PANIC;
>> >> > +
>> >> > +   csdev->dump_node = (void *)node;
>> >> > +   node->cpu = cpu;
>> >> > +   node->type = type;
>> >> > +   node->csdev = csdev;
>> >> > +
>> >> > +   mutex_lock(&coresight_dump_lock);
>> >> > +   list_add_tail(&node->list, &coresight_dump_list);
>> >> > +   mutex_unlock(&coresight_dump_lock);
>> >> > +   return 0;
>> >> > +}
>> >> > +
>> >> > +void coresight_dump_del(struct coresight_device *csdev)
>> >> > +{
>> >> > +   struct coresight_dump_node *node;
>> >> > +   coresight_cb_t cb;
>> >> > +
>> >> > +   /* Bail out when callback is NULL */
>> >> > +   cb = coresight_dump_get_cb(csdev);
>> >> > +   if (!cb)
>> >> > +           return;
>> >> > +
>> >> > +   mutex_lock(&coresight_dump_lock);
>> >> > +   list_for_each_entry(node, &coresight_dump_list, list) {
>> >>
>> >> list_for_each_entry_safe()
>> >
>> > Will fix.
>> >
>> >> > +           if (node->csdev == csdev) {
>> >> > +                   list_del(&node->list);
>> >> > +                   kfree(node);
>> >>
>> >> Just add a 'break' here and remove the error message as it serves little
>> >> purpose.
>> >
>> > Will fix.
>> >
>> >> > +                   mutex_unlock(&coresight_dump_lock);
>> >> > +                   return;
>> >> > +           }
>> >> > +   }
>> >> > +   mutex_unlock(&coresight_dump_lock);
>> >> > +
>> >> > +   dev_err(&csdev->dev, "Failed to find dump node.\n");
>> >> > +}
>> >>
>> >> Addition and delition should be done when a session start and ends, so that
>> >> only the devices involved in this session are taken into account by the dump
>> >> mechanic.
>> >
>> > In this version patch set addition and delition are called from
>> > coresight_register() and coresight_unregister(), and every device
>> > panic callback can check if the device has enabled or disabled. If the
>> > device is disabled then it can skip the panic dump.
>> >
>> > The implementation in patch v2 is following comment from patch set v1 [1],
>> > sorry I sent patch set v2 with very long interval from v1 and I might
>> > misunderstand the comments from you and Suzuki.
>>
>> Right, but this patchset is significantly different than your previous
>> one, as such is it normal that comments from the previous one my not
>> apply fully.  But that's Ok, this is work in progress.
>
> Agree, thanks for clarification.
>
>> > Could you confirm which option is better? One is addition and delition
>> > called from coresight_register() and coresight_unregister()? Another
>> > is from session start and end?
>>
>> Adding a tracer to the list at registration time is useless, see
>> explanation below.
>>
>> >
>> > [1] http://archive.armlinux.org.uk/lurker/message/20170608.181301.8adf6ec5.en.html
>> >
>> >> Speaking of which, would it make sense to replace all the
>> >> coresight_dump_xyz() with coresight_kdump_xyz()?
>> >
>> > I am fine to replace coresight_dump_xyz() with coresight_kdump_xyz().
>> >
>> >> > +
>> >> > +static int coresight_dump_notify(struct notifier_block *nb,
>> >> > +                            unsigned long mode, void *_unused)
>> >> > +{
>> >> > +   return coresight_dump(CORESIGHT_DUMP_TYPE_PANIC);
>> >> > +}
>> >> > +
>> >> > +static int __init coresight_dump_init(void)
>> >> > +{
>> >> > +   int ret;
>> >> > +
>> >> > +   coresight_dump_nb.notifier_call = coresight_dump_notify;
>> >> > +   ret = atomic_notifier_chain_register(&panic_notifier_list,
>> >> > +                                        &coresight_dump_nb);
>> >> > +   if (ret)
>> >> > +           return ret;
>> >> > +
>> >> > +   return coresight_dump(CORESIGHT_DUMP_TYPE_PRE_PANIC);
>> >>
>> >> I'm not sure why we need to dump the information at boot time.  All this work
>> >> has to be done when a panic happens.  That would also give us the advantage of
>> >> not having to carry a 'type'.
>> >
>> > Yes, but if we want to dump ETM per-CPU meta data, it's safe to dump
>> > them before panic, otherwise the panic CPU might cannot handle IPI
>> > when panic happens. This is main reason I add "PRE_PANIC" type.
>> >
>> > Another benefit for "PRE_PANIC" dump is for hang case, if we want to
>> > debug hang case with coresight dump, the "PRE_PANIC" can dump meta
>> > data in system initilization phase and after the system hang we can
>> > rely on system rebooting (like watchdog) + bootloader to dump trace
>> > data (ETB/ETF), finally kdump also can easily extract them out from
>> > 'vmcore' dump file.
>>
>> The problem is that trace data can't be decoded if the tracer
>> configuration (i.e metadata) isn't correct.  At boot time tracers get
>> a default configuration that can later be modified by users to fit
>> trace scenarios.  The real tracer configuration isn't known until the
>> session starts, hence my comment in the ETM dump patch.
>
> Thanks for detailed guidance for this point.
>
> I have read your suggestions fro ETM dump patch in another email, now
> it's more clear for me. I will follow the suggestion for next version
> patch.
>
>> > I saw you have another email for ETM dump patch, will reply for that
>> > email for related discussion.
>> >
>> >> > +}
>> >> > +late_initcall(coresight_dump_init);
>> >> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>> >> > index f1d0e21d..f268dbc 100644
>> >> > --- a/drivers/hwtracing/coresight/coresight-priv.h
>> >> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> >> > @@ -151,4 +151,21 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
>> >> >  static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
>> >> >  #endif
>> >> >
>> >> > +#define CORESIGHT_DUMP_TYPE_PRE_PANIC      0
>> >> > +#define CORESIGHT_DUMP_TYPE_PANIC  1
>> >> > +
>> >> > +#ifdef CONFIG_CORESIGHT_PANIC_DUMP
>> >> > +extern int coresight_dump_add(struct coresight_device *csdev, int cpu);
>> >> > +extern void coresight_dump_del(struct coresight_device *csdev);
>> >> > +extern int coresight_dump_update(struct coresight_device *csdev,
>> >> > +                            char *buf, unsigned int buf_size);
>> >> > +#else
>> >> > +static inline int coresight_dump_add(struct coresight_device *csdev, int cpu)
>> >> > +{ return 0; }
>> >>
>> >> static inline int
>> >> coresight_dump_add(struct coresight_device *csdev, int cpu) { return 0; }
>> >>
>> >> > +static inline void coresight_dump_del(struct coresight_device *csdev)
>> >> > +{ return; }
>> >>
>> >> static inline void coresight_dump_del(struct coresight_device *csdev) {}
>> >
>> > Will fix them.
>>
>> There is a lot of variables to take into account and it is quite
>> possible (and normal) that we adjust the solution as things go along,
>> based on new things we find.  This is work in progress and I think
>> it's coming a long well.
>
> Good to know this ahead.  Will mature the patch set step by step
> according to comments and suggestions.
>
> Thanks,
> Leo Yan

[PATCH v2 2/4] coresight: Add and delete dump node for registration/unregistration

[Leo Yan]

Coresight device registration is a proper time point to insert dump
node, the dump code utilizes the coresight device has panic callback
function pointer as flag to indicate if need insert panic node or not.
If the coresight device has panic callback function isn't NULL, then
allocates dump node and inserts node into list, otherwise directly
bail out with success.

When coresight device unregistration, it removes dump node from list
and releases resource properly.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index b8091be..3248f9b 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -1044,6 +1044,10 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	if (ret)
 		goto err_device_register;
 
+	ret = coresight_dump_add(csdev, desc->pdata ? desc->pdata->cpu : 0);
+	if (ret)
+		goto err_dump_add;
+
 	mutex_lock(&coresight_mutex);
 
 	coresight_fixup_device_conns(csdev);
@@ -1053,6 +1057,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 
 	return csdev;
 
+err_dump_add:
+	device_unregister(&csdev->dev);
 err_device_register:
 	kfree(conns);
 err_kzalloc_conns:
@@ -1068,6 +1074,7 @@ void coresight_unregister(struct coresight_device *csdev)
 {
 	/* Remove references of that device in the topology */
 	coresight_remove_conns(csdev);
+	coresight_dump_del(csdev);
 	device_unregister(&csdev->dev);
 }
 EXPORT_SYMBOL_GPL(coresight_unregister);
-- 
2.7.4

[PATCH v2 3/4] coresight: tmc: Hook panic dump callback for ETB/ETF

[Leo Yan]

The panic dump functionality has been ready, this patch is to hook
panic callback function for ETB/ETF.  Since the driver data structure
has allocated buffer when the session started, so simply save ETB/ETF
trace data into the buffer when panic happens and update related info
into dump node.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 29 +++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index e2513b7..1337c02 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -504,6 +504,34 @@ static void tmc_update_etf_buffer(struct coresight_device *csdev,
 	CS_LOCK(drvdata->base);
 }
 
+static void tmc_panic_cb(void *data)
+{
+	struct coresight_device *csdev = (struct coresight_device *)data;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	unsigned long flags;
+
+	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
+			 drvdata->config_type != TMC_CONFIG_TYPE_ETF))
+		return;
+
+	if (drvdata->mode == CS_MODE_DISABLED)
+		return;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	CS_UNLOCK(drvdata->base);
+
+	tmc_flush_and_stop(drvdata);
+	tmc_etb_dump_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+
+	/* Update buffer info for panic dump */
+	coresight_dump_update(csdev, drvdata->buf, drvdata->len);
+
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+}
+
 static const struct coresight_ops_sink tmc_etf_sink_ops = {
 	.enable		= tmc_enable_etf_sink,
 	.disable	= tmc_disable_etf_sink,
@@ -512,6 +540,7 @@ static const struct coresight_ops_sink tmc_etf_sink_ops = {
 	.set_buffer	= tmc_set_etf_buffer,
 	.reset_buffer	= tmc_reset_etf_buffer,
 	.update_buffer	= tmc_update_etf_buffer,
+	.panic_cb	= tmc_panic_cb,
 };
 
 static const struct coresight_ops_link tmc_etf_link_ops = {
-- 
2.7.4

[PATCH v2 4/4] coresight: etm4x: Hook panic dump callback for etmv4

[Leo Yan]

We need to dump etmv4 info as metadata, so the data can be used for
offline checking for configuration and generate 'perf' compatible
file.  This commit hooks etmv4 callback for panic dump.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 22 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index cf364a5..bf9608b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -418,11 +418,33 @@ static void etm4_disable(struct coresight_device *csdev,
 		local_set(&drvdata->mode, CS_MODE_DISABLED);
 }
 
+static void etm4_panic_cb(void *data)
+{
+	struct coresight_device *csdev = (struct coresight_device *)data;
+	int cpu = smp_processor_id();
+	struct etmv4_drvdata *drvdata = etmdrvdata[cpu];
+	struct etmv4_config *config = &drvdata->config;
+	struct etmv4_metadata *metadata = &drvdata->metadata;
+
+	metadata->magic = ETM4_METADATA_MAGIC;
+	metadata->cpu = cpu;
+	metadata->trcconfigr = config->cfg;
+	metadata->trctraceidr = drvdata->trcid;
+	metadata->trcidr0 = readl_relaxed(drvdata->base + TRCIDR0);
+	metadata->trcidr1 = readl_relaxed(drvdata->base + TRCIDR1);
+	metadata->trcidr2 = readl_relaxed(drvdata->base + TRCIDR2);
+	metadata->trcidr8 = readl_relaxed(drvdata->base + TRCIDR8);
+	metadata->trcauthstatus = readl_relaxed(drvdata->base + TRCAUTHSTATUS);
+
+	coresight_dump_update(csdev, (char *)metadata, sizeof(*metadata));
+}
+
 static const struct coresight_ops_source etm4_source_ops = {
 	.cpu_id		= etm4_cpu_id,
 	.trace_id	= etm4_trace_id,
 	.enable		= etm4_enable,
 	.disable	= etm4_disable,
+	.panic_cb	= etm4_panic_cb,
 };
 
 static const struct coresight_ops etm4_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index b3b5ea7..08dc8b7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -198,6 +198,20 @@
 #define ETM_EXLEVEL_NS_HYP		BIT(14)
 #define ETM_EXLEVEL_NS_NA		BIT(15)
 
+#define ETM4_METADATA_MAGIC		0x4040404040404040ULL
+
+struct etmv4_metadata {
+	u64 magic;
+	u64 cpu;
+	u64 trcconfigr;
+	u64 trctraceidr;
+	u64 trcidr0;
+	u64 trcidr1;
+	u64 trcidr2;
+	u64 trcidr8;
+	u64 trcauthstatus;
+};
+
 /**
  * struct etmv4_config - configuration information related to an ETMv4
  * @mode:	Controls various modes supported by this ETM.
@@ -393,6 +407,7 @@ struct etmv4_drvdata {
 	bool				atbtrig;
 	bool				lpoverride;
 	struct etmv4_config		config;
+	struct etmv4_metadata		metadata;
 };
 
 /* Address comparator access types */
-- 
2.7.4

[PATCH v2 4/4] coresight: etm4x: Hook panic dump callback for etmv4

[Mathieu Poirier]

On Tue, Nov 21, 2017 at 11:08:44AM +0800, Leo Yan wrote:
> We need to dump etmv4 info as metadata, so the data can be used for
> offline checking for configuration and generate 'perf' compatible
> file.  This commit hooks etmv4 callback for panic dump.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 22 ++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index cf364a5..bf9608b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -418,11 +418,33 @@ static void etm4_disable(struct coresight_device *csdev,
>  		local_set(&drvdata->mode, CS_MODE_DISABLED);
>  }
>  
> +static void etm4_panic_cb(void *data)
> +{
> +	struct coresight_device *csdev = (struct coresight_device *)data;
> +	int cpu = smp_processor_id();
> +	struct etmv4_drvdata *drvdata = etmdrvdata[cpu];
> +	struct etmv4_config *config = &drvdata->config;
> +	struct etmv4_metadata *metadata = &drvdata->metadata;
> +
> +	metadata->magic = ETM4_METADATA_MAGIC;
> +	metadata->cpu = cpu;
> +	metadata->trcconfigr = config->cfg;
> +	metadata->trctraceidr = drvdata->trcid;
> +	metadata->trcidr0 = readl_relaxed(drvdata->base + TRCIDR0);
> +	metadata->trcidr1 = readl_relaxed(drvdata->base + TRCIDR1);
> +	metadata->trcidr2 = readl_relaxed(drvdata->base + TRCIDR2);
> +	metadata->trcidr8 = readl_relaxed(drvdata->base + TRCIDR8);

I'm pretty sure this won't work on all architecture.  This tracer may not be
powered when this is called and depending on the power domain the TRCIDRx
registers are in it may lock.

Simply read those at boot time as part of the _probe() function.

> +	metadata->trcauthstatus = readl_relaxed(drvdata->base + TRCAUTHSTATUS);
> +
> +	coresight_dump_update(csdev, (char *)metadata, sizeof(*metadata));
> +}
> +
>  static const struct coresight_ops_source etm4_source_ops = {
>  	.cpu_id		= etm4_cpu_id,
>  	.trace_id	= etm4_trace_id,
>  	.enable		= etm4_enable,
>  	.disable	= etm4_disable,
> +	.panic_cb	= etm4_panic_cb,
>  };
>  
>  static const struct coresight_ops etm4_cs_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index b3b5ea7..08dc8b7 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -198,6 +198,20 @@
>  #define ETM_EXLEVEL_NS_HYP		BIT(14)
>  #define ETM_EXLEVEL_NS_NA		BIT(15)
>  
> +#define ETM4_METADATA_MAGIC		0x4040404040404040ULL
> +
> +struct etmv4_metadata {
> +	u64 magic;
> +	u64 cpu;
> +	u64 trcconfigr;
> +	u64 trctraceidr;
> +	u64 trcidr0;
> +	u64 trcidr1;
> +	u64 trcidr2;
> +	u64 trcidr8;
> +	u64 trcauthstatus;
> +};
> +
>  /**
>   * struct etmv4_config - configuration information related to an ETMv4
>   * @mode:	Controls various modes supported by this ETM.
> @@ -393,6 +407,7 @@ struct etmv4_drvdata {
>  	bool				atbtrig;
>  	bool				lpoverride;
>  	struct etmv4_config		config;
> +	struct etmv4_metadata		metadata;
>  };
>  
>  /* Address comparator access types */
> -- 
> 2.7.4
> 

[PATCH v2 4/4] coresight: etm4x: Hook panic dump callback for etmv4

[Mathieu Poirier]

On 22 November 2017 at 12:09, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On Tue, Nov 21, 2017 at 11:08:44AM +0800, Leo Yan wrote:
>> We need to dump etmv4 info as metadata, so the data can be used for
>> offline checking for configuration and generate 'perf' compatible
>> file.  This commit hooks etmv4 callback for panic dump.
>>
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> ---
>>  drivers/hwtracing/coresight/coresight-etm4x.c | 22 ++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++++++++++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index cf364a5..bf9608b 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -418,11 +418,33 @@ static void etm4_disable(struct coresight_device *csdev,
>>               local_set(&drvdata->mode, CS_MODE_DISABLED);
>>  }
>>
>> +static void etm4_panic_cb(void *data)
>> +{
>> +     struct coresight_device *csdev = (struct coresight_device *)data;
>> +     int cpu = smp_processor_id();
>> +     struct etmv4_drvdata *drvdata = etmdrvdata[cpu];
>> +     struct etmv4_config *config = &drvdata->config;
>> +     struct etmv4_metadata *metadata = &drvdata->metadata;
>> +
>> +     metadata->magic = ETM4_METADATA_MAGIC;
>> +     metadata->cpu = cpu;
>> +     metadata->trcconfigr = config->cfg;
>> +     metadata->trctraceidr = drvdata->trcid;
>> +     metadata->trcidr0 = readl_relaxed(drvdata->base + TRCIDR0);
>> +     metadata->trcidr1 = readl_relaxed(drvdata->base + TRCIDR1);
>> +     metadata->trcidr2 = readl_relaxed(drvdata->base + TRCIDR2);
>> +     metadata->trcidr8 = readl_relaxed(drvdata->base + TRCIDR8);
>
> I'm pretty sure this won't work on all architecture.  This tracer may not be
> powered when this is called and depending on the power domain the TRCIDRx
> registers are in it may lock.
>
> Simply read those at boot time as part of the _probe() function.

Thinking about this will work when operating from sysFS but
theoretically inaccurate from perf.  Say we have a 4 CPU system where
for a perf session only 2 CPUs were used for trace acquisition - the
process was simply not scheduled on the other 2 CPU.  With the above
code 2 CPUs (the ones that were actually used) will have the right
configuration while the other two will have the default tracer
configuration (since etm4_parse_event_config() was never called on
them).

Values in TRCIDR{0, 1, 2, 8} and TRCAUTHSTATUS is RO and won't change.
So those can be filled at _probe() time when tracers are instantiated.
Values for trcconfigr ang trctraceidr are trickier.

When operating from sysFS tracer configuration should be recorded in
etm4_enable_sysfs(), probably just before smp_call_function_single().
When operating from perf adding all tracers to the dump list should be
done in etm_setup_aux() and an update for the configuration in
etm4_enable_perf(), just before etm4_enable_hw().  When circling
through tracers to communicate the metadata to the kdump mechanic
check if buf != NULL, what way we only communicate information about
tracers that were used.

Removal of the tracers from the dump list should be done in etm_free_aux()

This will need testing :o)

Mathieu

>
>> +     metadata->trcauthstatus = readl_relaxed(drvdata->base + TRCAUTHSTATUS);
>> +
>> +     coresight_dump_update(csdev, (char *)metadata, sizeof(*metadata));
>> +}
>> +
>>  static const struct coresight_ops_source etm4_source_ops = {
>>       .cpu_id         = etm4_cpu_id,
>>       .trace_id       = etm4_trace_id,
>>       .enable         = etm4_enable,
>>       .disable        = etm4_disable,
>> +     .panic_cb       = etm4_panic_cb,
>>  };
>>
>>  static const struct coresight_ops etm4_cs_ops = {
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index b3b5ea7..08dc8b7 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -198,6 +198,20 @@
>>  #define ETM_EXLEVEL_NS_HYP           BIT(14)
>>  #define ETM_EXLEVEL_NS_NA            BIT(15)
>>
>> +#define ETM4_METADATA_MAGIC          0x4040404040404040ULL
>> +
>> +struct etmv4_metadata {
>> +     u64 magic;
>> +     u64 cpu;
>> +     u64 trcconfigr;
>> +     u64 trctraceidr;
>> +     u64 trcidr0;
>> +     u64 trcidr1;
>> +     u64 trcidr2;
>> +     u64 trcidr8;
>> +     u64 trcauthstatus;
>> +};
>> +
>>  /**
>>   * struct etmv4_config - configuration information related to an ETMv4
>>   * @mode:    Controls various modes supported by this ETM.
>> @@ -393,6 +407,7 @@ struct etmv4_drvdata {
>>       bool                            atbtrig;
>>       bool                            lpoverride;
>>       struct etmv4_config             config;
>> +     struct etmv4_metadata           metadata;
>>  };
>>
>>  /* Address comparator access types */
>> --
>> 2.7.4
>>

[PATCH v2 4/4] coresight: etm4x: Hook panic dump callback for etmv4

[Leo Yan]

On Thu, Nov 23, 2017 at 10:03:33AM -0700, Mathieu Poirier wrote:
> On 22 November 2017 at 12:09, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> > On Tue, Nov 21, 2017 at 11:08:44AM +0800, Leo Yan wrote:
> >> We need to dump etmv4 info as metadata, so the data can be used for
> >> offline checking for configuration and generate 'perf' compatible
> >> file.  This commit hooks etmv4 callback for panic dump.
> >>
> >> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >> ---
> >>  drivers/hwtracing/coresight/coresight-etm4x.c | 22 ++++++++++++++++++++++
> >>  drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++++++++++
> >>  2 files changed, 37 insertions(+)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> >> index cf364a5..bf9608b 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> >> @@ -418,11 +418,33 @@ static void etm4_disable(struct coresight_device *csdev,
> >>               local_set(&drvdata->mode, CS_MODE_DISABLED);
> >>  }
> >>
> >> +static void etm4_panic_cb(void *data)
> >> +{
> >> +     struct coresight_device *csdev = (struct coresight_device *)data;
> >> +     int cpu = smp_processor_id();
> >> +     struct etmv4_drvdata *drvdata = etmdrvdata[cpu];
> >> +     struct etmv4_config *config = &drvdata->config;
> >> +     struct etmv4_metadata *metadata = &drvdata->metadata;
> >> +
> >> +     metadata->magic = ETM4_METADATA_MAGIC;
> >> +     metadata->cpu = cpu;
> >> +     metadata->trcconfigr = config->cfg;
> >> +     metadata->trctraceidr = drvdata->trcid;
> >> +     metadata->trcidr0 = readl_relaxed(drvdata->base + TRCIDR0);
> >> +     metadata->trcidr1 = readl_relaxed(drvdata->base + TRCIDR1);
> >> +     metadata->trcidr2 = readl_relaxed(drvdata->base + TRCIDR2);
> >> +     metadata->trcidr8 = readl_relaxed(drvdata->base + TRCIDR8);
> >
> > I'm pretty sure this won't work on all architecture.  This tracer may not be
> > powered when this is called and depending on the power domain the TRCIDRx
> > registers are in it may lock.
> >
> > Simply read those at boot time as part of the _probe() function.
> 
> Thinking about this will work when operating from sysFS but
> theoretically inaccurate from perf.  Say we have a 4 CPU system where
> for a perf session only 2 CPUs were used for trace acquisition - the
> process was simply not scheduled on the other 2 CPU.  With the above
> code 2 CPUs (the ones that were actually used) will have the right
> configuration while the other two will have the default tracer
> configuration (since etm4_parse_event_config() was never called on
> them).

This is quite important for how to launch tracing you have reminded me
on IRC and I'd like to list all possible tracer configurations:

- Use sysFS to enable some or all ETM tracers;

- Use perf + '--per-thread' to open one session, as you said the
  session may only use some of CPUs but not all of them;

- Use perf + 'snapshot' mode, this mode now doesn't work but we can
  expect this mode can work later so can enable all tracers;

These three scenarios are what I can think out and panic dump should
handle them correctly, if I miss or misunderstand anything, please feel
free correct me.

> Values in TRCIDR{0, 1, 2, 8} and TRCAUTHSTATUS is RO and won't change.
> So those can be filled at _probe() time when tracers are instantiated.
> Values for trcconfigr ang trctraceidr are trickier.
> 
> When operating from sysFS tracer configuration should be recorded in
> etm4_enable_sysfs(), probably just before smp_call_function_single().
> When operating from perf adding all tracers to the dump list should be
> done in etm_setup_aux() and an update for the configuration in
> etm4_enable_perf(), just before etm4_enable_hw().  When circling
> through tracers to communicate the metadata to the kdump mechanic
> check if buf != NULL, what way we only communicate information about
> tracers that were used.

I'm not sure I catch all your points, so I want check if I can
understand your suggestion as below?

We need allocate dump buffer and save registers value when tracer is
enabled, also add their node into dump list (for sysFS, this is done
in etm4_enable_sysfs() and for perf is in etm_setup_aux() and
etm4_enable_perf()).  After panic happens we iterate all tracer dump
nodes and only update buffer info if (buf != NULL).

> Removal of the tracers from the dump list should be done in etm_free_aux()
> 
> This will need testing :o)

Thanks a lot for the detailed suggestion, will verify them.

[...]

Thanks,
Leo Yan

[PATCH v2 4/4] coresight: etm4x: Hook panic dump callback for etmv4

[Mathieu Poirier]

On 23 November 2017 at 21:06, Leo Yan <leo.yan@linaro.org> wrote:
> On Thu, Nov 23, 2017 at 10:03:33AM -0700, Mathieu Poirier wrote:
>> On 22 November 2017 at 12:09, Mathieu Poirier
>> <mathieu.poirier@linaro.org> wrote:
>> > On Tue, Nov 21, 2017 at 11:08:44AM +0800, Leo Yan wrote:
>> >> We need to dump etmv4 info as metadata, so the data can be used for
>> >> offline checking for configuration and generate 'perf' compatible
>> >> file.  This commit hooks etmv4 callback for panic dump.
>> >>
>> >> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> >> ---
>> >>  drivers/hwtracing/coresight/coresight-etm4x.c | 22 ++++++++++++++++++++++
>> >>  drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++++++++++
>> >>  2 files changed, 37 insertions(+)
>> >>
>> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>> >> index cf364a5..bf9608b 100644
>> >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> >> @@ -418,11 +418,33 @@ static void etm4_disable(struct coresight_device *csdev,
>> >>               local_set(&drvdata->mode, CS_MODE_DISABLED);
>> >>  }
>> >>
>> >> +static void etm4_panic_cb(void *data)
>> >> +{
>> >> +     struct coresight_device *csdev = (struct coresight_device *)data;
>> >> +     int cpu = smp_processor_id();
>> >> +     struct etmv4_drvdata *drvdata = etmdrvdata[cpu];
>> >> +     struct etmv4_config *config = &drvdata->config;
>> >> +     struct etmv4_metadata *metadata = &drvdata->metadata;
>> >> +
>> >> +     metadata->magic = ETM4_METADATA_MAGIC;
>> >> +     metadata->cpu = cpu;
>> >> +     metadata->trcconfigr = config->cfg;
>> >> +     metadata->trctraceidr = drvdata->trcid;
>> >> +     metadata->trcidr0 = readl_relaxed(drvdata->base + TRCIDR0);
>> >> +     metadata->trcidr1 = readl_relaxed(drvdata->base + TRCIDR1);
>> >> +     metadata->trcidr2 = readl_relaxed(drvdata->base + TRCIDR2);
>> >> +     metadata->trcidr8 = readl_relaxed(drvdata->base + TRCIDR8);
>> >
>> > I'm pretty sure this won't work on all architecture.  This tracer may not be
>> > powered when this is called and depending on the power domain the TRCIDRx
>> > registers are in it may lock.
>> >
>> > Simply read those at boot time as part of the _probe() function.
>>
>> Thinking about this will work when operating from sysFS but
>> theoretically inaccurate from perf.  Say we have a 4 CPU system where
>> for a perf session only 2 CPUs were used for trace acquisition - the
>> process was simply not scheduled on the other 2 CPU.  With the above
>> code 2 CPUs (the ones that were actually used) will have the right
>> configuration while the other two will have the default tracer
>> configuration (since etm4_parse_event_config() was never called on
>> them).
>
> This is quite important for how to launch tracing you have reminded me
> on IRC and I'd like to list all possible tracer configurations:
>
> - Use sysFS to enable some or all ETM tracers;
>
> - Use perf + '--per-thread' to open one session, as you said the
>   session may only use some of CPUs but not all of them;
>
> - Use perf + 'snapshot' mode, this mode now doesn't work but we can
>   expect this mode can work later so can enable all tracers;

There is two scenarios, sysFS and perf.  There is no need to
discriminate between all the various ways perf can operate.

>
> These three scenarios are what I can think out and panic dump should
> handle them correctly, if I miss or misunderstand anything, please feel
> free correct me.
>
>> Values in TRCIDR{0, 1, 2, 8} and TRCAUTHSTATUS is RO and won't change.
>> So those can be filled at _probe() time when tracers are instantiated.
>> Values for trcconfigr ang trctraceidr are trickier.
>>
>> When operating from sysFS tracer configuration should be recorded in
>> etm4_enable_sysfs(), probably just before smp_call_function_single().
>> When operating from perf adding all tracers to the dump list should be
>> done in etm_setup_aux() and an update for the configuration in
>> etm4_enable_perf(), just before etm4_enable_hw().  When circling
>> through tracers to communicate the metadata to the kdump mechanic
>> check if buf != NULL, what way we only communicate information about
>> tracers that were used.
>
> I'm not sure I catch all your points, so I want check if I can
> understand your suggestion as below?
>
> We need allocate dump buffer and save registers value when tracer is
> enabled, also add their node into dump list (for sysFS, this is done
> in etm4_enable_sysfs() and for perf is in etm_setup_aux() and
> etm4_enable_perf()).  After panic happens we iterate all tracer dump
> nodes and only update buffer info if (buf != NULL).

When working from sysFS, do _kdump_add() and _kdump_update() from
etm4_enable_sysfs().

When working from perf, do _kdump_add() from etm_setup_aux() and
_kdump_update() from etm4_enable_perf().

Get back to me if you need more information.

>
>> Removal of the tracers from the dump list should be done in etm_free_aux()
>>
>> This will need testing :o)
>
> Thanks a lot for the detailed suggestion, will verify them.
>
> [...]
>
> Thanks,
> Leo Yan