On 8/16/2018 8:29 AM, Steven Rostedt wrote:
Sorry for the late reply, I actually wrote this email over a week ago,
but never hit send. And the email was pushed back behind other
windows. :-/
Thanks for the review Steven.
And no problem on late reply, I was working on Will's comment about
instrumentation in arch code and was about to respin a v2 patch. I have
replied inline, let me know if any more corrections or improvements can
be done. I would also like if Kees or someone from pstore could comment
on patch 2.
On Fri, 3 Aug 2018 19:58:42 +0530
Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx> wrote:
diff --git a/kernel/trace/trace_rtb.c b/kernel/trace/trace_rtb.c
new file mode 100644
index 000000000000..e8c24db71a2d
--- /dev/null
+++ b/kernel/trace/trace_rtb.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 The Linux Foundation. All rights reserved.
+ */
+#include <linux/atomic.h>
+#include <linux/dma-mapping.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/rtb.h>
+#include <linux/sched/clock.h>
+
+static struct platform_device *rtb_dev;
+static atomic_t rtb_idx;
+
+struct rtb_state {
+ struct rtb_layout *rtb;
+ phys_addr_t phys;
+ unsigned int nentries;
+ unsigned int size;
+ int enabled;
+};
+
+static struct rtb_state rtb = {
+ .enabled = 0,
+};
No need for the initialization, you could just have:
static struct rtb_state rtb;
And it will be initialized to all zeros. Or did you do that to document
that it is not enabled at boot?
I will correct it in v2. RTB will not be enabled until pstore is
registered since we use pstore for logs. I will add a comment above the
static declaration saying so.
+
+static int rtb_panic_notifier(struct notifier_block *this,
+ unsigned long event, void *ptr)
+{
+ rtb.enabled = 0;
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block rtb_panic_blk = {
+ .notifier_call = rtb_panic_notifier,
+ .priority = INT_MAX,
+};
+
+static void rtb_write_type(const char *log_type,
+ struct rtb_layout *start)
+{
+ start->log_type = log_type;
+}
+
+static void rtb_write_caller(u64 caller, struct rtb_layout *start)
+{
+ start->caller = caller;
+}
+
+static void rtb_write_data(u64 data, struct rtb_layout *start)
+{
+ start->data = data;
+}
+
+static void rtb_write_timestamp(struct rtb_layout *start)
+{
+ start->timestamp = sched_clock();
+}
Why have the above static functions? They are not very helpful, and
appear to be actually confusing. They are used once.
Yes you are right, will remove those.
+
+static void uncached_logk_pc_idx(const char *log_type, u64 caller,
+ u64 data, int idx)
+{
+ struct rtb_layout *start;
+
+ start = &rtb.rtb[idx & (rtb.nentries - 1)];
+
+ rtb_write_type(log_type, start);
+ rtb_write_caller(caller, start);
+ rtb_write_data(data, start);
+ rtb_write_timestamp(start);
How is the above better than:
start->log_type = log_type;
start->caller = caller;
start->data = data;
start->timestamp = sched_clock();
??
Sure, will change it to above and post v2.
+ /* Make sure data is written */
+ mb();
+}
+
+static int rtb_get_idx(void)
+{
+ int i, offset;
+
+ i = atomic_inc_return(&rtb_idx);
+ i--;
+
+ /* Check if index has wrapped around */
+ offset = (i & (rtb.nentries - 1)) -
+ ((i - 1) & (rtb.nentries - 1));
+ if (offset < 0) {
+ i = atomic_inc_return(&rtb_idx);
+ i--;
+ }
+
+ return i;
+}
+
+noinline void notrace uncached_logk(const char *log_type, void *data)
BTW, all files in this directory have their functions notrace by
default.
Oh I missed it. Will remove notrace.
- Sai Prakash
