checkkpatch (in)sanity ?

checkkpatch (in)sanity ?

[Joe Perches]

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?

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

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

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:

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: 
> 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? It
encourages people to do even stupider things to work around it and results in
a bunch of "fix checkpatch warning" that touch existing code just to make the
result harder to read and make 'git blame' harder to work with.

By default you should only get the most critical warnings we have in the
kernel like missing S-O-B or corrupt patch.


2. A "who wrote these rules?": there seems to be a disconnect between the rules
checkpatch is trying to enforce and the accepted coding style enforced by
maintainers. 

Do a git-format-patch on all of the commits Linus authored in the past year or
two and see how many of them fail checkpatch (or do the same for any of the
commits that passed through and were accepted by the top maintainers),
according to checkpatch we need to make those guys stop touching the kernel.


3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
perl code that a fair amount of people don't know how to deal with. In 4.8 it's
6142 lines, making it the 124th largest source file in the kernel, well within
the top 1% of source files in the kernel.

This combination of size/language pushes people away from being involved in
what is supposed to be a central tool and gives them a reason to never use
it again after they see results they don't agree with (rather than fixing it).

-- 

Thanks,
Sasha

Re: checkkpatch (in)sanity ?

[Joe Perches]

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 still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
would be a good change.

https://lkml.org/lkml/2015/7/16/568

>  It
> encourages people to do even stupider things to work around it and results in
> a bunch of "fix checkpatch warning" that touch existing code just to make the
> result harder to read and make 'git blame' harder to work with.

Almost all of the crud in git-blame can be avoided with -w

> By default you should only get the most critical warnings we have in the
> kernel like missing S-O-B or corrupt patch.

I don't think so, but if you do, add a filter for ERROR only.

> 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> checkpatch is trying to enforce and the accepted coding style enforced by
> maintainers. 

Name some please.

> Do a git-format-patch on all of the commits Linus authored in the past year or
> two and see how many of them fail checkpatch (or do the same for any of the
> commits that passed through and were accepted by the top maintainers),
> according to checkpatch we need to make those guys stop touching the kernel.

Try it yourself and tell me what's wrong with the messages:

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
  grep -v " Linux [34]" | \
  while read commit ; do \
    echo $commit ; \
    git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
      ./scripts/checkpatch.pl - ; \
  done

Here's a summary done with an additional

  grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
  sort |uniq -c | sort -rn

     46 WARNING:LONG_LINE_COMMENT
     45 WARNING:LEADING_SPACE
     37 WARNING:LONG_LINE
     16 ERROR:GIT_COMMIT_ID
     11 WARNING:COMMIT_LOG_LONG_LINE
      5 WARNING:BRACES
      2 WARNING:BAD_SIGN_OFF
      2 WARNING:AVOID_BUG
      2 ERROR:SPACING
      1 WARNING:SPLIT_STRING
      1 WARNING:FILE_PATH_CHANGES
      1 WARNING:ENOSYS
      1 ERROR:MISSING_SIGN_OFF

> 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> 6142 lines, making it the 124th largest source file in the kernel, well within
> the top 1% of source files in the kernel.
> 
> This combination of size/language pushes people away from being involved in
> what is supposed to be a central tool and gives them a reason to never use
> it again after they see results they don't agree with (rather than fixing it).

Meh, I'm not a perl guy either.

I think almost all of it is regexes and most people
aren't very good at those.

So it wouldn't matter if it was perl or python.

spatch isn't the right tool.

What would you suggest instead?

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sat, 2016-08-27 at 18:42 -0700, Joe Perches wrote:

>      46 WARNING:LONG_LINE_COMMENT
>      45 WARNING:LEADING_SPACE
>      37 WARNING:LONG_LINE
>      16 ERROR:GIT_COMMIT_ID
>      11 WARNING:COMMIT_LOG_LONG_LINE
>       5 WARNING:BRACES
>       2 WARNING:BAD_SIGN_OFF
>       2 WARNING:AVOID_BUG
>       2 ERROR:SPACING
>       1 WARNING:SPLIT_STRING
>       1 WARNING:FILE_PATH_CHANGES
>       1 WARNING:ENOSYS
>       1 ERROR:MISSING_SIGN_OFF

My copy/pasta mistake, the above was Ingo Molnar's list

Here's Linus'

     37 WARNING:LONG_LINE
     19 ERROR:GIT_COMMIT_ID
      8 WARNING:PREFER_PR_LEVEL
      6 ERROR:SPACING
      4 WARNING:MACRO_WITH_FLOW_CONTROL
      3 WARNING:COMMIT_LOG_LONG_LINE
      2 WARNING:TYPO_SPELLING
      2 WARNING:BAD_SIGN_OFF
      2 ERROR:TRAILING_STATEMENTS
      1 WARNING:NEW_TYPEDEFS
      1 WARNING:LONG_LINE_COMMENT
      1 WARNING:LINE_SPACING
      1 WARNING:CONSTANT_COMPARISON
      1 WARNING:AVOID_BUG
      1 ERROR:STABLE_ADDRESS

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sat, Aug 27, 2016 at 09:42:59PM -0400, Joe Perches wrote:
> 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'm not trying to be specific with the 80 character thing, it's also true for
a few other things that makes people produce less readable code than what it
would have looked like if they'd ignore the warning.
 
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

That makes sense to you, but it doesn't make sense to the newer folks who are
told not to submit any patches with checkpatch errors/warnings. You know to
ignore these 80-character warnings when it makes sense, they see it as "you
must make the warning disappear no matter what".
 
> I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
> would be a good change.
> 
> https://lkml.org/lkml/2015/7/16/568

