On 03/09/19 4:39 PM, Michael Ellerman wrote:
> Hari Bathini <hbath...@linux.ibm.com> writes:
>> 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 <hbath...@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...
> 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...
[...]
>> + * Copyright 2019, IBM Corp.
>> + * Author: Hari Bathini <hbath...@linux.ibm.com>
>
> These can just be:
>
> * Copyright 2011, Mahesh Salgaonkar, IBM Corporation.
> * Copyright 2019, Hari Bathini, IBM Corporation.
>
Sure.
>> + */
>> +
>> +#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.
>> +#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?
- Hari
