Date: Tue, 25 Mar 2008 14:05:57 +0100
From: Ingo Molnar <mingo@elte.hu>
To: David Miller <davem@davemloft.net>
Cc: jirislaby@gmail.com, viro@ZenIV.linux.org.uk, joe@perches.com,
       tglx@linutronix.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups
	- formatting only
Message-ID: <20080325130557.GA9612@elte.hu>
References: <47E64BF7.4070808@gmail.com> <20080323.053037.144236584.davem@davemloft.net> <20080325084457.GA32103@elte.hu> <20080325.024204.175042056.davem@davemloft.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20080325.024204.175042056.davem@davemloft.net>
User-Agent: Mutt/1.5.17 (2007-11-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1922
Lines: 50


* David Miller <davem@davemloft.net> wrote:

> I can tell you one more example of things I strongly disagree with 
> that it does, for example, such as telling me how to document 
> spinlocks in datastructures.
> 
> It wants a comment right above the spinlock_t member, but this totally 
> ignores that perhaps I put a huge comment explaining the locking 
> semantics elsewhere.

firstly, this warning from checkpatch.pl is off by default.

There are 3 checkpatch warning categories: ERROR, WARNING, CHECK. 
spinlock_t without a warning is in this third category and you wont even 
see that warning unless you very explicitly do:

    checkpatch.pl --subjective

Secondly, even about this "checkpatch.pl --subjective" check you are 
wrong. As someone who had to decode (way!) too many lockdep backtraces 
in various kernel code that i didnt author and didnt maintain, i can 
tell it you with very strong authority that even in this case it's a 
minimum requirement to put a comment right to that lock:

 /*
  * Regarding the locking rules, see the big comment block above in
  * this file:
  */

or:

 /* See net/core/sock.c for the locking rules: */

_Way_ too many times do i have to wonder where the heck a given lock is 
documented. You _wrote_ and maintain a good portion of that code, so to 
you it's seemingly an annoyance and nuisance. To everyone else, it's 
must-have information. Locks are at the heart of kernel data structures, 
not having at least a minimal pointer at them is really bad.

(sidenote: the scheduler has one deficiency there and i've fixed it in 
my tree. this warning should be moved from the 'check' category into the 
warning category.)

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