Probably. Would you agree that by default we shouldn't show anything that's
not an error/defect?

> >  It
> > encourages people to do even stupider things to work around it and results in
> > a bunch of "fix checkpatch warning" that touch existing code just to make the
> > result harder to read and make 'git blame' harder to work with.
> 
> Almost all of the crud in git-blame can be avoided with -w

That doesn't deal with newlines people add to hide the 80 character stuff, nor it
deals with the "harder to read" part.

> > By default you should only get the most critical warnings we have in the
> > kernel like missing S-O-B or corrupt patch.
> 
> I don't think so, but if you do, add a filter for ERROR only.

I could, but the problem is the people who see the default output as "holy".

> > 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> > checkpatch is trying to enforce and the accepted coding style enforced by
> > maintainers. 
> 
> Name some please.

Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID). checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg didn't get the memo.

> > Do a git-format-patch on all of the commits Linus authored in the past year or
> > two and see how many of them fail checkpatch (or do the same for any of the
> > commits that passed through and were accepted by the top maintainers),
> > according to checkpatch we need to make those guys stop touching the kernel.
> 
> Try it yourself and tell me what's wrong with the messages:
> 
> $ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
>   grep -v " Linux [34]" | \
>   while read commit ; do \
>     echo $commit ; \
>     git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
>       ./scripts/checkpatch.pl - ; \
>   done
> 
> Here's a summary done with an additional
> 
>   grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
>   sort |uniq -c | sort -rn
> 
>      46 WARNING:LONG_LINE_COMMENT
>      45 WARNING:LEADING_SPACE
>      37 WARNING:LONG_LINE
>      16 ERROR:GIT_COMMIT_ID
>      11 WARNING:COMMIT_LOG_LONG_LINE
>       5 WARNING:BRACES
>       2 WARNING:BAD_SIGN_OFF
>       2 WARNING:AVOID_BUG
>       2 ERROR:SPACING
>       1 WARNING:SPLIT_STRING
>       1 WARNING:FILE_PATH_CHANGES
>       1 WARNING:ENOSYS
>       1 ERROR:MISSING_SIGN_OFF

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | grep -v " Linux [34]" | wc -l
64

Linus has more errors/warnings than commits. Why do we let him commit stuff?

> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> > 6142 lines, making it the 124th largest source file in the kernel, well within
> > the top 1% of source files in the kernel.
> > 
> > This combination of size/language pushes people away from being involved in
> > what is supposed to be a central tool and gives them a reason to never use
> > it again after they see results they don't agree with (rather than fixing it).
> 
> Meh, I'm not a perl guy either.
> 
> I think almost all of it is regexes and most people
> aren't very good at those.
> 
> So it wouldn't matter if it was perl or python.
> 
> spatch isn't the right tool.
> 
> What would you suggest instead?

This is a good topic to talk about, making checkpatch accessible to us
commoners could be useful, we just need to figure out how.

-- 

Thanks,
Sasha

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:

> Would you agree that by default we shouldn't show anything that's
> not an error/defect?

Not particularly, no.

> That doesn't deal with newlines people add to hide the 80 character stuff, nor it
> deals with the "harder to read" part.

Harder to read is almost all habituation.

80 columns is simply silly when dealing with either
long identifiers or many levels of indentation.

One thing that 80 column limit does do is encourage
shorter identifiers and fewer levels of indentation.

Generally, both of those are good things.

> > > By default you should only get the most critical warnings we have in the
> > > kernel like missing S-O-B or corrupt patch.
> > I don't think so, but if you do, add a filter for ERROR only.
> I could, but the problem is the people who see the default output as "holy".

Personally, I think the "my first kernel patch" beginners were
overly encouraged to produce these checkpatch whitespace type
changes by a couple things:

o Greg KH's TuxRadar article back in 2010
  http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
o The Eudyptula Challenge
  http://eudyptula-challenge.org/

I don't know if the Eudyptula scripts are specific to
drivers/staging and most of those beginners haven't read his
email from 2015 that essentially says "don't do that" on
anything other than drivers/staging.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-July/014699.html

Outreach is hard.  Those efforts were perhaps worthwhile, but
has even a single productive kernel developer been produced
from one of those two outreach efforts?

> > > 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> > > checkpatch is trying to enforce and the accepted coding style enforced by
> > > maintainers. 
> > Name some please.
> Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID).
> checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg didn't get the memo.

That bit is in Documentation/SubmttingPatches

12 was Linus' length after the original 7 was too short.
12 will still be long enough for a few years yet.
https://lkml.org/lkml/2010/10/28/287

> > I think almost all of it is regexes and most people
> > aren't very good at those.
> > 
> > So it wouldn't matter if it was perl or python.
> > 
> > spatch isn't the right tool.
> > 
> > What would you suggest instead?
> This is a good topic to talk about, making checkpatch accessible to us
> commoners could be useful, we just need to figure out how.

I'm not sure that matters much at all.

I'm sure if you tried, you could produce that
"ERRORS" only patch for checkpatch.

There would still be some issues categorizing the
various tests at the appropriate level to taste.

Re: checkkpatch (in)sanity ?

[Greg KH]

