Re: checkkpatch (in)sanity ?

From: Kalle Valo <kvalo@codeaurora.org>
To: Joe Perches <joe@perches.com>
Cc: "Levin\, Alexander" <alexander.levin@verizon.com>,
	Sasha Levin <levinsasha928@gmail.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"ksummit-discuss\@lists.linuxfoundation.org" 
	<ksummit-discuss@lists.linuxfoundation.org>
Subject: Re: checkkpatch (in)sanity ?
Date: Mon, 29 Aug 2016 14:15:10 +0300	[thread overview]
Message-ID: <874m634yip.fsf@purkki.adurom.net> (raw)
In-Reply-To: <1472348579.26978.47.camel@perches.com> (Joe Perches's message of "Sat, 27 Aug 2016 18:42:59 -0700")

Joe Perches <joe@perches.com> writes:

> On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
>> On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
>> > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
>> > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
>> > > > 
>> > > >     - Making checkpatch check for (some) of the stable kernel rules
>> > > >     (and possibly recommend adding the stable@ tag in certain cases?).
>> > > >       - Depends on: making checkpatch sane again
>> > > > >This sounds interesting.  What do you mean by "sane"?
>> > Sasha, can you expand your thoughts here please?
>> Sure. I have 2.5 concerns about the state of checkpatch:
> []
>> > Most all of the trivial spacing stuff can easily be
>> > ignored either by a human determining what's important
>> > or by using command line options like --ignore=spacing
>> 1.
>> This is the wrong default. By default checkpatch shouldn't be showing trivial
>> issues that encourage folks to try and work around them and as a result
>> produce worse code.
>> 
>> Look at the 80 character limit warning for example, what good does it do?
>
> That argument's been done several times. It keeps Linus happy.
> I don't care one way or another.
>
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

I wish that checkpatch would have a way to enable/disable warnings per
directory (or file). For example, there would be
drivers/net/wireless/ath/ath10k/.checkpatch which would disable the
warnings are not suitable for ath10k for one reason or another:

'MSLEEP',
'USLEEP_RANGE',
'PRINTK_WITHOUT_KERN_LEVEL',
'NETWORKING_BLOCK_COMMENT_STYLE',
'BLOCK_COMMENT_STYLE',
'LINUX_VERSION_CODE',
'COMPLEX_MACRO',
'PREFER_DEV_LEVEL',
'PREFER_PR_LEVEL',
'COMPARISON_TO_NULL',
'BIT_MACRO',
'CONSTANT_COMPARISON',
'MACRO_WITH_FLOW_CONTROL'

Currently my workaround is to have a custom ath10k-check script[1] which
runs checkpatch with those checks disabled. Oh, and it also filters out
some of the warnings based on the symbol it is located in.

https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check

-- 
Kalle Valo