Message-ID: <20231023184411.73919423@kernel.org>
Date: Mon, 23 Oct 2023 18:44:11 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Philip Li <philip.li@...el.com>
Cc: "Nambiar, Amritha" <amritha.nambiar@...el.com>,
 <oe-kbuild-all@...ts.linux.dev>, kernel test robot <lkp@...el.com>,
 <netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [net-next PATCH v5 01/10] netdev-genl: spec: Extend netdev
 netlink spec in YAML for queue

On Tue, 24 Oct 2023 09:02:46 +0800 Philip Li wrote:
> > I understand and appreciate the effort. 
> > 
> > I think that false positive has about a 100x the negative effect of a
> > true positive. If more than 1% of checkpatch warnings are ignored, we
> > should *not* report them to the list. Currently in networking we fully
> > trust the build bot and as soon as a patch set gets a reply from you it
> > gets auto-dropped from our review queue.  
> 
> Thanks for the trust. Sorry I didn't notice the false checkpatch report leads
> to trouble. From below info, may i understand networking already runs own
> checkpatch? Also consider the checkpatch reports from bot still contains quite
> some false ones, probably we can pause the checkpatch reporting for network
> side if it doesn't add much value and causes trouble?

Yes, correct, we already run checkpatch --strict on all patches.

If you have the ability to selectively disable checkpatch for net/ and
drivers/net, and/or patches which CC netdev@...r, that'd be great!


FWIW we have a simple dashboard reporting which checks in our own
local build fail the most: https://netdev.bots.linux.dev/checks.html
Not sure if it's of any interest to you, but that's where I got the
false positive rate I mentioned previously.

> > And the maintainer is not very receptive to improvements for false
> > positives:
> > https://lore.kernel.org/all/20231013172739.1113964-1-kuba@kernel.org/  
> 
> I see. We got this pattern as well, what we do now is to maintain the pattern
> internally to avoid unnecessary reports (some are extracted below). I'm looking
> for publishing these patterns later, which may get more inputs to filter out
> unnecessary reports.
> 
> == part of low confidence patterns of checkpatch in bot ==

Interesting!

> __func__ should be used instead of gcc specific __FUNCTION__

This one I don't see failing often.

> line over 80 characters

This one happens a lot, yes.

> LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged

This is very rare upstream.

> Missing commit description - Add an appropriate one

Should be rare upstream..

> please write a help paragraph that fully describes the config symbol

This check I think is semi-broken in checkpatch.
Sometimes it just doesn't recognize the help even if symbol has it.
So yes, we see if false-positive as well.

> Possible repeated word: 'Google'

Yes! :)

> Possible unwrapped commit description \(prefer a maximum 75 chars per line\)

This one indeed has a lot of false positives. It should check if
*majority* of the commit message lines (excluding tags) are too long,
not any single line. Because one line can be a crash dump or a commit
reference, and be longer for legit reasons..

Every now and then I feel like we should fork checkpatch or start a new
tool which would report only high-confidence problems.

> > > But as you mentioned above, we will take furture care to the output
> > > of checkpatch to be conservative for the reporting.  
> > 
> > FWIW the most issues that "get through" in networking are issues 
> > in documentation (warnings for make htmldocs) :(  
> 
> Do you suggest that warnings for make htmldocs or kernel-doc warning when building
> with W=1 can be ignored and no need to send them to networking side?

No, no, the opposite! Documentation is one part we currently don't test,
even tho we should.

Do you run make htmldocs as part of kernel build bot? As you allude to -
W=1 checks kdoc already, and scripts/kernel-doc can be used to validate
headers even more easily. But to validate the ReST files under
Documentation/ one has to actually run make htmldocs (or perhaps some
other docs target), not just a normal build.

Powered by blists - more mailing lists