On Sun, Aug 28, 2016 at 10:15:57AM -0700, Joe Perches wrote:
> On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > > > By default you should only get the most critical warnings we have in the
> > > > kernel like missing S-O-B or corrupt patch.
> > > I don't think so, but if you do, add a filter for ERROR only.
> > I could, but the problem is the people who see the default output as "holy".
> 
> Personally, I think the "my first kernel patch" beginners were
> overly encouraged to produce these checkpatch whitespace type
> changes by a couple things:
> 
> o Greg KH's TuxRadar article back in 2010
>   http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
> o The Eudyptula Challenge
>   http://eudyptula-challenge.org/
> 
> I don't know if the Eudyptula scripts are specific to
> drivers/staging and most of those beginners haven't read his
> email from 2015 that essentially says "don't do that" on
> anything other than drivers/staging.

I have been assured that Eudyptula says to stick only with
drivers/staging/  If anyone knows otherwise, please let me know and I
will work to resolve that.

thanks,

greg k-h

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> 
> > Would you agree that by default we shouldn't show anything that's
> > not an error/defect?
> 
> Not particularly, no.

I think that we need to figure out this disagreement first then. My claim is that checkpatch's output isn't useful.

Based on your bash snippet, populated with the KS program committee + the first few maintainers I spotted on 'git log':

commiter	commits		issues
arnd		858		2155
axboe		53		22
corbet		15		9
davem		55		81
grant.likely	2		0
gregkh		38 	 	46
hch 	 	393 	 	581
James.Bottomley	15 	 	15
martin.petersen	18 	 	20
mchehab 	678 		1042
mgorman 	104 		256
mingo 	 	58 		192
paulmck 	176 		68
peterz 	 	226 		511
rostedt 	123 		178
shuahkh 	53 		6
tglx 	 	200 		287
torvalds 	64 		89
tytso 		37 		77
viro 	 	350	 	256

And for the last 10,000 commits in the log, that script has observed 10,783 issues.

It'll be interesting to hear from these people about their view of checkpatch, but IMO when on average there are more issues than commits I can suggest two possible causes:

 1. People are used to ignore checkpatch warnings.
 2. People aren't using checkpatch.

Can you really make the claim that this is how checkpatch is supposed to be working?

-- 

Thanks,
Sasha

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sun, 2016-08-28 at 18:37 -0400, Levin, Alexander wrote:
> On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> > On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > > Would you agree that by default we shouldn't show anything that's
> > > not an error/defect?
> > Not particularly, no.
> I think that we need to figure out this disagreement first then. My
> claim is that checkpatch's output isn't useful.
[]
> It'll be interesting to hear from these people about their view of
> checkpatch, but IMO when on average there are more issues than commits
> I can suggest two possible causes:
> 
>  1. People are used to ignore checkpatch warnings.
>  2. People aren't using checkpatch.
> 
> Can you really make the claim that this is how checkpatch is supposed
> to be working?

<shrug>.  I make no particular claims about checkpatch.

I think checkpatch isn't particularly useful for those
thoroughly inculcated in what style the kernel uses and
is more useful for infrequent or new submitters.

The long time submitters and key maintainers are already
pretty consistent about coding style.

It would be good to examine the specific messages though.

For instance, Thomas Gleixner's messages for 200 commits:

     81 WARNING:COMMIT_LOG_LONG_LINE
     71 WARNING:BAD_SIGN_OFF
     37 WARNING:LONG_LINE
     22 WARNING:AVOID_BUG
     17 ERROR:SPACING
     16 WARNING:TYPO_SPELLING
     13 WARNING:UNSPECIFIED_INT
      7 ERROR:GIT_COMMIT_ID
      6 WARNING:MINMAX
      2 WARNING:PREFER_PR_LEVEL
      2 WARNING:LONG_LINE_COMMENT
      2 WARNING:LEADING_SPACE
      2 WARNING:FILE_PATH_CHANGES
      2 ERROR:CODE_INDENT
      1 WARNING:SUSPECT_CODE_INDENT
      1 WARNING:SPACING
      1 WARNING:ONE_SEMICOLON
      1 WARNING:LINE_SPACING
      1 WARNING:BLOCK_COMMENT_STYLE
      1 ERROR:TRAILING_STATEMENTS
      1 ERROR:INITIALISED_STATIC
      1 CHECK:PARENTHESIS_ALIGNMENT

28 ERRORs and a lot more WARNINGs

It seems that most of the BAD_SIGN_OFF uses are where
he signed his original patches and then did something
like git-am -s on those same patches.

The COMMIT_LOG_LONG_LINE messages are almost all just
slightly over the generic 80 column output when used
with just "git log".

I think the rest of the messages are reasonable and
generally follow CodingStyle.

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sun, Aug 28, 2016 at 07:20:52PM -0400, Joe Perches wrote:
> On Sun, 2016-08-28 at 18:37 -0400, Levin, Alexander wrote:
> > On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> > > On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > > > Would you agree that by default we shouldn't show anything that's
> > > > not an error/defect?
> > > Not particularly, no.
> > I think that we need to figure out this disagreement first then. My
> > claim is that checkpatch's output isn't useful.
> []
> > It'll be interesting to hear from these people about their view of
> > checkpatch, but IMO when on average there are more issues than commits
> > I can suggest two possible causes:
> > 
> >  1. People are used to ignore checkpatch warnings.
> >  2. People aren't using checkpatch.
> > 
> > Can you really make the claim that this is how checkpatch is supposed
> > to be working?
> 
> <shrug>.  I make no particular claims about checkpatch.
> 
> I think checkpatch isn't particularly useful for those
> thoroughly inculcated in what style the kernel uses and
> is more useful for infrequent or new submitters.
> 
> The long time submitters and key maintainers are already
> pretty consistent about coding style.

I did the same test for authors of 5-9 commits (just an arbitrary choice of numbers for "infrequent"), the results there are much worse: 3981 commits, 7175 issues.

