Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups- formatting only

[Ingo Molnar]

* David Miller <davem@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/