On Fri, Mar 30, 2018 at 11:15:21AM +0800, Leo Yan wrote:
> After kernel panic happens, Coresight tracing data has much useful info
> which can be used for analysis. For example, the trace info from ETB
> RAM can be used to check the CPU execution flows before the crash. So
> we can save the tracing data from sink devices, and rely on kdump to
> save DDR content and uses "crash" tool to extract Coresight dumping
> from the vmcore file.
>
> This patch is to add a simple framework to support panic dump
> functionality; it registers panic notifier, and provide the helper
> functions coresight_kdump_source()/coresight_kdump_sink() so Coresight
> source and sink devices can be recorded into Coresight kdump node for
> kernel panic kdump.
>
> When kernel panic happens, the notifier iterates dump array and invoke
> callback function to dump tracing data. Later the tracing data can be
> used to reverse execution flow before the kernel panic.
>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
> drivers/hwtracing/coresight/Kconfig | 9 +
> drivers/hwtracing/coresight/Makefile | 1 +
> .../hwtracing/coresight/coresight-panic-kdump.c | 199 +++++++++++++++++++++
> drivers/hwtracing/coresight/coresight-priv.h | 12 ++
> include/linux/coresight.h | 4 +
> 5 files changed, 225 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-panic-kdump.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index ef9cb3c..3089abf 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_KDUMP
> + bool "CoreSight Panic Kdump driver"
> + depends on ARM || ARM64
> + help
> + This driver provides panic kdump functionality for CoreSight devices.
> + When kernel panic happen Coresight device supplied callback function
s/Coresight/CoreSight
> + is to dump trace data to memory. From then on, kdump can be used to
> + extract the trace data from kernel dump file.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 61db9dd..946fe19 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -18,3 +18,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_KDUMP) += coresight-panic-kdump.o
> diff --git a/drivers/hwtracing/coresight/coresight-panic-kdump.c b/drivers/hwtracing/coresight/coresight-panic-kdump.c
> new file mode 100644
> index 0000000..f4589e9
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-panic-kdump.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017~2018 Linaro Limited.
I don't remember if I commented on this before but the above line (not the SPDX)
should be enclosed with C style comments (/* */) rather than C++ (//).
I would also add a new line between the copyright statement and the header file
listing.
> +#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"
> +
> +/**
> + * struct coresight_kdump_node - Node information for dump
> + * @source_csdev: Handler for source coresight device
> + * @sink_csdev: Handler for sink coresight device
> + */
> +struct coresight_kdump_node {
> + struct coresight_device *source_csdev;
> + struct coresight_device *sink_csdev;
> +};
> +
> +static DEFINE_SPINLOCK(coresight_kdump_lock);
> +static struct coresight_kdump_node *coresight_kdump_nodes;
> +static struct notifier_block coresight_kdump_nb;
> +
> +/**
> + * coresight_kdump_source - Set source dump info for specific CPU
> + * @cpu: CPU ID
> + * @csdev: Source device structure handler
> + * @data: Pointer for source device metadata buffer
> + * @data_sz: Size of source device metadata buffer
> + *
> + * This function is a helper function which is used to set/clear source device
> + * handler and metadata when the tracer is enabled; and it can be used to clear
> + * source device related info when the tracer is disabled.
> + *
> + * Returns: 0 on success, negative errno otherwise.
> + */
> +int coresight_kdump_source(int cpu, struct coresight_device *csdev,
> + char *data, unsigned int data_sz)
> +{
> + struct coresight_kdump_node *node;
> + unsigned long flags;
> +
> + if (!coresight_kdump_nodes)
> + return -EPROBE_DEFER;
Before grabbing the lock you also need to make sure
@cpu is < num_possible_cpus().
> +
> + spin_lock_irqsave(&coresight_kdump_lock, flags);
> +
> + node = &coresight_kdump_nodes[cpu];
> + node->source_csdev = csdev;
> +
> + csdev->kdump_buf = data;
> + csdev->kdump_buf_sz = data_sz;
> +
> + spin_unlock_irqrestore(&coresight_kdump_lock, flags);
> + return 0;
> +}
> +
> +/**
> + * coresight_kdump_sink - Set sink device handler for specific CPU
> + * @cpu: CPU ID
> + * @csdev: Sink device structure handler
> + *
> + * This function is a helper function which is used to set sink device handler
> + * when the Coresight path has been enabled for specific CPU; and it can be used
> + * to clear sink device handler when the path is disabled.
> + *
> + * Returns: 0 on success, negative errno otherwise.
> + */
> +int coresight_kdump_sink(int cpu, struct coresight_device *csdev)
> +{
> + struct coresight_kdump_node *node;
> + unsigned long flags;
> +
> + if (!coresight_kdump_nodes)
> + return -EPROBE_DEFER;
Same comment as above.
> +
> + spin_lock_irqsave(&coresight_kdump_lock, flags);
> +
> + node = &coresight_kdump_nodes[cpu];
> + node->sink_csdev = csdev;
csdev->kdump_buf = NULL;
csdev->kdump_buf_sz = 0;
> +
> + spin_unlock_irqrestore(&coresight_kdump_lock, flags);
> + return 0;
> +}
> +
> +/**
> + * coresight_kdump_sink_cb - Invoke sink callback for specific CPU
> + * @cpu: CPU ID
> + *
> + * This function is to invoke sink device corresponding callback. It needs
> + * to check two cases: one case is the CPU has not been enabled for Coresight
> + * path so there totally has no trace data for the CPU, another case is the
> + * CPU shares the same sink device with other CPUs but the tracing data has
> + * been dumped by previous CPUs; it skips dump for these two cases.
> + */
> +static void coresight_kdump_sink_cb(int cpu)
> +{
> + struct coresight_kdump_node *node;
> + struct coresight_device *csdev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&coresight_kdump_lock, flags);
> +
> + node = &coresight_kdump_nodes[cpu];
> + csdev = node->sink_csdev;
> +
> + /* Path has not been enabled */
> + if (!csdev)
> + goto skip_dump;
> +
> + /* Have been dumped by previous CPU */
> + if (csdev->kdump_buf)
I would use csdev->kdump_buf_sz instead of csdev->kdump_buf. The reason is that
the sink may not always use an internal SRAM buffer. For instance the ETR uses
system RAM, either as a contiguous buffer or a scatter-gather list. When the
panic callback for an ETR is implemented the code in the core (i.e this file)
need not change as kdump_buf_sz is independent of the way data is conveyed.
> + goto skip_dump;
> +
> + /* Invoke panic callback */
> + csdev = coresight_kdump_nodes[cpu].sink_csdev;
> + if (csdev && sink_ops(csdev)->panic_cb)
> + sink_ops(csdev)->panic_cb(csdev);
> +
> +skip_dump:
> + spin_unlock_irqrestore(&coresight_kdump_lock, flags);
> +}
> +
> +/**
> + * coresight_kdump_notify - Invoke panic dump callbacks
> + * @nb: Pointer to notifier block
> + * @event: Notification reason
> + * @_unused: Pointer to notification data object, unused
> + *
> + * This function is called when panic happens to invoke dump callbacks, it takes
> + * panic CPU tracing data with high priority to firstly invoke panic CPU sink
> + * callback function, then the notifier iterates callback functions one by one
> + * for other CPUs. If one sink device is shared among CPUs, the sink panic
> + * callback is invoked for the first traversed CPU node and other sequential
> + * CPUs are skipped.
> + *
> + * Returns: 0 on success.
> + */
> +static int coresight_kdump_notify(struct notifier_block *nb,
> + unsigned long event, void *_unused)
> +{
> + int cpu, first;
> +
> + /* Give panic CPU trace data with high priority */
I would replace the above comment with "Start with the panic'ed CPU".
> + first = atomic_read(&panic_cpu);
> + coresight_kdump_sink_cb(first);
> +
> + /* Dump rest CPUs trace data */
> + for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
> + if (cpu == first)
> + continue;
> +
> + coresight_kdump_sink_cb(cpu);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * coresight_kdump_init - Coresight kdump module initialization
> + *
> + * This function allcoates dump array and register panic norifier.
> + *
> + * Returns: 0 on success, negative errno otherwise.
> + */
> +static int __init coresight_kdump_init(void)
> +{
> + int ret;
> +
> + coresight_kdump_nodes = kmalloc_array(num_possible_cpus(),
> + sizeof(*coresight_kdump_nodes),
> + GFP_KERNEL);
> + if (!coresight_kdump_nodes) {
> + pr_err("%s: kmalloc failed\n", __func__);
> + return -ENOMEM;
> + }
> +
> + memset(coresight_kdump_nodes, 0,
> + num_possible_cpus() * sizeof(*coresight_kdump_nodes));
If you use kcalloc() above you don't need to explicitly zero out the memory.
> +
> + coresight_kdump_nb.notifier_call = coresight_kdump_notify;
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &coresight_kdump_nb);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + kfree(coresight_kdump_nodes);
> + return ret;
> + }
> +
> + return 0;
> +}
> +postcore_initcall(coresight_kdump_init);
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index f1d0e21d..76d27d6 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -151,4 +151,16 @@ 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
>
> +#ifdef CONFIG_CORESIGHT_PANIC_KDUMP
> +extern int coresight_kdump_source(int cpu, struct coresight_device *csdev,
> + char *data, unsigned int data_sz);
> +extern int coresight_kdump_sink(int cpu, struct coresight_device *csdev);
> +#else
> +static inline int coresight_kdump_source(int cpu,
> + struct coresight_device *csdev,
> + char *data, unsigned int data_sz) { return 0; }
> +static inline void coresight_kdump_sink(int cpu,
> + struct coresight_device *csdev) { return 0; }
To me the above is harder to read - I suggest:
static inline int
coresight_kdump_source(int cpu,
struct coresight_device *csdev,
char *data, unsigned int data_sz) { return 0; }
static inline void
coresight_kdump_sink(int cpu,
struct coresight_device *csdev) { return 0; }
> +#endif
> +
> #endif
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index d950dad..89aad8d 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -171,6 +171,8 @@ 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 */
> + char *kdump_buf;
> + unsigned int kdump_buf_sz;
Please add structure documentation, the same way all the other fields in this
structure is.
> };
>
> #define to_coresight_device(d) container_of(d, struct coresight_device, dev)
> @@ -189,6 +191,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 +208,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);
> };
>
> /**
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