The only big subsystem that seems to be forcing checkpatch "correctness" is mm/, where akpm is fixing up checkpatch issues himself. Otherwise, it looks like maintainers are not running checkpatch nor are making sure that the commits they merge in don't have checkpatch issues.

> It would be good to examine the specific messages though.

What for? The point is that with that amount of issues it's evident that people don't actually use checkpatch to begin with. We can discuss whether the output it produces makes sense all we want, but the fact is that people just don't use it - and I've tried to give my opinion of why I think it happens.

-- 

Thanks,
Sasha

Re: checkkpatch (in)sanity ?

[Christoph Hellwig]

Seriously folks, checkpatch is a tool that's to be used for a reason,
not a reason by itself.

And it used to be a lot more useful before adding all kinds of bullshit
warnings.  I use checkpatch a lot, and I also ignore silly warnings in
it a lot as it's piling up more and more crap.

And then again there are just plenty of commits that touch existing code
for a minor change, and I'm not going to bother rewriting the steaming
pile of crap around it.  And checkpath really shouldn't even bother to
warn for context of diffs that's not even touched by default.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Alexandre Belloni]

On 28/08/2016 at 18:37:59 -0400, Levin, Alexander via Ksummit-discuss wrote :
> On Sun, Aug 28, 2016 at 01:15:57PM -0400, Joe Perches wrote:
> > On Sat, 2016-08-27 at 22:47 -0400, Levin, Alexander wrote:
> > 
> > > Would you agree that by default we shouldn't show anything that's
> > > not an error/defect?
> > 
> > Not particularly, no.
> 
> I think that we need to figure out this disagreement first then. My claim is that checkpatch's output isn't useful.
> 
> Based on your bash snippet, populated with the KS program committee + the first few maintainers I spotted on 'git log':
> 
> commiter	commits		issues
> arnd		858		2155
> axboe		53		22
> corbet		15		9
> davem		55		81
> grant.likely	2		0
> gregkh		38 	 	46
> hch 	 	393 	 	581
> James.Bottomley	15 	 	15
> martin.petersen	18 	 	20
> mchehab 	678 		1042
> mgorman 	104 		256
> mingo 	 	58 		192
> paulmck 	176 		68
> peterz 	 	226 		511
> rostedt 	123 		178
> shuahkh 	53 		6
> tglx 	 	200 		287
> torvalds 	64 		89
> tytso 		37 		77
> viro 	 	350	 	256
> 
> And for the last 10,000 commits in the log, that script has observed 10,783 issues.
> 
> It'll be interesting to hear from these people about their view of checkpatch, but IMO when on average there are more issues than commits I can suggest two possible causes:
> 
>  1. People are used to ignore checkpatch warnings.
>  2. People aren't using checkpatch.
> 

Well, Arnd is used to move around old code when refactoring. As the code
just moves, he rarely solves checkpatch issues when doing so which is
the right thing to do.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Arnd Bergmann]

On Monday, August 29, 2016 9:15:15 AM CEST Alexandre Belloni wrote:
> > 
> > commiter      commits         issues
> > arnd          858             2155
> > axboe         53              22
> > corbet                15              9
> > davem         55              81
> > grant.likely  2               0
> > gregkh                38              46
> > hch           393             581
> > James.Bottomley       15              15
> > martin.petersen       18              20
> > mchehab       678             1042
> > mgorman       104             256
> > mingo                 58              192
> > paulmck       176             68
> > peterz                226             511
> > rostedt       123             178
> > shuahkh       53              6
> > tglx          200             287
> > torvalds      64              89
> > tytso                 37              77
> > viro          350             256
> > 
> > And for the last 10,000 commits in the log, that script has observed 10,783 issues.
> > 
> > It'll be interesting to hear from these people about their view of checkpatch, but IMO when on average there are more issues than commits I can suggest two possible causes:
> > 
> >  1. People are used to ignore checkpatch warnings.
> >  2. People aren't using checkpatch.
> > 
> 
> Well, Arnd is used to move around old code when refactoring. As the code
> just moves, he rarely solves checkpatch issues when doing so which is
> the right thing to do.

I don't find checkpatch.pl overly useful for my own patches and rarely
run it. I looked over the last few hundred commits and found that almost
all the warnings were for:

- having overly long lines in commit messages when I quoted a long
  compiler warning. I generally don't wrap those to make it easier to
  search for the warnings in the git history

- missing the word "commit" before a reference to another changeset
  in full-text. I'll change that in the future if that makes people
  happy, but it doesn't seem important.

- existing style issues that I did not fix when fixing a bug. In many
  cases I find it better not to change coding style while fixing
  a bug, but there are other cases in which I do.

	Arnd

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 11:01 +0200, Arnd Bergmann wrote: 

> I don't find checkpatch.pl overly useful for my own patches and rarely
> run it.

I mostly run checkpatch to test new checkpatch rules.

I generally don't run it on my own patches, mostly out
of possibly misplaced confidence in my own adherence to
the nominal kernel style.  It sometimes leads to mild
regret over things like whitespace defects.

I get over it quickly.

But I also think checkpatch's overall false positive
reporting rate is relatively low.  Most all of what
it does to report possible defects is nominally correct.

If anyone has examples of bad reporting by checkpatch,
please send it.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Josh Triplett]

On Mon, Aug 29, 2016 at 05:47:59AM -0700, Joe Perches wrote:
> I generally don't run it on my own patches, mostly out
> of possibly misplaced confidence in my own adherence to
> the nominal kernel style.  It sometimes leads to mild
> regret over things like whitespace defects.

In the specific case of whitespace defects, does git diff not catch
those?

