Message-ID: <ZTclVfX5mtWXziSW@rli9-mobl>
Date: Tue, 24 Oct 2023 10:00:53 +0800
From: Philip Li <philip.li@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
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 Mon, Oct 23, 2023 at 06:44:11PM -0700, Jakub Kicinski wrote:
> 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!

Got it, thanks for the detail info, we will pause the reports for these
places. Before that, i will also pause the overall checkpatch check until
we have resolved this for networking side.

> 
> 
> 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.

This is very advanced and clear! Thanks for sharing, let me dig into
it to learn from the dashboard.

> 
> > > 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..

Thanks for all above comments/analysis/experience, which brings a lot insights.

> 
> 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 -

yes, the bot runs make htmldocs check as part of various checks such as
includecheck, dtcheck, etc.

We will continue doing this for networking side.

> 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