On 9/3/19 9:35 PM, Hari Bathini wrote:
>
>
> 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?
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.
Thanks,
-Mahesh.