For that matter, should we add a .gitattributes file to the kernel
enabling additional whitespace errors git knows how to catch?  git.git
has such a file.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 10:16 -0700, Josh Triplett wrote:
> On Mon, Aug 29, 2016 at 05:47:59AM -0700, Joe Perches wrote:
> > 
> > I generally don't run it on my own patches, mostly out
> > of possibly misplaced confidence in my own adherence to
> > the nominal kernel style.  It sometimes leads to mild
> > regret over things like whitespace defects.
> In the specific case of whitespace defects, does git diff not catch
> those?
> 
> For that matter, should we add a .gitattributes file to the kernel
> enabling additional whitespace errors git knows how to catch?  git.git
> has such a file.

For reference:
    https://git-scm.com/docs/gitattributes
https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration
and here's the git.git .gitattributes file:
    $ cat .gitattributes 
    * whitespace=!indent,trail,space
    *.[ch] whitespace=indent,trail,space diff=cpp
    *.sh whitespace=indent,trail,space

Using something like that for kernel git would be a
good idea for trailing whitespace and space before HT.

But aren't those the generic git default options now?

RE: [Ksummit-discuss] checkkpatch (in)sanity ?

[Luck, Tony]

> 80 columns is simply silly when dealing with either
> long identifiers or many levels of indentation.
>
> One thing that 80 column limit does do is encourage
> shorter identifiers and fewer levels of indentation.
>
> Generally, both of those are good things.

I think the main complaint with the limit is that people fix it by simply
breaking the long line, which often makes for less readable code.

Perhaps there would be less pushback on this if checkpatch also
complained about clumsily broken long lines and offered the advice
to restructure the code with helper functions etc. to avoid deep
indentation?

FWIW I do find checkpatch is helpful enough with useful tips
that it has value even when it generates some noise. Generally
the better you are at conforming to kernel style, the more irritating
it will be, because you only see the questionable output. For
newbies, and less frequent contributors (especially those who
work on other projects with other style guides) it is likely still
doing a good job.

In the journey from 4.6 to 4.7 we had 13433 commits. 2258 (16%)
from people with 5 or fewer commits in that release. Those are
the people most helped by checkpatch (plus the maintainers who
took those patches didn't have to spend as many cycles complaining
about style).

I think the bottom line is whether checkpatch's helpful messages do
more good than the grey area messages that cause people to make
questionable changes to shut checkpatch up.

-Tony

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > 
> > 80 columns is simply silly when dealing with either
> > long identifiers or many levels of indentation.
> > 
> > One thing that 80 column limit does do is encourage
> > shorter identifiers and fewer levels of indentation.
> > 
> > Generally, both of those are good things.
> I think the main complaint with the limit is that people fix it by simply
> breaking the long line, which often makes for less readable code.
> 
> Perhaps there would be less pushback on this if checkpatch also
> complained about clumsily broken long lines and offered the advice
> to restructure the code with helper functions etc. to avoid deep
> indentation?

It suggests that already for 6+ leading tabs, but some more
intelligence for nominally ugly added line breaks would
definitely help.

Using longish simple identifiers or multiple dereferences
can make the line breaks at 80 columns silly.

Simple things like:

			if (longish_identifier != AN_EVEN_LONGER_DEFINED_CONSTANT_VALUE)
and
			if (some_pointer->member[index].another_member >> shift_constant)

shouldn't really ever be broken into multiple lines,
but I see that submitted by some names I haven't seen
before all the time.

It's not an easy problem to solve with regexes though.

> FWIW I do find checkpatch is helpful enough with useful tips
> that it has value even when it generates some noise. Generally
> the better you are at conforming to kernel style, the more irritating
> it will be, because you only see the questionable output. For
> newbies, and less frequent contributors (especially those who
> work on other projects with other style guides) it is likely still
> doing a good job.
> 
> In the journey from 4.6 to 4.7 we had 13433 commits. 2258 (16%)
> from people with 5 or fewer commits in that release. Those are
> the people most helped by checkpatch (plus the maintainers who
> took those patches didn't have to spend as many cycles complaining
> about style).
> 
> I think the bottom line is whether checkpatch's helpful messages do
> more good than the grey area messages that cause people to make
> questionable changes to shut checkpatch up.
> 
> -Tony

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 11:01 -0700, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > [checkpatch] offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> It suggests that already for 6+ leading tabs,

And here's an inexact little histogram of the code that
expands indent levels in the -next kernel source tree.

$ grep -rP --include=*.[ch] -oh "^[\t]+(do|while|for|if|else|return|goto|continue|switch|default|case|break)\b" * | \
  sed -r 's/^(\t+).*$/\1/' | awk '{print length($0)}' | sort -n | uniq -c
1217165 1
 783085 2
 249655 3
  59775 4
  11653 5
   1993 6
    444 7
    158 8
     50 9
     19 10
     10 11
      4 12
      1 13

Some of that code, as Linus once put it, is eye-gouging.
Luckily, almost all of the 7+ tab indent code is prehistoric.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Josh Triplett]

On Mon, Aug 29, 2016 at 11:01:40AM -0700, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > > 
> > > 80 columns is simply silly when dealing with either
> > > long identifiers or many levels of indentation.
> > > 
> > > One thing that 80 column limit does do is encourage
> > > shorter identifiers and fewer levels of indentation.
> > > 
> > > Generally, both of those are good things.
> > I think the main complaint with the limit is that people fix it by simply
> > breaking the long line, which often makes for less readable code.
> > 
> > Perhaps there would be less pushback on this if checkpatch also
> > complained about clumsily broken long lines and offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> 
> It suggests that already for 6+ leading tabs, but some more
> intelligence for nominally ugly added line breaks would
> definitely help.
> 
> Using longish simple identifiers or multiple dereferences
> can make the line breaks at 80 columns silly.
> 
> Simple things like:
> 
> 			if (longish_identifier != AN_EVEN_LONGER_DEFINED_CONSTANT_VALUE)
> and
> 			if (some_pointer->member[index].another_member >> shift_constant)
> 
> shouldn't really ever be broken into multiple lines,

