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]

Message-ID: <alpine.LFD.2.02.1306072208420.24812@ionos>
Date:	Fri, 7 Jun 2013 22:42:53 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Kefeng Wang <wangkefeng.wang@...wei.com>
cc:	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 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.

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
+++ linux-2.6/kernel/irq/chip.c
@@ -56,7 +56,8 @@ EXPORT_SYMBOL(irq_set_chip);
 int irq_set_irq_type(unsigned int irq, unsigned int type)
 {
 	unsigned long flags;
-	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
+					IRQ_GET_DESC_CHECK_GLOBAL);

This is horrible to parse. If we enforce the 80 character limit here
at all, which I doubt it has a value for this particular case, then
please adhere to the coding style used in this file and make it 

	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.

Aside of that, there is no real value to make changes like this:

--- linux-2.6.orig/kernel/irq/irqdomain.c
+++ linux-2.6/kernel/irq/irqdomain.c
@@ -419,7 +419,7 @@ static void irq_domain_disassociate_many
 		irq_data->hwirq = 0;

 		/* Clear reverse map */
-		switch(domain->revmap_type) {
+		switch (domain->revmap_type) {

And this:

--- linux-2.6.orig/kernel/irq/proc.c
+++ linux-2.6/kernel/irq/proc.c
@@ -276,7 +276,7 @@ static int name_unique(unsigned int irq,
 	int ret = 1;

 	raw_spin_lock_irqsave(&desc->lock, flags);
-	for (action = desc->action ; action; action = action->next) {
+	for (action = desc->action; action; action = action->next) {

Now there is another category:

@@ -47,7 +47,7 @@ static void warn_no_thread(unsigned int 
 	if (test_and_set_bit(IRQTF_WARNED, &action->thread_flags))
 		return;

-	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);

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.

We can write a coccinelle script to fix such issues in one go all over
the kernel source tree. Though I doubt that it has much value.

It does not make the source better. It does not fix any bugs. It's
just pointless entries in the changelogs

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.

Thanks,

	tglx
--
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/