On 04/09/19 2:32 PM, Mahesh Jagannath Salgaonkar wrote:

On 9/3/19 9:35 PM, Hari Bathini wrote:

quoted

On 03/09/19 4:39 PM, Michael Ellerman wrote:

quoted

Hari Bathini [off-list ref] writes:

quoted

Make way for refactoring platform specific FADump code by moving code
that could be referenced from multiple places to fadump-common.c file.

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---
 arch/powerpc/kernel/Makefile        |    2 
 arch/powerpc/kernel/fadump-common.c |  140 ++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/fadump-common.h |    8 ++
 arch/powerpc/kernel/fadump.c        |  146 ++---------------------------------
 4 files changed, 158 insertions(+), 138 deletions(-)
 create mode 100644 arch/powerpc/kernel/fadump-common.c

I don't understand why we need fadump.c and fadump-common.c? They're
both common/shared across pseries & powernv aren't they?

The convention I tried to follow to have fadump-common.c shared between fadump.c,
pseries & powernv code while pseries & powernv code take callback requests from
fadump.c and use fadump-common.c (shared by both platforms), if necessary to fullfil
those requests...

quoted

By the end of the series we end up with 149 lines in fadump-common.c
which seems like a waste of time. Just put it all in fadump.c.

Yeah. Probably not worth a new C file. Will just have two separate headers. One for
internal code and one for interfacing with other modules...

[...]

quoted

quoted

+ * Copyright 2019, IBM Corp.
+ * Author: Hari Bathini [off-list ref]

These can just be:

 * Copyright 2011, Mahesh Salgaonkar, IBM Corporation.
 * Copyright 2019, Hari Bathini, IBM Corporation.

Sure.

quoted

quoted

+ */
+
+#undef DEBUG

Don't undef DEBUG please.

Sorry! Seeing such thing in most files, I thought this was the convention. Will drop
this change in all the new files I added.

quoted

quoted

+#define pr_fmt(fmt) "fadump: " fmt
+
+#include <linux/memblock.h>
+#include <linux/elf.h>
+#include <linux/mm.h>
+#include <linux/crash_core.h>
+
+#include "fadump-common.h"
+
+void *fadump_cpu_notes_buf_alloc(unsigned long size)
+{
+	void *vaddr;
+	struct page *page;
+	unsigned long order, count, i;
+
+	order = get_order(size);
+	vaddr = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, order);
+	if (!vaddr)
+		return NULL;
+
+	count = 1 << order;
+	page = virt_to_page(vaddr);
+	for (i = 0; i < count; i++)
+		SetPageReserved(page + i);
+	return vaddr;
+}

I realise you're just moving this code, but why do we need all this hand
rolled allocation stuff?

Yeah, I think alloc_pages_exact() may be better here. Mahesh, am I missing something?

We hook up the physical address of this buffer to ELF core header as
PT_NOTE section. Hence we don't want these pages to be moved around or
reclaimed.

alloc_pages_exact() + mark_page_reserved() should take care of that, I guess..

- Hari