linux-kernel - Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

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]

Date:   Wed, 17 Apr 2019 13:39:36 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Alex Elder <elder@...aro.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Madhumitha Prabakaran <madhumithabiw@...il.com>,
        devel@...verdev.osuosl.org, greybus-dev@...ts.linaro.org,
        elder@...nel.org, johan@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus
 driver

On Wed, Apr 17, 2019 at 06:19:50AM -0500, Alex Elder wrote:

> I'm not completely sure about the inline function, but on the no blank
> lines thing (and many other minor issues) "checkpatch.pl" is to blame.
> There are lots of examples of issues that checkpatch points out that are
> matters of opinion and not hardened kernel style rules.
> 
> We try to encourage people to get involved with kernel development by
> fixing minor problems, and we tell them a good way to find them is
> by running checkpatch and "fixing" what it reports.  Unfortunately,
> it is often things of this type, and reviewers balk and say "no,
> please leave it," and the poor new person has a bad first experience.
> 
> I *like* "checkpatch.pl".  And the fact that it can point out some
> of these sorts of things is great.  But it would be nice if certain
> types of problems (like multiple blank lines, or lines that are 81
> characters wide for example) would only be reported when a "--strict"
> option or something were supplied.

The problem is that --strict is enabled by default when running
checkpatch on code in staging (and net).

And it has all sorts of weird tests (prefixed as "CHECK" rather than
"WARNING") to catch everyone and their mom's pet peeve.

I don't think the intention ever was that all those should be "fixed",
but this appears to be where this checkpatch mission creep comes from
(and we're now seeing --strict being used on code outside of staging
too).

IMO we're setting a bad example for new contributers by accepting such
changes by default. Blindly trusting a tool is not how kernel
development works, but that seems to be the message currently sent.

Johan