Messages in this thread | | | Date | Thu, 17 Apr 2008 13:55:41 -0700 | From | Andrew Morton <> | Subject | Re: [v2.6.26] what's brewing in x86.git for v2.6.26 |
| |
On Thu, 17 Apr 2008 22:39:55 +0200 "Vegard Nossum" <vegard.nossum@gmail.com> wrote:
> Hi Andrew, > > Thank you very much for the review of this patch. Those are hard to > come by, and I've posted kmemcheck to LKML already 3 or 4 times, with > relatively sparse response. I mean, the fact that they were ALL > whitespace damaged, but discovered by nobody, quite plainly tells me > that nobody actually tried to apply it (except perhaps Daniel Walker, > but we never realized it was whitespace damage causing the problems). > The patches that Ingo took into x86 were probably sent as an > attachment...
hm. Our review problem is well-understood.
> to Ingo before sending this to the list, so he should know. But it > should not have been part of this patch, I agree. > > > > early_param("kmemcheck", param_kmemcheck); > > > > kmemcheck= is documented in at least three places, which is nice, but it > > isn't mentioned in the place where we document kernel-parameters: > > Documentation/kernel-parameters.txt. A brief section there which directs > > the user to the extended docs would be fine. > > > > early_param() is unusual - we normally use __setup(). I assume there's a > > reason for using early_param(), but that reason cannot be discerned from > > reading the code. A /*comment*/ is the way to fix that. > > The reason is that we need to set this before kmalloc() is ever > called. A comment will come. > > But it seems that __setup() is what is really missing a comment. I > don't know what it is or how it works, and the comments around the > definition are not very helpful. Maybe somebody could enlighten me?
It makes my head spin too. Reading through the first bit of init/main.c:start_kernel() is your best hope, sorry.
> > > +static pte_t * > > > +address_get_pte(unsigned int address) > > > > This is not the preferred way of laying out function declarations but I've > > basically given up on this one. > > > > (void *)address > > > > is more common, but I'm close to giving up on that too. > > > > > static int > > > -show_addr(uint32_t addr) > > > +show_addr(uint32_t address) > > > > u32 is preferred, but ditto. > > This will be turned into unsigned long with 64-bit support. (Hopefully > we can get that working too.) > > Changing these to match the rest of the kernel is no problem for me. > It is not the way I would write it, but Pekka and Ingo has already > forced me to write if () instead of if(), so there should be no reason > to stop here! :-)
These things are OK as-is, I think. It'd be somewhat less nice in situations where newly-added code was inconsistent with surrounding existing code but it's still hardly a huge problem.
> > Perhaps we should get all this code onto the list(s) for re-review. It's > > been a while.. > > I'm not sure it would make much of a difference, except perhaps for > you, if you want to review it all. (My latest post to LKML had 0 > replies in total. Well, except private e-mail exchange with Ingo and > Pekka; they should know the code already. Once again, thanks to them > for helping me.) Do you still want me to post it again?
mm... I wouldn't mind taking a closer look at it all. That documentation file makes it _much_ easier to review the code, and the review becomes more effective than it would otherwise be.
> > PS: And it's not that I do that much testing/reviewing myself. But I > do think I have the excuse of being a newbie at this :-)
You're in good company ;)
| |