Agreed.

Honestly, I almost never see a line that should break solely based on
length.  Almost any line that makes sense to break at a given point
would make sense to break at that point even with a target line length
of 200.

For instance:

			if (an_interesting_function(x) == TARGET_VALUE_FOR_X
			    || an_interesting_function(y) == TARGET_VALUE_FOR_Y) {

That line break makes sense whether you want to break lines at 80
characters, 100, or 800.  (You could argue about
before-or-after-operator, or about line alignment.)  In almost no
circumstances would you want to also break around the '==', even though
that second line takes up 82 characters.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Arnd Bergmann]

On Monday 29 August 2016, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > > 
> > > 80 columns is simply silly when dealing with either
> > > long identifiers or many levels of indentation.
> > > 
> > > One thing that 80 column limit does do is encourage
> > > shorter identifiers and fewer levels of indentation.
> > > 
> > > Generally, both of those are good things.
> > I think the main complaint with the limit is that people fix it by simply
> > breaking the long line, which often makes for less readable code.
> > 
> > Perhaps there would be less pushback on this if checkpatch also
> > complained about clumsily broken long lines and offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> 
> It suggests that already for 6+ leading tabs, but some more
> intelligence for nominally ugly added line breaks would
> definitely help.
> 
> Using longish simple identifiers or multiple dereferences
> can make the line breaks at 80 columns silly.

My preferred personal guideline for the maximum indentation is
the area that a function takes up in the editor. It's sometimes
ok to have really long functions (hundreds of lines), but only
with one or two levels of indentation. It's also sometimes ok
to have five or six levels of intendation, but only if the
function is really short and you can see immediately how
it works.

Having a long function with multiple nested loops and conditions
is almost always a problem for readability, and we should be
able to detect this programatically if we want to.

There are more accurate ways to tell if you are getting
too complex (e.g. CONFIG_GCC_PLUGIN_CYC_COMPLEXITY), but that
becomes harder to warn about.

	Arnd

Re: checkkpatch (in)sanity ?

[Kalle Valo]

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

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 14:15 +0300, Kalle Valo wrote:
> 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

Hey Kalle:

I looked at your script (which also does compilation
and sparse checking) I don't see how a .checkpatch_conf
hierarchy helps you much there as you've added all those
long symbol name long line avoidance bits.

Also, there'd be a lot of rework to the globals in
checkpatch for per-directory specific overrides if someone
fed it files in multiple directories like

checkpatch.pl <patchfile touching lib/kernel/include>

A couple btw's:

Why avoid the printk, sleep or macro tests?

And this for ath10k_core_register_work:

    ('ath10k_core_register_work', 'RETURN_VOID'),

and the code associated to it:

err:
	/* TODO: It's probably a good idea to release device from the driver
	 * but calling device_release_driver() here will cause a deadlock.
	 */
	return;
}

ia avoided a few times in the kernel by using a bare ";"
instead of "return;" before the function closing brace.

It's maybe unfortunate that gcc / c spec doesn't allow
jumping to a label just before the function close brace.

Re: checkkpatch (in)sanity ?

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Mon, 2016-08-29 at 14:15 +0300, Kalle Valo wrote:
>> 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
>
> Hey Kalle:
>
> I looked at your script (which also does compilation
> and sparse checking) I don't see how a .checkpatch_conf
> hierarchy helps you much there as you've added all those
> long symbol name long line avoidance bits.

Yeah, it would not completely replace my script. But there's now quite a
difference with checkpatch parameters what other people use and what I
use.

> Also, there'd be a lot of rework to the globals in
> checkpatch for per-directory specific overrides if someone
> fed it files in multiple directories like
>
> checkpatch.pl <patchfile touching lib/kernel/include>

Oh, that's a good point. ath10k patches only touch one directory so I
didn't think of this.

> A couple btw's:
>
> Why avoid the printk, sleep or macro tests?

Actually I don't remember anymore :) I should have documented that in
the script.

(Runs some tests)

PRINTK_WITHOUT_KERN_LEVEL: I think I can enable this again, IIRC earlier
it gave useless warnings in ath10k_warn() & co.

MSLEEP: I think this is just noise, we don't care if it's actually 20 ms
even if ask for 10 ms. That's the minimum time to wait, not the maximum
(from our/HW point of view).

drivers/net/wireless/ath/ath10k/ahb.c:422: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:427: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:432: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:437: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:442: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2244: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2251: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2275: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt

USLEEP_RANGE: I don't remember why I disabled this, most likely just
because of the msleep noise above

drivers/net/wireless/ath/ath10k/pci.c:2672: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt

COMPLEX_MACRO: Can't remember this one either, I guess I just didn't
find a good solution to shut up checkpatch. But that was a long time
ago, it might be fixable now.

drivers/net/wireless/ath/ath10k/wmi.h:315: Macros with complex values should be enclosed in parentheses
drivers/net/wireless/ath/ath10k/wmi.h:6344: Macros with complex values should be enclosed in parentheses

BIT_MACRO: there's a lot of warnings from this one, the benefit from
changing all of them is questionable so I just disabled it.

