* James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > On Wed, 2009-06-10 at 03:00 +0200, Ingo Molnar wrote: > > * Ingo Molnar <mingo@xxxxxxx> wrote: > > > > > See for example what happened in linux-next today: Voyager broke > > > x86 and didnt even build, and Stephen had to keep it out of > > > today's linux-next build. > > > > I also took a look at that tree: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jejb/voyager-2.6.git master > > > > A couple of quick, mostly high-level observations: > > > > 1) > > > > The Voyager bits got rebased _yesterday_. _All_ the commits: > > > > commit 0ff51d1467af91bca4210b0d09372b6e7ded7524 > > Author: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > > AuthorDate: Mon Feb 23 15:02:04 2009 -0600 > > Commit: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > > CommitDate: Mon Jun 8 09:49:08 2009 -0500 > > > > I told James before that he should _not_ rebase: > > > > http://lkml.org/lkml/2009/2/1/76 > > > > In no uncertain terms. He completely ignored me and he messes up > > trees that way again. Now i should pull such a damaged tree while > > all the other x86 contributors we pull from do a thorough job? > > > > ( And note that the above link points to _another_ similar incident, > > where James rebased a tree and broke the build. It is a pattern of > > behavior. ) > > OK, so we disagree on this. But think what I'm having to do ... I > need a patch set to apply on top of your somewhat volatile changes > to x86 ... I'm using git to manage that patch set and I have no > downstream users of the voyager tree, so rebasing is a good > management tool. Rebasing is not a "management tool" - it's a destructive "past history is so messy or harmful that it's worthless so throw it away" tool. As explained be me in the link above. Also, care to clarify what you meant under "somewhat volatile changes to x86"? We dont rebase the x86 tree, so there's nothing 'volatile' about it. > If I go the merge point route, I get a tree with more non trivial > merge points than commits, so it becomes incredibly difficult for > anyone to follow what's going on. I also can no longer use > git-email to send my patch series anywhere. The proper work flow is to get the generic changes into the x86 tree and then to send the voyager bits without affecting anything else. There wont be any conflict in a normal workflow. > So I beg to differ and conclude the rebase maintenance of this > patch series is the logically correct thing for me to do at the > current time. > > > 2) > > > > The tree structure is unacceptable: > > > > a087b5f: [VOYAGER] x86: add prefill_possible_map to x86_quirks > > f5ef55a: [VOYAGER] x86/mca: make mca_nmi_hook external > > 55c8430: [VOYAGER] x86: add {safe,hard}_smp_processor_id to smp_ops > > fd1ab45: Revert "MAINTAINERS - Remove x86/Voyager no loner in tree" > > 0ff51d1: Revert "x86: remove the Voyager 32-bit subarch" > > > > That is crap. It starts with a huge 'revert' - which of course > > breaks the tree and needs 16 commits to bring back into action. > > The revert doesn't actually break anything ... you saw to that > yourself by marking CONFIG_VOYAGER BROKEN and by pulling all it's > files out of the central makefile about two weeks before you > actually removed the code. But that is not what matters: the revert is _ugly_ and unacceptable. > > It might be OK to _revive_ the source code that way privately - > > but the history completely incorrectly suggests a 'revert'. We > > revert buggy commits. > > > > What we want here is a clean submission, and a tidy, well > > constructed, non-rebased, well-tested Git tree. Or plain email > > submissions initially, because frankly i shouldnt Git-pull such > > a messy tree. > > > > 3) > > > > The comments. For example the revert: > > > > From 0ff51d1467af91bca4210b0d09372b6e7ded7524 Mon Sep 17 00:00:00 2001 > > From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > > Date: Mon, 23 Feb 2009 15:02:04 -0600 > > Subject: [PATCH] Revert "x86: remove the Voyager 32-bit subarch" > > > > This reverts commit 965c7ecaf2e2b083d711a01ab33735a4bdeee1a4. > > --- > > > > That is not how we do reverts! We always give a reason for a > > revert. > > Well "put it back again" is the reason ... however, it did seem to > be obvious in the Subject line ... but I can add an additional > reason if you insist ... what apart from "revive voyager" do you > want it to say? No, we dont want these reverts at all - see tglx's mails. We want a clean series adding hw support to the x86 architecture. > > 4) > > > > Small details like standard commit titles in the x86 tree. It > > shouldnt be: > > > > [VOYAGER] x86: eliminate subarchitecture file do_timer.h > > > > It should be: > > > > x86: Voyager: Eliminate subarchitecture file do_timer.h > > > > Note the different order and different capitalization. > > Well [TREE] is a standard. I've been using it for my trees for > ages. If you were to take voyager through your tree, then it would > follow your standards ... but you're forcing me to send it through > my own, so it follows mine. If as a sub-maintainer of a particular piece of code you are not willing to follow the commit standards of your upstream partner, then i simply will refuse to pull from you. > > 5) > > > > And as i requested it in an earier thread, quirky platforms should > > be supported via a _single_ quirks file. > > > > That makes it easy to handle and makes it easy to ignore as well. > > It's basically a "weird hardware driver". We have examples of that, > > for example arch/x86/kernel/visws_quirks.c. Voyager will be larger > > than that but it's OK - it's not like it will undergo massive > > development. > > > > Putting it back into arch/x86/mach-voyager/ again reintroduces the > > subarch directory structure we got rid of. > > It's just a convenient place to put the voyager stuff ... I can > move it somewhere, but code motion just makes the diffstats > explode for no good reason and makes it hard to judge the actual > changes. > > I'd argue having voyager in a separate directory which is never > built unless you enable the config option is further out of sight > and out of mind. > > But this is a tiny detail ... the three voyager files can go > anywhere, I'm not wedded to their actual location. Please put them into a single .c file so that we can demonstrate that even a non-trivial non-PC sub-architecture can be added via a single driver - like other, existing subarchitectures already do. We'll expect the same stringent quality requirements from any future embedded-space submissions as well. > > 6) > > > > This ordering of subsequent patches: > > > > 5c173bb: [VOYAGER] x86: eliminate subarchitecture file do_timer.h > > 18d3288: [VOYAGER] x86: eliminate subarchitecture file entry_arch.h > > 42c7289: [VOYAGER] x86: eliminate subarchitecture file setup_arch.h > > > > is totally wrong - it removes something that should not have > > been put there in the first place. > > I think you'll find do_timer.h, entry_arch.h and setup_arch.h > still exist in the current tree. > > What these patches are actually doing is completing your partial > subarchitecture elimination by folding the splits back into the > mainline code and removing the header files that enabled them. > > Out of all the patches, I would have thought you'd like these the > best, since they do finally eliminate the rest of the > subarchitecture stuff which still exists as trace elements in > arch/x86 today. Heh, you are right - the '[VOYAGER]' tag fooled me there. Mea culpa. > > If then Voyager should be added as a clean series of patches: > > first the generic patches that introduce infrastructure changes > > (to x86_quirks and smp_ops, etc.), then a single final patch > > that adds in the Voyager quirks platform driver. > > > > And we've been through similar excercises multiple times before. > > > > All in one, this tree is quite poor and needs a lot of work. > > So basically, if the only problems you have are commit log naming > and patch sequencing, you find the actual code technically fine? I havent gone to that detail yet - i'd like to see the generic changes first, and i'd like to see the general structure and principle of the patches be correct. But hey ... here it goes: I just did a quick 3-places sample into the voyager code and there's 3 easy-to-solve visual things i noticed: one thing you should take care of is to please use the customary comment style: /* * Comment ..... * ...... goes here: */ specified in Documentation/CodingStyle. Also, please get rid of VDEBUG() and VOYAGER_DEBUG or convert it to pr_debug(). Plus there's a number of places where you use __u8 types needlessly, such as in before_handle_vic_irq(): __u8 cpu = smp_processor_id(); We only use __u8 where it truly matters when interacting with hardware. Here (and in other places) it should be 'int'. Please re-read the whole code with such general cleanliness principles in mind, i'm sure there are other places to fix as well. I can re-review it if you think it's all 100% squeaky clean and in accordancy with the typical Linux kernel and arch/x86 coding style and principles. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-next" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
