[Linux-kernel-mentees] [PATCH] checkpatch: add --fix option for INCLUDE_LINUX

Lukas Bulwahn lukas.bulwahn at gmail.com
Sat Nov 21 13:12:50 UTC 2020


On Sat, Nov 21, 2020 at 12:56 PM Dwaipayan Ray <dwaipayanray1 at gmail.com> wrote:
>
> On Sat, Nov 21, 2020 at 3:15 PM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
> >
> > On Sat, Nov 21, 2020 at 9:09 AM Dwaipayan Ray <dwaipayanray1 at gmail.com> wrote:
> > >
> > > Provide fix option to INCLUDE_LINUX check to replace asm
> > > includes.
> > >
> > > Macros of type:
> > >  #include <asm/percpu.h>
> > >
> > > are corrected to:
> > >  #include <linux/percpu.h>
> > >
> >
> > The patch itself looks okay; but should this fix option not be limited
> > to lines where content is added?
> >
>
> Hi,
> I think the warning itself is emitted for lines which are added.
> So I think the fix option even now will change those added lines only.
>
> > Further we need to investigate:
> > - is this still a valid rule?
> > - is this rule and the rationale documented somewhere?
>
> I think the rule is valid. I was going through the documentation files
> and in several examples I saw that the linux headers are preferred.
>
> Like in Documentation/core-api/local_ops.rst:
>
> 91:
>    #include <linux/percpu.h>
>    #include <asm/local.h>
>
> In Documentation/trace/kprobes.rst:
>
> 362:
> #include <linux/kprobes.h>
> int register_kprobe(struct kprobe *kp);
>
> 400:
> #include <linux/kprobes.h>
> #include <linux/ptrace.h>
> int pre_handler(struct kprobe *p, struct pt_regs *regs);
>
> 410:
> #include <linux/kprobes.h>
> #include <linux/ptrace.h>
> void post_handler(struct kprobe *p, struct pt_regs *regs,
>   unsigned long flags);
>
> and so on...
>
> I didn't find any explicit mention about absolutely preffering
> linux headers over asm headers whenever it exists.
>
> > - how many rule violations currently exist in the kernel repository?
> > - are there known exceptions for some files in some directories?
>
> There are exceptions to this.
> Like in include/linux/percpu.h,
> #include <asm/percpu.h> needs to be included. as linux/percpu wraps
> over asm/percpu. So these kinds are exempted from the check and
> checkpatch doesn't report any warning.
>
> I did find some violations like in arch/csky/kernel/probes/decode-insn.h:
> CHECK: Consider using #include <linux/kprobes.h> instead of <asm/kprobes.h>
> #7: FILE: arch/csky/kernel/probes/decode-insn.h:7:
> +#include <asm/kprobes.h>
>
> I found one commit which fixed one such case in the kernel:
> commit ca5999fde0a17 ("mm: introduce include/linux/pgtable.h")
> It says "The include/linux/pgtable.h is going to be the home of
> generic page table
>  manipulation functions."
>
> So I think it means that future additings should use <linux/pgtable.h>
> instead of
> <asm/pgtable.h>
>
> I think the ARCH_INCLUDE_LINUX also deserves a --fix option similarly.
>
> What do you think? Should I continue with this?
>

Yes, that is good. Thanks for the investigation.

You can consider to:

- also fix the cases where this rule is violated and send patches to
the maintainers.
- add some documentation on this rule.

Lukas


More information about the Linux-kernel-mentees mailing list