Re: [PATCH V1 2/2] treewide: include linux/pgalloc.h instead of asm/pgalloc.h

[Harry Yoo <harry.yoo@oracle.com>]

On Mon, Oct 20, 2025 at 05:44:08PM +0300, Mike Rapoport wrote:
> On Sun, Oct 19, 2025 at 04:25:09PM +0900, Harry Yoo wrote:
> > For now, including <asm/pgalloc.h> instead of <linux/pgalloc.h>
> > is technically fine unless the .c file calls p*d_populate_kernel()
> > helper functions.
> > 
> > But it is a better practice to always include <linux/pgalloc.h>.
> > Include <linux/pgalloc.h> instead of <asm/pgalloc.h> outside arch/.
> > 
> > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> > ---
> >  drivers/firmware/efi/arm-runtime.c   | 2 +-
> >  drivers/firmware/efi/riscv-runtime.c | 2 +-
> >  drivers/s390/char/sclp_sd.c          | 3 +--
> >  fs/dax.c                             | 2 +-
> >  kernel/fork.c                        | 2 +-
> >  mm/debug_vm_pgtable.c                | 2 +-
> >  mm/filemap.c                         | 2 +-
> >  mm/huge_memory.c                     | 2 +-
> >  mm/hugetlb.c                         | 2 +-
> >  mm/hugetlb_vmemmap.c                 | 2 +-
> >  mm/khugepaged.c                      | 2 +-
> >  mm/memory.c                          | 2 +-
> >  mm/mmu_gather.c                      | 2 +-
> >  mm/mremap.c                          | 2 +-
> >  mm/pgtable-generic.c                 | 2 +-
> >  mm/pt_reclaim.c                      | 2 +-
> >  16 files changed, 16 insertions(+), 17 deletions(-)

Hi Mike, thanks for review.

I wasn't really sure if there are certain rules for header inclusion
(yeah, they are not very consistent across different files...) but now that
you mention it I can see them, thanks!

> > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> > index 83092d93f36a..2e3b5131df32 100644
> > --- a/drivers/firmware/efi/arm-runtime.c
> > +++ b/drivers/firmware/efi/arm-runtime.c
> > @@ -19,11 +19,11 @@
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/pgtable.h>
> > +#include <linux/pgalloc.h>
>
> Looks like here the includes are sorted alphabetically, please keep it.

Will do.

> >  #include <asm/cacheflush.h>
> >  #include <asm/efi.h>
> >  #include <asm/mmu.h>
> > -#include <asm/pgalloc.h>
> >  
> >  #if defined(CONFIG_PTDUMP_DEBUGFS) || defined(CONFIG_ARM_PTDUMP_DEBUGFS)
> >  #include <asm/ptdump.h>
> > diff --git a/drivers/firmware/efi/riscv-runtime.c b/drivers/firmware/efi/riscv-runtime.c
> > index fa71cd898120..5b8d72885fee 100644
> > --- a/drivers/firmware/efi/riscv-runtime.c
> > +++ b/drivers/firmware/efi/riscv-runtime.c
> > @@ -21,11 +21,11 @@
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/pgtable.h>
> > +#include <linux/pgalloc.h>
> 
> And here as well.

Will do.

> >  #include <asm/cacheflush.h>
> >  #include <asm/efi.h>
> >  #include <asm/mmu.h>
> > -#include <asm/pgalloc.h>
> 
> ...
> 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 3da0f08615a9..9d48646e659f 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -107,7 +107,7 @@
> >  #include <linux/tick.h>
> >  #include <linux/unwind_deferred.h>
> >  
> > -#include <asm/pgalloc.h>
> > +#include <linux/pgalloc.h>
> >  #include <linux/uaccess.h>
> 
> I'd kill an empty line before include <linux/pgalloc.h> and add one here to
> separate include <asm/.

That looks good and will do.

> >  #include <asm/mmu_context.h>
> >  #include <asm/cacheflush.h>
> 
> ...
> 
> > diff --git a/mm/pt_reclaim.c b/mm/pt_reclaim.c
> > index 7e9455a18aae..ec6fe4eb2644 100644
> > --- a/mm/pt_reclaim.c
> > +++ b/mm/pt_reclaim.c
> > @@ -1,7 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/hugetlb.h>
> >  #include <asm-generic/tlb.h>
> > -#include <asm/pgalloc.h>
> > +#include <linux/pgalloc.h>
> 
> Please move it before include <asm-generic/tlb.h>

Will do, and in this case no empty line between #include <asm*/*.h> and
#include <linux/*.h>, right?

> >  #include "internal.h"
> 
> With those fixed
> 
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

Thanks a lot!

> -- 
> Sincerely yours,
> Mike.

-- 
Cheers,
Harry / Hyeonggon