linux-kernel - Re: [PATCH 0/7] irq: fix checkpatch errors and warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 07 Jun 2013 15:24:56 -0700
From:	Joe Perches <joe@...ches.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Kefeng Wang <wangkefeng.wang@...wei.com>,
	Grant Likely <grant.likely@...aro.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	linux-kernel@...r.kernel.org, guohanjun@...wei.com
Subject: Re: [PATCH 0/7] irq: fix checkpatch errors and warnings

On Fri, 2013-06-07 at 22:42 +0200, Thomas Gleixner wrote:
> On Thu, 6 Jun 2013, Kefeng Wang wrote:
> 
> > Fix all the checkpath errors in kernel/irq dir, and some warnings
> > also fixed.
> 
> Sorry, I'm not really interested in this kind of patches. To be
> honest, it would be way more exciting if you had taught checkpatch to
> actually fix the missing space after the comma.

Yup.  Kefeng, if you want to take that up,
I'd be happy to help/guide you.

> Aside of that your mechanical fixups are mostly making the code worse
> to read. Just a few examples:
> 
> --- linux-2.6.orig/kernel/irq/chip.c
[]
>         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
>                                                      IRQ_GET_DESC_CHECK_GLOBAL);
> 
> Aligning the first arguments of the first and the second line makes it
> way simpler to read.

or maybe use

	struct irq_desc *desc =
		irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);

and satisfy 80 cols too.

Existing uses that exceed the 80 column text limit shouldn't
be changed without making other useful changes at the same
time.  Make 80 column changes as part of a larger patch set,

> --- linux-2.6.orig/kernel/irq/handle.c
[]
> @@ -47,7 +47,7 @@ static void warn_no_thread(unsigned int 

> -       printk(KERN_WARNING "IRQ %d device %s returned IRQ_WAKE_THREAD "
> +       pr_warn("IRQ %d device %s returned IRQ_WAKE_THREAD "
>                "but no thread function available.", irq, action->name);

It'd be useful to add a terminating \n though to avoid
interleaving by other thread pr_cont/naked printks.

> The checkpatch warning is to prevent new code to use the old style
> printk format, but it's not intended to force that on existing code.

Yup, and checkpatch will never be a perfect tool.

> Please sit down and read and understand the code and try to find real
> issues which cannot be detected and solved by scripts and
> tools. That's what's kernel hacking is about. You are not becoming a
> kernel developer by running tools and blindly fixing the complaints of
> the tools. You have to understand the code and you have to learn to
> judge the output of tools.

Quite right.

Maybe add something like it to SubmittingPatches in
Section 2 ala:

5) "Don't be a checkpatch monkey - How not to write patches"


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/