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

Dwaipayan Ray dwaipayanray1 at gmail.com
Sat Nov 21 16:16:04 UTC 2020


On Sat, Nov 21, 2020 at 6:43 PM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
>
> 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.
>
Okay sure I will try doing that.

I am just sending this patch unchanged to Joe and LKML to have their
review. If it is ok, I will get on with the next task.

Thanks,
Dwaipayan.


More information about the Linux-kernel-mentees mailing list