drivers/net/wireless/ath/ath10k/rx_desc.h:676: Prefer using the BIT macro

> And this for ath10k_core_register_work:
>
>     ('ath10k_core_register_work', 'RETURN_VOID'),
>
> and the code associated to it:
>
> err:
> 	/* TODO: It's probably a good idea to release device from the driver
> 	 * but calling device_release_driver() here will cause a deadlock.
> 	 */
> 	return;
> }
>
> ia avoided a few times in the kernel by using a bare ";"
> instead of "return;" before the function closing brace.

Heh, didn't know that.

> It's maybe unfortunate that gcc / c spec doesn't allow
> jumping to a label just before the function close brace.

Indeed.

I find checkpatch very useful to maintain certain coding style in ath10k
and I don't need to worry small details like whitespace. I just need to
disable some of the warnings so that they don't hide the real warnings
I'm interested about.

-- 
Kalle Valo

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 21:01 +0300, Kalle Valo wrote:

> there's now quite a
> difference with checkpatch parameters what other people use and what I
> use.
[]
> I find checkpatch very useful to maintain certain coding style in ath10k
> and I don't need to worry small details like whitespace. I just need to
> disable some of the warnings so that they don't hide the real warnings
> I'm interested about.

I don't see a conflict here.

The entire point of classifying all of those checkpatch
message types was to allow exactly what you are doing.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Arnd Bergmann]

On Monday 29 August 2016, Kalle Valo wrote:
> MSLEEP: I think this is just noise, we don't care if it's actually 20 ms
> even if ask for 10 ms. That's the minimum time to wait, not the maximum
> (from our/HW point of view).
> 
> drivers/net/wireless/ath/ath10k/ahb.c:422: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:427: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:432: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:437: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:442: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/pci.c:2244: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/pci.c:2251: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/pci.c:2275: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> 

I've seen way too many patches addressing this warning in various ways that
do not improve readability or behavior of the driver. Typically a driver
calls "msleep(1)" in a loop as a way to poll for an event that will happen
at some point in the future but that generates no IRQ or other event we
can wait for, the stuff that used to be handled with "yield" a long time
ago.

Now every third caller of usleep_range() uses '1000' as the minimum time
and an arbitrary upper bound.

	Arnd

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Alexey Dobriyan]

On Sat, Aug 27, 2016 at 09:06:13PM -0400, Levin, Alexander via Ksummit-discuss wrote:
> 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> 6142 lines, making it the 124th largest source file in the kernel, well within
> the top 1% of source files in the kernel.
> 
> This combination of size/language pushes people away from being involved in
> what is supposed to be a central tool and gives them a reason to never use
> it again after they see results they don't agree with (rather than fixing it).

It is a textbook example of what's wrong with Perl. Instead of parsing
C code like compilers do, the script is one big pile of regexes.
It mostly works ("doing its job" in perlspeak) because people mostly
follow the coding style.

Regarding individual warnings: some are good (RETURN_VOID, DATE_TIME,
USE_NEGATIVE_ERRNO), some are OK given kernel style of allocating memory
but the rationale is bogus (UNNECESSARY_CASTS, linking to userspace
example of malloc() returning "int"!).

And then there is ALLOC_SIZEOF_STRUCT which advocates "kmalloc(sizeof(*p))".

The problem is that c-h.pl generates noise in the commit history and
makes git-blame less useful than it can be.

I for one given up on it more or less since its introduction.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Julia Lawall]

On Sun, 28 Aug 2016, Alexey Dobriyan wrote:

> On Sat, Aug 27, 2016 at 09:06:13PM -0400, Levin, Alexander via Ksummit-discuss wrote:
> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> > 6142 lines, making it the 124th largest source file in the kernel, well within
> > the top 1% of source files in the kernel.
> >
> > This combination of size/language pushes people away from being involved in
> > what is supposed to be a central tool and gives them a reason to never use
> > it again after they see results they don't agree with (rather than fixing it).
>
> It is a textbook example of what's wrong with Perl. Instead of parsing
> C code like compilers do, the script is one big pile of regexes.
> It mostly works ("doing its job" in perlspeak) because people mostly
> follow the coding style.

Parsing is slow.  Perfect parsing is impossible due to configuration
options.  There is definitely a place for regexps in checking code.
Perhaps there is a better way to express the regexps, or to provide
regexps for integration into the checkpatch infrastructure.

> Regarding individual warnings: some are good (RETURN_VOID, DATE_TIME,
> USE_NEGATIVE_ERRNO), some are OK given kernel style of allocating memory
> but the rationale is bogus (UNNECESSARY_CASTS, linking to userspace
> example of malloc() returning "int"!).
>
> And then there is ALLOC_SIZEOF_STRUCT which advocates "kmalloc(sizeof(*p))".
>
> The problem is that c-h.pl generates noise in the commit history and
> makes git-blame less useful than it can be.

Could it be that this is a problem with git blame, rather than with
checkpatch?  Last year there was a discussion on this list about how there
is an option to git blame that will cause it to step through the history,
and not show only the most recent patch that has modified a given line.

julia

>
> I for one given up on it more or less since its introduction.
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Sun, 2016-08-28 at 11:59 +0200, Julia Lawall wrote:
> On Sun, 28 Aug 2016, Alexey Dobriyan wrote:
[]
> > The problem is that c-h.pl generates noise in the commit history and
> > makes git-blame less useful than it can be.
> 
> Could it be that this is a problem with git blame, rather than with
> checkpatch?  Last year there was a discussion on this list about how there
> is an option to git blame that will cause it to step through the history,
> and not show only the most recent patch that has modified a given line.

