Messages in this thread | | | Date | Fri, 15 Aug 2008 13:15:18 +0100 (BST) | From | "Maciej W. Rozycki" <> | Subject | Re: [patch 8/8] x86: apic - unify init_bsp_APIC |
| |
On Fri, 15 Aug 2008, Ingo Molnar wrote:
> > >> @@ -962,7 +962,8 @@ void __init init_bsp_APIC(void) > > >> */ > > >> apic_write(APIC_LVT0, APIC_DM_EXTINT); > > >> value = APIC_DM_NMI; > > >> - if (!lapic_is_integrated()) /* 82489DX */ > > >> + /* discrete on 82489DX */ > > >> + if (!lapic_is_integrated()) > > >> value |= APIC_LVT_LEVEL_TRIGGER; > > >> apic_write(APIC_LVT1, value); > > >> } > > > > > > Please elaborate. > > > > > > Maciej > > > > > > > Hi Maciej, > > > > don't really understand what do you mean. [...] > > i suspect the question might have been: 'why this change'.
Indeed -- the change just moves a comment around from the obvious place (where it means: "the condition true applies to the 82489DX") to elsewhere while rephrasing it in a way that makes me wonder: "What the hell is that meant to mean?" Perhaps it is clearer to the others, but for me it is just obfuscation.
I think the comment as it is is clear enough and is also clearly visible when browsing through the source, when you want to spot all the odd bits related to the discrete APIC. This is no longer true after the change. And any more complete explanation belongs to the definition of lapic_is_integrated().
> If that was the question, the answer would be: to unify apic_32.c and > apic_64.c we first use tiny little changes to bring the two files in > sync. Presumably, each such change is a NOP or at least very safe - and > clearly bisectable in the worst-case.
If the obfuscation comes from apic_64.c, then the clean-up should be done in th other direction IMO. Not everything that comes from the 64-bit variation is better than its 32-bit counterpart.
Please note I do not question the semantic changes contained within this patch, so the issue of bisectability or code bloat (I am assuming lapic_is_integrated() expands to 0 on 64-bit; if not, this is clearly asking for improvement) is irrelevant.
Maciej
| |