linux-kernel - Re: checkkpatch (in)sanity ?

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: <874m634yip.fsf@purkki.adurom.net>
Date:   Mon, 29 Aug 2016 14:15:10 +0300
From:   Kalle Valo <kvalo@...eaurora.org>
To:     Joe Perches <joe@...ches.com>
Cc:     "Levin\, Alexander" <alexander.levin@...izon.com>,
        Sasha Levin <levinsasha928@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "ksummit-discuss\@lists.linuxfoundation.org" 
        <ksummit-discuss@...ts.linuxfoundation.org>
Subject: Re: checkkpatch (in)sanity ?

Joe Perches <joe@...ches.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