Re: [PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

[Mark Rutland]

Hi,

On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott wrote:
> ptdump_register currently initializes a set of page table information and
> registers debugfs. There are uses for the ptdump option without wanting the
> debugfs options. Split this out to make it a separate option.
> 
> Signed-off-by: Laura Abbott <labbott@xxxxxxxxxx>
> ---
>  arch/arm64/Kconfig.debug        |  6 +++++-
>  arch/arm64/include/asm/ptdump.h | 15 +++++++++++++--
>  arch/arm64/mm/Makefile          |  3 ++-
>  arch/arm64/mm/dump.c            | 30 +++++++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+), 25 deletions(-)
>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c

As a heads-up, Ard has new ARM64_PTUMP user under drivers/firmware/efi queued
up in the EFI tree, which will also need fixing up. See commit d80448ac92b72051
("efi/arm64: Add debugfs node to dump UEFI runtime page tables") [1].

[...]

> +#include <linux/seq_file.h>
>  #include <linux/mm_types.h>

Nit: please keep headers in alphabetical order.

> -static void walk_pgd(struct pg_state *st, struct mm_struct *mm,
> +static void __walk_pgd(struct pg_state *st, struct mm_struct *mm,

Can we leave this name as-is? We didn't change walk_{pud,pmd,pte}, so this is
inconsistent, and we haven't reused the name.

[...]

> +int ptdump_register(struct ptdump_info *info, const char *name)
> +{
> +	ptdump_initialize(info);
> +	return ptdump_debugfs_create(info, name);
>  }

It feels like a layering violation to have the core ptdump code call the
debugfs ptdump code. Is there some reason this has to live here?

Other than the above points, this looks good to me.

Thanks,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=next&id=9d80448ac92b720512c415265597d349d8b5c3e8