Messages in this thread |  | | | Date | Fri, 3 Jul 2009 19:24:29 +0100 | | From | Alan Cox <> | | Subject | Re: [PATCH] vt: add an event interface |
> The thing is, IMO if the author or the affected maintainer(s) are > not willing to do _basic_ cleanup of new drivers, they simply dont > deserve more substantial review.
That ignores what is actually going on:
Firstly what happens is someone posts a draft of their new driver. What they get back is - Mails from people who clearly can't work a compiler but can run checkpatch and have nothing useful to do, who complain about spaces and brackets - Little or no feedback on actual code
What they actually need is
- Feedback on the code - particularly algorithms, areas they could use kernel library functions better, obvious design flaws - Feedback on gross style issues ("Dude lose the Hungarian notation") NOT "Checkpatch says line 151 has a bogus space haha you suck" which is how the sort of replies they get today must feel. Even things like "here's a handy indent script". Stupid as it sounds I stopped one driver vendor throwing in the towel simply because they didn't know about indent, nobody told them and they thought they'd have to assign one of their developers to play space pusher.
There is a simple reason for focussing on big code issues (and major style) first:
If the code has minor style issues then fine for a new driver at the end of the day it is important to clean it up. That goes even for the stuff Checkpatch doesn't handle (like taste). There is however no point doing that for minor style until the code is algorithmically and generally stylistically in shape because much of what you clean up may not be there in the final version anyway.
So the world the checkpatch nutters create is one where a newcomer cannot get useful feedback on their code without fixing every single space, in order to be told after that actually you know 2/3rds of that code can be deleted and use the rbtree library instead. That's just wrong and a huge block to new users. What is worse is for a lot of submissions what you actually get is the can't work a C compiler checkpatch nutters whining about silly spaces issues and then when that is finished the actual submission gets ignored - totally - despite the efforts of a few people (official hero AKPM notably) to catch them all.
In the case where someone submits a new driver that is elegant code with minor style problems, then and only then is it appropriate to say "please tidy that comment to fit 80 columns". It would be nice if the people doing that could a) program and b) bothered to say things like "nice code, structure looks right, just needs a final polish" rather than parroting checkpatch pastes.
(Real world reality check for people here: Faced with a student who hands in a badly presented first draft of a school assignment with good basic facts does the teacher - read out loud each minor spelling mistake for the class to laugh at or - point out the strong areas, categorise the general weaknesses and suggest strategies for improvement, and while maybe suggesting using the spellchecker, believe the student can work that out if they are told it will be important next time
Its an obvious answer, yet the kernel list is increasingly operating in the wrong mode and we (all) need to fix that. )
> So such unclean drivers you mention above should probably go to > drivers/staging/ - that is an effort that tries to de-crappify > drivers before they get mixed into core drivers.
Staging is a big help as it provides an idiot evasion system, but most people submitting initial patches and bits of code don't know about it.
> All in one, i dont see at all the harm from people looking at > stylistic issues first
Imagine if Shakespeare had first been reviewed from the point of view of stylistic compliance with the recommended grammar, language and spelling of his day.
I have one big driver in mind btw which I suspect will hit staging at some point when released. It passes checkpatch perfectly. Its everything a checkpatch weenie could dribble over. Its also absolutely godawful. It's been recommended to go via staging for that reason.
We are going to get lots more wasted effort of that kind unless people both in the community and outside it stop treating checkpatch as a rule system, an arbiter of taste and the definition of what goes in tree. Right now there are people out there writing contracts to "provide a Linux driver that passes the checkpatch script", not that works or is good upstream code.
Incidentally if there are people reading this who really enjoy all the niggly little checkpatch hunting and twiddling its a great use of that talent to pick off older in tree code and fix it to pass checkpatch. That code is already in tree, and wants tidying.
Alan
|  |