Re: [GIT PULL -tip][PATCH 0/9] MTRR fix trivial style patches

[Ingo Molnar]

* Jaswinder Singh Rajput <jaswinder@xxxxxxxxxx> wrote:

> > I think even the MTRR code (which is indeed one of the few x86 
> > places still not fully cleaned up) supports my arguments.
> > 
> > Look at the averages:
> > 
> >                                       errors   lines of code   errors/KLOC
> > arch/x86/kernel/cpu/mtrr/amd.c             2             120          16.6
> > arch/x86/kernel/cpu/mtrr/centaur.c         8             225          35.5
> > arch/x86/kernel/cpu/mtrr/cleanup.c         0            1102             0
> > arch/x86/kernel/cpu/mtrr/cyrix.c          10             275          36.3
> > arch/x86/kernel/cpu/mtrr/generic.c        16             730          21.9
> > arch/x86/kernel/cpu/mtrr/if.c             11             428          25.7
> > arch/x86/kernel/cpu/mtrr/main.c           50             751          66.5
> > arch/x86/kernel/cpu/mtrr/state.c           0              83             0
> > 
> 
> By these trivial mtrr cleanup patches:
>                                       errors
> arch/x86/kernel/cpu/mtrr/amd.c             0
> arch/x86/kernel/cpu/mtrr/centaur.c         0
> arch/x86/kernel/cpu/mtrr/cleanup.c         0
> arch/x86/kernel/cpu/mtrr/cyrix.c           0
> arch/x86/kernel/cpu/mtrr/generic.c         0
> arch/x86/kernel/cpu/mtrr/if.c              0
> arch/x86/kernel/cpu/mtrr/main.c            0
> arch/x86/kernel/cpu/mtrr/state.c           0
> 
> The following changes since commit 2137f10fd78ce8540ec4305f4dcd559039444544:
>   Ingo Molnar (1):
>         Merge branch 'perfcounters/urgent'
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-top.git master
> 
> Jaswinder Singh Rajput (9):
>       x86: mtrr/amd.c fix trivial style problems
>       x86: mtrr/centaur.c fix style problems
>       x86: mtrr/cleanup.c fix trivial style problems
>       x86: mtrr/cyrix.c fix trivial style problems
>       x86: mtrr/generic.c fix style problems
>       x86: mtrr/if.c fix trivial style problems
>       x86: mtrr/mtrr.h fix trivial style problems
>       x86: mtrr/state.c fix trivial style problems
>       x86: mtrr/main.c fix style problems
> 
>  arch/x86/kernel/cpu/mtrr/amd.c     |    7 +-
>  arch/x86/kernel/cpu/mtrr/centaur.c |  127 +++----------------------
>  arch/x86/kernel/cpu/mtrr/cleanup.c |   18 ++--
>  arch/x86/kernel/cpu/mtrr/cyrix.c   |   30 +++---
>  arch/x86/kernel/cpu/mtrr/generic.c |  147 +++++++++++++++-------------
>  arch/x86/kernel/cpu/mtrr/if.c      |   51 +++++++----
>  arch/x86/kernel/cpu/mtrr/main.c    |  185 +++++++++++++++++++-----------------
>  arch/x86/kernel/cpu/mtrr/mtrr.h    |    7 +-
>  arch/x86/kernel/cpu/mtrr/state.c   |   20 +++--
>  9 files changed, 263 insertions(+), 329 deletions(-)

Jaswinder, i'm going to ignore such pull requests from you in the 
future, because your changes are exceedingly painful and you just 
dont seem to learn.

This is the Nth incident, and there were a countless number of prior 
incidents where i warned you about various classes of avoidable 
problems, and you are not showing signs of significant improvements.

To get more specific: in this case too it's the same old problem of 
low quality of patches from you again:

 - You managed to put _3_ separate bugs into these 'trivial'
   cleanups. That is not acceptable. If you cannot make trivial
   patches be truly trivial, you should not be doing them really. 

 - i had to remove/undo/fix some bogus 'fixes' you did in those
   patches where you just blindly followed checkpatch instead of
   thinking about it for a minute. We dont want checkpatch warriors 
   - we want people with taste who use it as a tool to keep their
   new patches tidy, or who use it as a tool to aid cleanups.

 - i had to complete the patches and do all the cleanups you missed
   - sometimes on the next time to a change you did. You apparently 
   only checked checkpatch output - you didnt even look at all the 
   files for how it all looks like and whether there are other 
   things in those files that checkpatch missed. You made the files
   'checkpatch clean' - but you didnt actually look at whether the 
   files got cleaned up for good really. You left a half-done 
   jumbled mess behind you with files that still had inconsistent 
   looking style in them. Check the deltas of the patches i
   committed versus the ones you sent to see the countless cases you
   missed. And you cannot even claim to have done the 'trivial' 
   ones safely - because you did it in a buggy way and because the 
   final cleanups i did are all trivial too and can be validated.

 - i had to double check each commit on the assembly level to prove 
   that the patches are true cleanups. The new commit logs, size 
   checks, md5 sums are proof of that due diligence process. You 
   obviously didnt do any of that - you'd have noticed the bugs you 
   have put in had you done that.

 - i had to edit _all_ your changelogs. Again. You _still_ dont 
   think about your changelogs, they still suck 90% of the time.

All things considered it took me 6 hours to fix up and complete your 
'trivial' patches into which you have put only 3 hours of work:

 - your buggy and incomplete cleanups did:

      9 files changed, 263 insertions(+), 329 deletions(-)

 - the proper, fixed, rounded up, checked, validated and working 
   cleanups i committed with 6 extra hours of work:

      9 files changed, 868 insertions(+), 862 deletions(-)

The MTRR code is a highly critical piece of x86 code and i had to 
check assembly dumps manually and compared them to make sure the 
changes dont introduce subtle regressions.

The fact is, there is no other way to establish the trust level of 
trivial cleanups but to invest this depth of review and this level 
of testing and checking. I cannot just review until i find the first 
bug or problem and bounce it back to you as a review failure - i 
cannot know whether i can trust your next version of the patch and i 
cannot check the same trivial cleanups again and again as a machine, 
until you manage to submit the correct one.

So i've tested the trustworthyness of your changes for a final time 
and you failed this test.

I still committed it all under your name because i kept a fair 
amount of your changes so it's all v2 versions of your original 
patches - and per kernel convention i noted it in the changelog that 
i modified it further (and i didnt want to create 9 more commits, 
especially as some of your patches were broken so not bisection 
safe) - but this was the last time i went through such an effort 
with your patches.

As i warned you _repeatedly_ in the past that you should insert a 
lot more effort into patches before sending any patches and before 
sending any pull request. Other x86 maintainers warned you about 
that too. You seem to prefer five patches a day (a few of which are 
inevitably broken) instead of one good patch every five days.

This low level of patch quality just does not scale for maintainers 
of a busy tree which has more than a hundred contributors in every 
cycle. If i cannot trust a contributor i cannot apply such patches 
directly.

All in one, you were making the same kinds of mistakes again and 
again, over a many month time-span, and you are causing a 
considerable amount of maintainer overhead that is just not 
sustainable really.

So this simply does not work in this form. I can work with pretty 
much anyone, but there's a final limit to the energy i can invest 
into this. Please find some other Linux maintainer who can work with 
you and who is willing to apply your patches. If you want to work on 
x86 bits please find an x86 developer or maintainer who is willing 
to apply your patches or who is willing to give you Reviewed-by tags 
before sending them to me.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/