It is more or less an ease-of-use limitation of git blame.

There are some that want an ncurses only version of git blame
that could
use arrow-key style navigation for historical commit
line-ranges.

git gui blame kind of works, but it's not ncurses/text based.
git-cola kind of works too, but it's not text based either.

Are there other existing tools for blame history viewing?

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Jiri Kosina]

On Sun, 28 Aug 2016, Joe Perches wrote:

> Are there other existing tools for blame history viewing?

fugitive vim plugin has 'Gblame' command, which I personally find rather 
useful, and given the text-oriented nature could potentially be useful for 
your needs.

-- 
Jiri Kosina
SUSE Labs

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Dennis Kaarsemaker]

On zo, 2016-08-28 at 12:52 -0700, Joe Perches wrote:
> On Sun, 2016-08-28 at 11:59 +0200, Julia Lawall wrote:
> > 
> > On Sun, 28 Aug 2016, Alexey Dobriyan wrote:
> []
> > 
> > > 
> > > The problem is that c-h.pl generates noise in the commit history
> > > and
> > > makes git-blame less useful than it can be.
> > Could it be that this is a problem with git blame, rather than with
> > checkpatch?  Last year there was a discussion on this list about
> > how there
> > is an option to git blame that will cause it to step through the
> > history,
> > and not show only the most recent patch that has modified a given
> > line.
> It is more or less an ease-of-use limitation of git blame.
> 
> There are some that want an ncurses only version of git blame
> that could
> use arrow-key style navigation for historical commit
> line-ranges.
> 
> git gui blame kind of works, but it's not ncurses/text based.
> git-cola kind of works too, but it's not text based either.
> 
> Are there other existing tools for blame history viewing?

tig has a neat way of doing blame history digging: do a `tig blame
filename`, select a line and hit the comma key to re-blame from the
parent of the commit that was blamed for that line.

(hat tip to Jeff King who pointed out this trick recently)

D.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Sun, 2016-08-28 at 23:24 +0200, Dennis Kaarsemaker wrote:

> > There are some that want an ncurses only version of git blame
> > that could use arrow-key style navigation for historical commit
> > line-ranges.
> > 
> > git gui blame kind of works, but it's not ncurses/text based.
> > git-cola kind of works too, but it's not text based either.
> > 
> > Are there other existing tools for blame history viewing?
> tig has a neat way of doing blame history digging: do a `tig blame
> filename`, select a line and hit the comma key to re-blame from the
> parent of the commit that was blamed for that line.

Thanks, of course I neglected to mention tig.

What I think some here want is the ability to view
back and forth from old to new rather than just split
the window horizontally with the commit content.

Basically, apply the patch hunks for a specific range
to see color coded changes rather just show the entire
patch in a separate view.

Sure, seeing the commit log and patch is useful.

Seeing the block of code pre and post patch for the
specific section can be more useful.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Dan Carpenter]

I would like a couple changes which you know already:

1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
send checkpatch.pl fixes aren't qualified to say when it's legal or not
so they sometimes introduce bugs.

2) We could put some text in the output of --file output to say that if
it's not a staging patch, then we don't care about just making
checkpatch happy.  So consider if it is a waste of maintainer time
before sending.

regards,
dan carpenter

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Josh Triplett]

On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> I would like a couple changes which you know already:
> 
> 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> send checkpatch.pl fixes aren't qualified to say when it's legal or not
> so they sometimes introduce bugs.

I do think we should have *something* that catches such things.
Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
generates additional warnings, and can perhaps use more global
information to determine legality?

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Dan Carpenter]

On Mon, Aug 29, 2016 at 12:10:20PM -0700, Josh Triplett wrote:
> On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> > I would like a couple changes which you know already:
> > 
> > 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> > send checkpatch.pl fixes aren't qualified to say when it's legal or not
> > so they sometimes introduce bugs.
> 
> I do think we should have *something* that catches such things.
> Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
> generates additional warnings, and can perhaps use more global
> information to determine legality?

Perhaps.  But that shouldn't delay us from deleting this code which just
encourages newbies to introduce bugs.

regards,
dan carpenter

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 22:17 +0300, Dan Carpenter wrote:
> On Mon, Aug 29, 2016 at 12:10:20PM -0700, Josh Triplett wrote:
> > On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> > > I would like a couple changes which you know already:
> > > 
> > > 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> > > send checkpatch.pl fixes aren't qualified to say when it's legal or not
> > > so they sometimes introduce bugs.
> > I do think we should have *something* that catches such things.
> > Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
> > generates additional warnings, and can perhaps use more global
> > information to determine legality?
> Perhaps.  But that shouldn't delay us from deleting this code which just
> encourages newbies to introduce bugs.

You could send a patch.

I still kinda like the --force option

https://patchwork.kernel.org/patch/5814071/

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Joe Perches]

On Mon, 2016-08-29 at 12:10 -0700, Josh Triplett wrote:
> On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> > 
> > I would like a couple changes which you know already:
> > 
> > 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> > send checkpatch.pl fixes aren't qualified to say when it's legal or not
> > so they sometimes introduce bugs.
> I do think we should have *something* that catches such things.
> Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
> generates additional warnings, and can perhaps use more global
> information to determine legality?

nit: validity rather than legality.

There are still rather a lot of these.

$ git grep -E "\bmem.*,\s*(ETH_ALEN|6)\s*\);" | wc -l
1776

Dunno if any of them are in performance sensitive
areas where it actually matters.

Someone, I forget who, had a concern about the
object being set possibly being in a struct where
it's possible for the alignment of the set object
to be altered by another change like adding a new
member.