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

[Oliver O'Halloran <oohall@gmail.com>]

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.

> 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.

> +	.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.