Re: [PATCH v5 02/31] powerpc/fadump: move internal code to a new file

From: Hari Bathini <hbathini@linux.ibm.com>
To: Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	linuxppc-dev <linuxppc-dev@ozlabs.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>,
	Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
	Nicholas Piggin <npiggin@gmail.com>, Oliver <oohall@gmail.com>,
	Vasant Hegde <hegdevasant@linux.ibm.com>,
	Daniel Axtens <dja@axtens.net>
Subject: Re: [PATCH v5 02/31] powerpc/fadump: move internal code to a new file
Date: Wed, 4 Sep 2019 23:56:44 +0530	[thread overview]
Message-ID: <012215ea-99ab-1993-4eb2-0d3fc248a1ea@linux.ibm.com> (raw)
In-Reply-To: <3c6bacc2-777d-a66c-0c43-79b9fbe76c15@linux.vnet.ibm.com>

On 04/09/19 2:32 PM, Mahesh Jagannath Salgaonkar wrote:
> On 9/3/19 9:35 PM, Hari Bathini wrote:
>>
>>
>> On 03/09/19 4:39 PM, Michael Ellerman wrote:
>>> Hari Bathini <hbathini@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 <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...
>>
>>> 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 <hbathini@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.

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

- Hari