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

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: <20080325130557.GA9612@elte.hu>
Date:	Tue, 25 Mar 2008 14:05:57 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	David Miller <davem@...emloft.net>
Cc:	jirislaby@...il.com, viro@...IV.linux.org.uk, joe@...ches.com,
	tglx@...utronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups
	- formatting only

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