Re: [PATCH v3 03/16] pseries/fadump: move out platform specific support from generic code

[Hari Bathini <hbathini@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]


On 03/07/19 9:34 AM, Oliver O'Halloran wrote:
> On Wed, 2019-06-26 at 02:16 +0530, Hari Bathini wrote:
>> Introduce callbacks for platform specific operations like register,
>> unregister, invalidate & such, and move pseries specific code into
>> platform code.
> Please don't move around large blocks of code *and* change the code in
> a single patch. It makes reviewing the changes extremely tedious since
> the changes are mixed in with hundreds of lines of nothing.

Right, Oliver.
I am already working on splitting up few other patches for ease of reviewing.
Thought of keeping this patch this way though as it doesn't add any new logic
but moves the code around. Will ensure I split this one up too for the sake
of sanity.

>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>>  arch/powerpc/include/asm/fadump.h            |   75 ----
>>  arch/powerpc/kernel/fadump-common.h          |   38 ++
>>  arch/powerpc/kernel/fadump.c                 |  500 ++-----------------------
>>  arch/powerpc/platforms/pseries/Makefile      |    1 
>>  arch/powerpc/platforms/pseries/rtas-fadump.c |  529 ++++++++++++++++++++++++++
>>  arch/powerpc/platforms/pseries/rtas-fadump.h |   96 +++++
>>  6 files changed, 700 insertions(+), 539 deletions(-)
>>  create mode 100644 arch/powerpc/platforms/pseries/rtas-fadump.c
>>  create mode 100644 arch/powerpc/platforms/pseries/rtas-fadump.h
>>
>> +static struct fadump_ops pseries_fadump_ops = {
>> +	.init_fadump_mem_struct	= pseries_init_fadump_mem_struct,
>> +	.register_fadump	= pseries_register_fadump,
> I realise you are just translating the existing interface, but why is
> init_fadump_mem_struct() done as a seperate step and not as a part of
> the registration function? The struct doesn't seem to be necessary
> until the actual registration happens.

Yeah. But registration is a user choice and can happen multiple times within
a single boot (for example, due to hotplug operations) but the structure
contents remain the same. So, it is initialized once early on...

>
>> +	.unregister_fadump	= pseries_unregister_fadump,
>> +	.invalidate_fadump	= pseries_invalidate_fadump,
>> +	.process_fadump		= pseries_process_fadump,
>> +	.fadump_region_show	= pseries_fadump_region_show,
>> +	.crash_fadump		= pseries_crash_fadump,
> Rename this to fadump_trigger or something, it's not clear what it
> does.

Sure.
Thanks for the review!

- Hari


[-- Attachment #2: Type: text/html, Size: 3962 bytes --]