* [git-pull -tip] x86: cleanup patches
@ 2009-03-14 16:13 Jaswinder Singh Rajput
2009-03-14 17:03 ` Ingo Molnar
0 siblings, 1 reply; 7+ messages in thread
From: Jaswinder Singh Rajput @ 2009-03-14 16:13 UTC (permalink / raw)
To: Ingo Molnar, x86 maintainers, LKML
The following changes since commit 4cca0345b9c1ee3573bcd0ea5feb3b44caa7930c:
Ingo Molnar (1):
Merge branch 'tracing/ftrace'
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git master
Jaswinder Singh Rajput (9):
x86: cpu/intel.c cleanup
x86: i8237.c cleanup
x86: topology.c cleanup
x86: kdebugfs.c cleanup
x86: i8253 cleanup
x86: pci-nommu.c cleanup
x86: io_delay.c cleanup
x86: rtc.c cleanup
x86: trampoline.c cleanup
arch/x86/kernel/cpu/intel.c | 186 +++++++++++++++++++++---------------------
arch/x86/kernel/i8237.c | 5 +-
arch/x86/kernel/i8253.c | 25 +++---
arch/x86/kernel/io_delay.c | 6 +-
arch/x86/kernel/kdebugfs.c | 35 +++++---
arch/x86/kernel/pci-nommu.c | 29 ++++--
arch/x86/kernel/rtc.c | 22 +++--
arch/x86/kernel/topology.c | 10 ++-
arch/x86/kernel/trampoline.c | 3 +-
9 files changed, 175 insertions(+), 146 deletions(-)
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [git-pull -tip] x86: cleanup patches 2009-03-14 16:13 [git-pull -tip] x86: cleanup patches Jaswinder Singh Rajput @ 2009-03-14 17:03 ` Ingo Molnar 2009-03-14 18:02 ` Jaswinder Singh Rajput 2009-03-16 12:18 ` Jaswinder Singh Rajput 0 siblings, 2 replies; 7+ messages in thread From: Ingo Molnar @ 2009-03-14 17:03 UTC (permalink / raw) To: Jaswinder Singh Rajput Cc: x86 maintainers, LKML, Thomas Gleixner, H. Peter Anvin * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote: > The following changes since commit 4cca0345b9c1ee3573bcd0ea5feb3b44caa7930c: > Ingo Molnar (1): > Merge branch 'tracing/ftrace' > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git master > > Jaswinder Singh Rajput (9): > x86: cpu/intel.c cleanup > x86: i8237.c cleanup > x86: topology.c cleanup > x86: kdebugfs.c cleanup > x86: i8253 cleanup > x86: pci-nommu.c cleanup > x86: io_delay.c cleanup > x86: rtc.c cleanup > x86: trampoline.c cleanup > > arch/x86/kernel/cpu/intel.c | 186 +++++++++++++++++++++--------------------- > arch/x86/kernel/i8237.c | 5 +- > arch/x86/kernel/i8253.c | 25 +++--- > arch/x86/kernel/io_delay.c | 6 +- > arch/x86/kernel/kdebugfs.c | 35 +++++--- > arch/x86/kernel/pci-nommu.c | 29 ++++-- > arch/x86/kernel/rtc.c | 22 +++-- > arch/x86/kernel/topology.c | 10 ++- > arch/x86/kernel/trampoline.c | 3 +- > 9 files changed, 175 insertions(+), 146 deletions(-) Hm, most of these files need deeper cleanups than just surface polishing. Are you willing to do those cleanups if someone goes through those files and comes up with a few suggestions? Which reminds me, i suggested a few structural and code flow cleanups wrt. smp_read_mpc() in the past and those didnt seem to have happened either. See the attached mail below - most of the code flow suggetions i made in it went unaddressed AFAICS. Ingo ----- Forwarded message from Ingo Molnar <mingo@elte.hu> ----- Date: Fri, 2 Jan 2009 18:21:50 +0100 From: Ingo Molnar <mingo@elte.hu> To: Jaswinder Singh Rajput <jaswinder@infradead.org> Subject: Re: [PATCH] x86: mpparse.c fix style problems Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>, x86 maintainers <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org> * Jaswinder Singh Rajput <jaswinder@infradead.org> wrote: > Impact: cleanup, fix style problems, more readable > @@ -314,7 +314,9 @@ static int __init smp_read_mpc(struct mp_config_table *mpc, unsigned early) > return 1; > > if (mpc->mpc_oemptr && x86_quirks->smp_read_mpc_oem) { > - struct mp_config_oemtable *oem_table = (struct mp_config_oemtable *)(unsigned long)mpc->mpc_oemptr; > + struct mp_config_oemtable *oem_table; > + oem_table = (struct mp_config_oemtable *) > + (unsigned long)mpc->mpc_oemptr; > x86_quirks->smp_read_mpc_oem(oem_table, mpc->mpc_oemsize); > } i think it would be cleaner to rename all the the mpc->mpc_X fields to mpc->X - that alone would give 4 characters per usage site. (we already know that it's an 'mpc' entity - no need to duplicate that in the field too) likewise, mp_config_oemtable should be renamed to mpc_oemtable to make it all more compact. also, instead of: > + struct mp_config_oemtable *oem_table; > + oem_table = (struct mp_config_oemtable *) > + (unsigned long)mpc->mpc_oemptr; we can do this oneliner: > + struct mpc_oemtable oem_table = (void *)(long)mpc->mpc_oemptr; these types of printk string tweaks: > - printk(KERN_INFO "No spare slots, try to append...take your risk, new mpc_length %x\n", count); > + printk(KERN_INFO "No spare slots, try to append" > + "...take your risk, new mpc_length %x\n", > + count); do not actually improve the result - as they break the string in about 40 columns - making grepping harder and making it less readable. So it's a step backwards. To solve the 80 columns wrap problem, the following could be and should be done instead. 1) get the type names right - they should be expressive but short - like a good Huffman encoding: See the mpc_ suggestion above, but there are more examples as well: struct mpc_config_processor *m = (struct mpc_config_processor *)mpt; mpc_config_processor should be renamed to mpc_cpu. The reason: the 'c' in MPC already means 'config' - no need to repeat that in the type name. Plus 'processor' is a lot longer than 'cpu' - so we try to use 'cpu' in all type names, as much as possible. 2) get the code flow granularity right. Use small but expressive functions, where each function does one well-defined thing that is easy to think about as one unit of activity. For example observe that replace_intsrc_all() is too big and not particularly well structured. furthermore, the whole function could be split up with a few helper functions. Most of the loops could be split up by doing something like: while (count < mpc->mpc_length) { switch (*mpt) { case MP_PROCESSOR: skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); continue; case MP_BUS: skip_entry(&mpt, &count, sizeof(struct mpc_bus)); continue; [...] case MP_INTSRC: parse_mpc_irq_entry(&mpt, &count); continue; default: ... goto out; } The whole thing is way more readable, and it's immediately obvious that the real work is done by MP_INTSRC - in a separate helper function. The skip_entry() helper function just skips over 3) Get the details right. Look at the source code - should that code be done like that and does it look as compact as it could be? for example these bits: while (count < mpc->mpc_length) { switch (*mpt) { case MP_PROCESSOR: { struct mpc_config_processor *m = (struct mpc_config_processor *)mpt; mpt += sizeof(*m); count += sizeof(*m); break; } case MP_BUS: are _way_ too wasteful with tabs - and that is causing the 80 cols problems. (we'd fix this if we hadnt fixed it at step 2 already ;-) Or these bits: ----------------> static int __init replace_intsrc_all(struct mp_config_table *mpc, unsigned long mpc_new_phys, unsigned long mpc_new_length) { #ifdef CONFIG_X86_IO_APIC int i; int nr_m_spare = 0; #endif int count = sizeof(*mpc); unsigned char *mpt = ((unsigned char *)mpc) + count; <---------------- are more readable as: ----------------> static int __init replace_intsrc_all(struct mpc_table *mpc, unsigned long mpc_new_phys, unsigned long mpc_new_length) { unsigned char *mpt = (void *)mpc; int count = 0; skip_entry(&mpt, &count, sizeof(struct mpc_table)); <---------------- we were able to do this because we introduced the skip_entry() helper that can be used for the initial mpc_table skip, and we were also able to remove the ugly #ifdef IO_APIC variable section because the totality of the MP_INTSRC parsing code moved into a helper function. The same principles can be applied to this loop: #ifdef CONFIG_X86_IO_APIC for (i = 0; i < mp_irq_entries; i++) { if (irq_used[i]) continue; all without changing any functionality of the code. The end result will be day and light to what we had before. Would you be interested in doing these cleanups? Ideally they should be done as a series of 5-10 patches - with a single conceptual cleanup per patch. Ingo ----- End forwarded message ----- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [git-pull -tip] x86: cleanup patches 2009-03-14 17:03 ` Ingo Molnar @ 2009-03-14 18:02 ` Jaswinder Singh Rajput 2009-03-16 12:18 ` Jaswinder Singh Rajput 1 sibling, 0 replies; 7+ messages in thread From: Jaswinder Singh Rajput @ 2009-03-14 18:02 UTC (permalink / raw) To: Ingo Molnar; +Cc: x86 maintainers, LKML, Thomas Gleixner, H. Peter Anvin On Sat, 2009-03-14 at 18:03 +0100, Ingo Molnar wrote: > * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote: > > > The following changes since commit 4cca0345b9c1ee3573bcd0ea5feb3b44caa7930c: > > Ingo Molnar (1): > > Merge branch 'tracing/ftrace' > > > > are available in the git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tiptop.git master > > > > Jaswinder Singh Rajput (9): > > x86: cpu/intel.c cleanup > > x86: i8237.c cleanup > > x86: topology.c cleanup > > x86: kdebugfs.c cleanup > > x86: i8253 cleanup > > x86: pci-nommu.c cleanup > > x86: io_delay.c cleanup > > x86: rtc.c cleanup > > x86: trampoline.c cleanup > > > > arch/x86/kernel/cpu/intel.c | 186 +++++++++++++++++++++--------------------- > > arch/x86/kernel/i8237.c | 5 +- > > arch/x86/kernel/i8253.c | 25 +++--- > > arch/x86/kernel/io_delay.c | 6 +- > > arch/x86/kernel/kdebugfs.c | 35 +++++--- > > arch/x86/kernel/pci-nommu.c | 29 ++++-- > > arch/x86/kernel/rtc.c | 22 +++-- > > arch/x86/kernel/topology.c | 10 ++- > > arch/x86/kernel/trampoline.c | 3 +- > > 9 files changed, 175 insertions(+), 146 deletions(-) > > Hm, most of these files need deeper cleanups than just surface > polishing. Are you willing to do those cleanups if someone goes > through those files and comes up with a few suggestions? > OK. > Which reminds me, i suggested a few structural and code flow > cleanups wrt. smp_read_mpc() in the past and those didnt seem to > have happened either. See the attached mail below - most of the > code flow suggetions i made in it went unaddressed AFAICS. > I have almost completed what you suggested me only few things was left and as I remembered I also prepared the final patches but I do not have any multiple processor machines to test it, and then I was busy in another threads like headers-check, perf-counters, cpu_debug. I will again search those patches and send to you for final testing. Thanks for reminding :-) -- JSR ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [git-pull -tip] x86: cleanup patches 2009-03-14 17:03 ` Ingo Molnar 2009-03-14 18:02 ` Jaswinder Singh Rajput @ 2009-03-16 12:18 ` Jaswinder Singh Rajput 2009-03-16 12:33 ` Jaswinder Singh Rajput ` (2 more replies) 1 sibling, 3 replies; 7+ messages in thread From: Jaswinder Singh Rajput @ 2009-03-16 12:18 UTC (permalink / raw) To: Ingo Molnar; +Cc: x86 maintainers, LKML, Thomas Gleixner, H. Peter Anvin On Sat, 2009-03-14 at 18:03 +0100, Ingo Molnar wrote: > ----- Forwarded message from Ingo Molnar <mingo@elte.hu> ----- > > Date: Fri, 2 Jan 2009 18:21:50 +0100 > From: Ingo Molnar <mingo@elte.hu> > To: Jaswinder Singh Rajput <jaswinder@infradead.org> > Subject: Re: [PATCH] x86: mpparse.c fix style problems > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>, x86 maintainers <x86@kernel.org>, > LKML <linux-kernel@vger.kernel.org> > > > * Jaswinder Singh Rajput <jaswinder@infradead.org> wrote: > > > Impact: cleanup, fix style problems, more readable > > > @@ -314,7 +314,9 @@ static int __init smp_read_mpc(struct mp_config_table *mpc, unsigned early) > > return 1; > > > > if (mpc->mpc_oemptr && x86_quirks->smp_read_mpc_oem) { > > - struct mp_config_oemtable *oem_table = (struct mp_config_oemtable *)(unsigned long)mpc->mpc_oemptr; > > + struct mp_config_oemtable *oem_table; > > + oem_table = (struct mp_config_oemtable *) > > + (unsigned long)mpc->mpc_oemptr; > > x86_quirks->smp_read_mpc_oem(oem_table, mpc->mpc_oemsize); > > } > > i think it would be cleaner to rename all the the mpc->mpc_X fields to > mpc->X - that alone would give 4 characters per usage site. (we already > know that it's an 'mpc' entity - no need to duplicate that in the field > too) > Already fixed earlier. > likewise, mp_config_oemtable should be renamed to mpc_oemtable to make it > all more compact. > > also, instead of: > > > + struct mp_config_oemtable *oem_table; > > + oem_table = (struct mp_config_oemtable *) > > + (unsigned long)mpc->mpc_oemptr; > > we can do this oneliner: > > > + struct mpc_oemtable oem_table = (void *)(long)mpc->mpc_oemptr; > Already fixed earlier. > these types of printk string tweaks: > > > - printk(KERN_INFO "No spare slots, try to append...take your risk, new mpc_length %x\n", count); > > + printk(KERN_INFO "No spare slots, try to append" > > + "...take your risk, new mpc_length %x\n", > > + count); > > do not actually improve the result - as they break the string in about 40 > columns - making grepping harder and making it less readable. So it's a > step backwards. > > To solve the 80 columns wrap problem, the following could be and should be > done instead. > > 1) get the type names right - they should be expressive but short - like a > good Huffman encoding: > > See the mpc_ suggestion above, but there are more examples as well: > > struct mpc_config_processor *m = > (struct mpc_config_processor *)mpt; > > mpc_config_processor should be renamed to mpc_cpu. The reason: the 'c' in > MPC already means 'config' - no need to repeat that in the type name. Plus > 'processor' is a lot longer than 'cpu' - so we try to use 'cpu' in all > type names, as much as possible. > Already fixed earlier. > 2) get the code flow granularity right. Use small but expressive > functions, where each function does one well-defined thing that is easy > to think about as one unit of activity. > > For example observe that replace_intsrc_all() is too big and not > particularly well structured. > > furthermore, the whole function could be split up with a few helper > functions. Most of the loops could be split up by doing something like: > > while (count < mpc->mpc_length) { > switch (*mpt) { > > case MP_PROCESSOR: > skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); > continue; > > case MP_BUS: > skip_entry(&mpt, &count, sizeof(struct mpc_bus)); > continue; > > [...] > case MP_INTSRC: > parse_mpc_irq_entry(&mpt, &count); > continue; > > default: > ... > goto out; > } > > The whole thing is way more readable, and it's immediately obvious that > the real work is done by MP_INTSRC - in a separate helper function. The > skip_entry() helper function just skips over > Here is the patch for skip_entry, I cannot check this patch on my side, so please check it and let me know the feedbacks: >From ff5a4d728b42af867dc8f6aef43107c7a015a5b4 Mon Sep 17 00:00:00 2001 From: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> Date: Mon, 16 Mar 2009 17:06:49 +0530 Subject: [PATCH] x86: mpparse cleanup Impact: cleanup Introduced helper function skip_entry and check_irq_src Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> --- arch/x86/kernel/mpparse.c | 179 ++++++++++++++++++++------------------------- 1 files changed, 78 insertions(+), 101 deletions(-) diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c index 47673e0..f1cf457 100644 --- a/arch/x86/kernel/mpparse.c +++ b/arch/x86/kernel/mpparse.c @@ -68,9 +68,9 @@ static void __init MP_processor_info(struct mpc_cpu *m) generic_processor_info(apicid, m->apicver); } -#ifdef CONFIG_X86_IO_APIC static void __init MP_bus_info(struct mpc_bus *m) { +#ifdef CONFIG_X86_IO_APIC char str[7]; memcpy(str, m->bustype, 6); str[6] = 0; @@ -108,11 +108,10 @@ static void __init MP_bus_info(struct mpc_bus *m) #endif } else printk(KERN_WARNING "Unknown bustype %s - ignoring\n", str); +#endif /* CONFIG_X86_IO_APIC */ } -#endif #ifdef CONFIG_X86_IO_APIC - static int bad_ioapic(unsigned long address) { if (nr_ioapics >= MAX_IO_APICS) { @@ -127,9 +126,11 @@ static int bad_ioapic(unsigned long address) } return 0; } +#endif /* CONFIG_X86_IO_APIC */ static void __init MP_ioapic_info(struct mpc_ioapic *m) { +#ifdef CONFIG_X86_IO_APIC if (!(m->flags & MPC_APIC_USABLE)) return; @@ -145,25 +146,32 @@ static void __init MP_ioapic_info(struct mpc_ioapic *m) mp_ioapics[nr_ioapics].apicver = m->apicver; mp_ioapics[nr_ioapics].flags = m->flags; nr_ioapics++; +#endif /* CONFIG_X86_IO_APIC */ } static void print_MP_intsrc_info(struct mpc_intsrc *m) { +#ifdef CONFIG_X86_IO_APIC apic_printk(APIC_VERBOSE, "Int: type %d, pol %d, trig %d, bus %02x," " IRQ %02x, APIC ID %x, APIC INT %02x\n", m->irqtype, m->irqflag & 3, (m->irqflag >> 2) & 3, m->srcbus, m->srcbusirq, m->dstapic, m->dstirq); +#endif /* CONFIG_X86_IO_APIC */ } static void __init print_mp_irq_info(struct mpc_intsrc *mp_irq) { +#ifdef CONFIG_X86_IO_APIC apic_printk(APIC_VERBOSE, "Int: type %d, pol %d, trig %d, bus %02x," " IRQ %02x, APIC ID %x, APIC INT %02x\n", mp_irq->irqtype, mp_irq->irqflag & 3, (mp_irq->irqflag >> 2) & 3, mp_irq->srcbus, mp_irq->srcbusirq, mp_irq->dstapic, mp_irq->dstirq); +#endif /* CONFIG_X86_IO_APIC */ } +#ifdef CONFIG_X86_IO_APIC + static void __init assign_to_mp_irq(struct mpc_intsrc *m, struct mpc_intsrc *mp_irq) { @@ -275,6 +283,12 @@ static int __init smp_check_mpc(struct mpc_table *mpc, char *oem, char *str) return 1; } +static void skip_entry(unsigned char **ptr, int *count, int size) +{ + *ptr += size; + *count += size; +} + static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) { char str[16]; @@ -310,55 +324,27 @@ static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - /* ACPI may have already provided this data */ - if (!acpi_lapic) - MP_processor_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + /* ACPI may have already provided this data */ + if (!acpi_lapic) + MP_processor_info((struct mpc_cpu *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; -#ifdef CONFIG_X86_IO_APIC - MP_bus_info(m); -#endif - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_bus_info((struct mpc_bus *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_ioapic *m = (struct mpc_ioapic *)mpt; - MP_ioapic_info(m); -#endif - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + MP_ioapic_info((struct mpc_ioapic *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - MP_intsrc_info(m); -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + MP_intsrc_info((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - MP_lintsrc_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_lintsrc_info((struct mpc_lintsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); @@ -850,15 +836,46 @@ static int __init get_MP_intsrc_index(struct mpc_intsrc *m) static struct mpc_intsrc __initdata *m_spare[SPARE_SLOT_NUM]; #endif +static int check_irq_src(struct mpc_intsrc *m) +{ +#ifdef CONFIG_X86_IO_APIC + int i, nr_m_spare = 0; + + apic_printk(APIC_VERBOSE, "OLD "); + print_MP_intsrc_info(m); + + i = get_MP_intsrc_index(m); + if (i > 0) { + assign_to_mpc_intsrc(&mp_irqs[i], m); + apic_printk(APIC_VERBOSE, "NEW "); + print_mp_irq_info(&mp_irqs[i]); + return 0; + } + if (!i) { + /* legacy, do nothing */ + return 0; + } + + /* + * not found (-1), or duplicated (-2) + * are invalid entries, + * we need to use the slot later + */ + m_spare[nr_m_spare] = m; + nr_m_spare++; + + return nr_m_spare; +#endif /* CONFIG_X86_IO_APIC */ +} + static int __init replace_intsrc_all(struct mpc_table *mpc, unsigned long mpc_new_phys, unsigned long mpc_new_length) { #ifdef CONFIG_X86_IO_APIC int i; - int nr_m_spare = 0; #endif - + int nr_m_spare = 0; int count = sizeof(*mpc); unsigned char *mpt = ((unsigned char *)mpc) + count; @@ -866,61 +883,21 @@ static int __init replace_intsrc_all(struct mpc_table *mpc, while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - apic_printk(APIC_VERBOSE, "OLD "); - print_MP_intsrc_info(m); - i = get_MP_intsrc_index(m); - if (i > 0) { - assign_to_mpc_intsrc(&mp_irqs[i], m); - apic_printk(APIC_VERBOSE, "NEW "); - print_mp_irq_info(&mp_irqs[i]); - } else if (!i) { - /* legacy, do nothing */ - } else if (nr_m_spare < SPARE_SLOT_NUM) { - /* - * not found (-1), or duplicated (-2) - * are invalid entries, - * we need to use the slot later - */ - m_spare[nr_m_spare] = m; - nr_m_spare++; - } -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + nr_m_spare = check_irq_src((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); -- 1.6.0.6 > 3) Get the details right. Look at the source code - should that code be > done like that and does it look as compact as it could be? > > for example these bits: > > while (count < mpc->mpc_length) { > switch (*mpt) { > case MP_PROCESSOR: > { > struct mpc_config_processor *m = > (struct mpc_config_processor *)mpt; > mpt += sizeof(*m); > count += sizeof(*m); > break; > } > case MP_BUS: > > are _way_ too wasteful with tabs - and that is causing the 80 cols > problems. (we'd fix this if we hadnt fixed it at step 2 already ;-) > > Or these bits: > > ----------------> > static int __init replace_intsrc_all(struct mp_config_table *mpc, > unsigned long mpc_new_phys, > unsigned long mpc_new_length) > { > #ifdef CONFIG_X86_IO_APIC > int i; > int nr_m_spare = 0; > #endif > > int count = sizeof(*mpc); > unsigned char *mpt = ((unsigned char *)mpc) + count; > <---------------- > > are more readable as: > > ----------------> > static int __init > replace_intsrc_all(struct mpc_table *mpc, > unsigned long mpc_new_phys, > unsigned long mpc_new_length) > { > unsigned char *mpt = (void *)mpc; > int count = 0; > > skip_entry(&mpt, &count, sizeof(struct mpc_table)); > <---------------- > Already fixed earlier. Thanks, -- JSR ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [git-pull -tip] x86: cleanup patches 2009-03-16 12:18 ` Jaswinder Singh Rajput @ 2009-03-16 12:33 ` Jaswinder Singh Rajput 2009-03-16 13:18 ` Jaswinder Singh Rajput 2009-03-16 14:18 ` Jaswinder Singh Rajput 2 siblings, 0 replies; 7+ messages in thread From: Jaswinder Singh Rajput @ 2009-03-16 12:33 UTC (permalink / raw) To: Ingo Molnar; +Cc: x86 maintainers, LKML, Thomas Gleixner, H. Peter Anvin On Mon, 2009-03-16 at 17:48 +0530, Jaswinder Singh Rajput wrote: > Here is the patch for skip_entry, I cannot check this patch on my side, > so please check it and let me know the feedbacks: > >From ff5a4d728b42af867dc8f6aef43107c7a015a5b4 Mon Sep 17 00:00:00 2001 > From: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> > Date: Mon, 16 Mar 2009 17:06:49 +0530 > Subject: [PATCH] x86: mpparse cleanup > > Impact: cleanup > > Introduced helper function skip_entry and check_irq_src With minor fix: >From 8bf5ec7d8cd427bfc7622f062d9b30277e2882f4 Mon Sep 17 00:00:00 2001 From: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> Date: Mon, 16 Mar 2009 17:06:49 +0530 Subject: [PATCH] x86: mpparse cleanup Impact: cleanup Introduced helper function skip_entry and check_irq_src Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> --- arch/x86/kernel/mpparse.c | 180 ++++++++++++++++++++------------------------- 1 files changed, 79 insertions(+), 101 deletions(-) diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c index 47673e0..f651af2 100644 --- a/arch/x86/kernel/mpparse.c +++ b/arch/x86/kernel/mpparse.c @@ -68,9 +68,9 @@ static void __init MP_processor_info(struct mpc_cpu *m) generic_processor_info(apicid, m->apicver); } -#ifdef CONFIG_X86_IO_APIC static void __init MP_bus_info(struct mpc_bus *m) { +#ifdef CONFIG_X86_IO_APIC char str[7]; memcpy(str, m->bustype, 6); str[6] = 0; @@ -108,11 +108,10 @@ static void __init MP_bus_info(struct mpc_bus *m) #endif } else printk(KERN_WARNING "Unknown bustype %s - ignoring\n", str); +#endif /* CONFIG_X86_IO_APIC */ } -#endif #ifdef CONFIG_X86_IO_APIC - static int bad_ioapic(unsigned long address) { if (nr_ioapics >= MAX_IO_APICS) { @@ -127,9 +126,11 @@ static int bad_ioapic(unsigned long address) } return 0; } +#endif /* CONFIG_X86_IO_APIC */ static void __init MP_ioapic_info(struct mpc_ioapic *m) { +#ifdef CONFIG_X86_IO_APIC if (!(m->flags & MPC_APIC_USABLE)) return; @@ -145,25 +146,32 @@ static void __init MP_ioapic_info(struct mpc_ioapic *m) mp_ioapics[nr_ioapics].apicver = m->apicver; mp_ioapics[nr_ioapics].flags = m->flags; nr_ioapics++; +#endif /* CONFIG_X86_IO_APIC */ } static void print_MP_intsrc_info(struct mpc_intsrc *m) { +#ifdef CONFIG_X86_IO_APIC apic_printk(APIC_VERBOSE, "Int: type %d, pol %d, trig %d, bus %02x," " IRQ %02x, APIC ID %x, APIC INT %02x\n", m->irqtype, m->irqflag & 3, (m->irqflag >> 2) & 3, m->srcbus, m->srcbusirq, m->dstapic, m->dstirq); +#endif /* CONFIG_X86_IO_APIC */ } static void __init print_mp_irq_info(struct mpc_intsrc *mp_irq) { +#ifdef CONFIG_X86_IO_APIC apic_printk(APIC_VERBOSE, "Int: type %d, pol %d, trig %d, bus %02x," " IRQ %02x, APIC ID %x, APIC INT %02x\n", mp_irq->irqtype, mp_irq->irqflag & 3, (mp_irq->irqflag >> 2) & 3, mp_irq->srcbus, mp_irq->srcbusirq, mp_irq->dstapic, mp_irq->dstirq); +#endif /* CONFIG_X86_IO_APIC */ } +#ifdef CONFIG_X86_IO_APIC + static void __init assign_to_mp_irq(struct mpc_intsrc *m, struct mpc_intsrc *mp_irq) { @@ -275,6 +283,12 @@ static int __init smp_check_mpc(struct mpc_table *mpc, char *oem, char *str) return 1; } +static void skip_entry(unsigned char **ptr, int *count, int size) +{ + *ptr += size; + *count += size; +} + static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) { char str[16]; @@ -310,55 +324,27 @@ static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - /* ACPI may have already provided this data */ - if (!acpi_lapic) - MP_processor_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + /* ACPI may have already provided this data */ + if (!acpi_lapic) + MP_processor_info((struct mpc_cpu *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; -#ifdef CONFIG_X86_IO_APIC - MP_bus_info(m); -#endif - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_bus_info((struct mpc_bus *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_ioapic *m = (struct mpc_ioapic *)mpt; - MP_ioapic_info(m); -#endif - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + MP_ioapic_info((struct mpc_ioapic *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - MP_intsrc_info(m); -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + MP_intsrc_info((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - MP_lintsrc_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_lintsrc_info((struct mpc_lintsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); @@ -850,15 +836,47 @@ static int __init get_MP_intsrc_index(struct mpc_intsrc *m) static struct mpc_intsrc __initdata *m_spare[SPARE_SLOT_NUM]; #endif +static int check_irq_src(struct mpc_intsrc *m) +{ + int nr_m_spare = 0; +#ifdef CONFIG_X86_IO_APIC + int i; + + apic_printk(APIC_VERBOSE, "OLD "); + print_MP_intsrc_info(m); + + i = get_MP_intsrc_index(m); + if (i > 0) { + assign_to_mpc_intsrc(&mp_irqs[i], m); + apic_printk(APIC_VERBOSE, "NEW "); + print_mp_irq_info(&mp_irqs[i]); + return 0; + } + if (!i) { + /* legacy, do nothing */ + return 0; + } + + /* + * not found (-1), or duplicated (-2) + * are invalid entries, + * we need to use the slot later + */ + m_spare[nr_m_spare] = m; + nr_m_spare++; +#endif /* CONFIG_X86_IO_APIC */ + + return nr_m_spare; +} + static int __init replace_intsrc_all(struct mpc_table *mpc, unsigned long mpc_new_phys, unsigned long mpc_new_length) { #ifdef CONFIG_X86_IO_APIC int i; - int nr_m_spare = 0; #endif - + int nr_m_spare = 0; int count = sizeof(*mpc); unsigned char *mpt = ((unsigned char *)mpc) + count; @@ -866,61 +884,21 @@ static int __init replace_intsrc_all(struct mpc_table *mpc, while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - apic_printk(APIC_VERBOSE, "OLD "); - print_MP_intsrc_info(m); - i = get_MP_intsrc_index(m); - if (i > 0) { - assign_to_mpc_intsrc(&mp_irqs[i], m); - apic_printk(APIC_VERBOSE, "NEW "); - print_mp_irq_info(&mp_irqs[i]); - } else if (!i) { - /* legacy, do nothing */ - } else if (nr_m_spare < SPARE_SLOT_NUM) { - /* - * not found (-1), or duplicated (-2) - * are invalid entries, - * we need to use the slot later - */ - m_spare[nr_m_spare] = m; - nr_m_spare++; - } -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + nr_m_spare = check_irq_src((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [git-pull -tip] x86: cleanup patches 2009-03-16 12:18 ` Jaswinder Singh Rajput 2009-03-16 12:33 ` Jaswinder Singh Rajput @ 2009-03-16 13:18 ` Jaswinder Singh Rajput 2009-03-16 14:18 ` Jaswinder Singh Rajput 2 siblings, 0 replies; 7+ messages in thread From: Jaswinder Singh Rajput @ 2009-03-16 13:18 UTC (permalink / raw) To: Ingo Molnar; +Cc: x86 maintainers, LKML, Thomas Gleixner, H. Peter Anvin On Mon, 2009-03-16 at 17:48 +0530, Jaswinder Singh Rajput wrote: > > > Here is the patch for skip_entry, I cannot check this patch on my side, > so please check it and let me know the feedbacks: > >From ff5a4d728b42af867dc8f6aef43107c7a015a5b4 Mon Sep 17 00:00:00 2001 > From: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> > Date: Mon, 16 Mar 2009 17:06:49 +0530 > Subject: [PATCH] x86: mpparse cleanup > > Impact: cleanup > > Introduced helper function skip_entry and check_irq_src hmm, this one seems much better: Subject: [PATCH] x86: mpparse cleanup Impact: cleanup Introduced helper function skip_entry and check_irq_src Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> --- arch/x86/kernel/mpparse.c | 202 ++++++++++++++++++++------------------------- 1 files changed, 88 insertions(+), 114 deletions(-) diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c index 47673e0..9347602 100644 --- a/arch/x86/kernel/mpparse.c +++ b/arch/x86/kernel/mpparse.c @@ -7,29 +7,29 @@ * (c) 2008 Alexey Starikovskiy <astarikovskiy@suse.de> */ -#include <linux/mm.h> -#include <linux/init.h> -#include <linux/delay.h> -#include <linux/bootmem.h> #include <linux/kernel_stat.h> #include <linux/mc146818rtc.h> +#include <linux/bootmem.h> #include <linux/bitops.h> -#include <linux/acpi.h> #include <linux/module.h> +#include <linux/delay.h> +#include <linux/acpi.h> +#include <linux/init.h> #include <linux/smp.h> +#include <linux/mm.h> -#include <asm/mtrr.h> -#include <asm/mpspec.h> +#include <asm/trampoline.h> +#include <asm/bios_ebda.h> #include <asm/pgalloc.h> #include <asm/io_apic.h> +#include <asm/mpspec.h> #include <asm/proto.h> -#include <asm/bios_ebda.h> -#include <asm/e820.h> -#include <asm/trampoline.h> #include <asm/setup.h> +#include <asm/apic.h> +#include <asm/e820.h> +#include <asm/mtrr.h> #include <asm/smp.h> -#include <asm/apic.h> /* * Checksum an MP configuration block. */ @@ -109,9 +109,6 @@ static void __init MP_bus_info(struct mpc_bus *m) } else printk(KERN_WARNING "Unknown bustype %s - ignoring\n", str); } -#endif - -#ifdef CONFIG_X86_IO_APIC static int bad_ioapic(unsigned long address) { @@ -224,8 +221,12 @@ static void __init MP_intsrc_info(struct mpc_intsrc *m) if (++mp_irq_entries == MAX_IRQ_SOURCES) panic("Max # of irq sources exceeded!!\n"); } +#else /* CONFIG_X86_IO_APIC */ +static inline void __init MP_bus_info(struct mpc_bus *m) {} +static inline void __init MP_ioapic_info(struct mpc_ioapic *m) {} +static inline void __init MP_intsrc_info(struct mpc_intsrc *m) {} +#endif /* CONFIG_X86_IO_APIC */ -#endif static void __init MP_lintsrc_info(struct mpc_lintsrc *m) { @@ -275,6 +276,12 @@ static int __init smp_check_mpc(struct mpc_table *mpc, char *oem, char *str) return 1; } +static void skip_entry(unsigned char **ptr, int *count, int size) +{ + *ptr += size; + *count += size; +} + static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) { char str[16]; @@ -310,55 +317,27 @@ static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - /* ACPI may have already provided this data */ - if (!acpi_lapic) - MP_processor_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + /* ACPI may have already provided this data */ + if (!acpi_lapic) + MP_processor_info((struct mpc_cpu *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; -#ifdef CONFIG_X86_IO_APIC - MP_bus_info(m); -#endif - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_bus_info((struct mpc_bus *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_ioapic *m = (struct mpc_ioapic *)mpt; - MP_ioapic_info(m); -#endif - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + MP_ioapic_info((struct mpc_ioapic *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - MP_intsrc_info(m); -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + MP_intsrc_info((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - MP_lintsrc_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_lintsrc_info((struct mpc_lintsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); @@ -848,7 +827,43 @@ static int __init get_MP_intsrc_index(struct mpc_intsrc *m) #define SPARE_SLOT_NUM 20 static struct mpc_intsrc __initdata *m_spare[SPARE_SLOT_NUM]; -#endif + +static int check_irq_src(struct mpc_intsrc *m) +{ + int nr_m_spare = 0; + int i; + + apic_printk(APIC_VERBOSE, "OLD "); + print_MP_intsrc_info(m); + + i = get_MP_intsrc_index(m); + if (i > 0) { + assign_to_mpc_intsrc(&mp_irqs[i], m); + apic_printk(APIC_VERBOSE, "NEW "); + print_mp_irq_info(&mp_irqs[i]); + return 0; + } + if (!i) { + /* legacy, do nothing */ + return 0; + } + + /* + * not found (-1), or duplicated (-2) + * are invalid entries, + * we need to use the slot later + */ + m_spare[nr_m_spare] = m; + nr_m_spare++; + + return nr_m_spare; +} +#else /* CONFIG_X86_IO_APIC */ +static inline int check_irq_src(struct mpc_intsrc *m) +{ + return 0; +} +#endif /* CONFIG_X86_IO_APIC */ static int __init replace_intsrc_all(struct mpc_table *mpc, unsigned long mpc_new_phys, @@ -856,9 +871,8 @@ static int __init replace_intsrc_all(struct mpc_table *mpc, { #ifdef CONFIG_X86_IO_APIC int i; - int nr_m_spare = 0; #endif - + int nr_m_spare = 0; int count = sizeof(*mpc); unsigned char *mpt = ((unsigned char *)mpc) + count; @@ -866,61 +880,21 @@ static int __init replace_intsrc_all(struct mpc_table *mpc, while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - apic_printk(APIC_VERBOSE, "OLD "); - print_MP_intsrc_info(m); - i = get_MP_intsrc_index(m); - if (i > 0) { - assign_to_mpc_intsrc(&mp_irqs[i], m); - apic_printk(APIC_VERBOSE, "NEW "); - print_mp_irq_info(&mp_irqs[i]); - } else if (!i) { - /* legacy, do nothing */ - } else if (nr_m_spare < SPARE_SLOT_NUM) { - /* - * not found (-1), or duplicated (-2) - * are invalid entries, - * we need to use the slot later - */ - m_spare[nr_m_spare] = m; - nr_m_spare++; - } -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + nr_m_spare = check_irq_src((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [git-pull -tip] x86: cleanup patches 2009-03-16 12:18 ` Jaswinder Singh Rajput 2009-03-16 12:33 ` Jaswinder Singh Rajput 2009-03-16 13:18 ` Jaswinder Singh Rajput @ 2009-03-16 14:18 ` Jaswinder Singh Rajput 2 siblings, 0 replies; 7+ messages in thread From: Jaswinder Singh Rajput @ 2009-03-16 14:18 UTC (permalink / raw) To: Ingo Molnar; +Cc: x86 maintainers, LKML, Thomas Gleixner, H. Peter Anvin On Mon, 2009-03-16 at 17:48 +0530, Jaswinder Singh Rajput wrote: > Here is the patch for skip_entry, I cannot check this patch on my side, > so please check it and let me know the feedbacks: > >From ff5a4d728b42af867dc8f6aef43107c7a015a5b4 Mon Sep 17 00:00:00 2001 > From: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> > Date: Mon, 16 Mar 2009 17:06:49 +0530 > Subject: [PATCH] x86: mpparse cleanup > > Impact: cleanup > > Introduced helper function skip_entry and check_irq_src There is a while loop so we cannot set nr_m_spare to zero, So here is new patch, hope this will work: Subject: [PATCH] x86: mpparse cleanup Impact: cleanup Introduced helper function skip_entry and check_irq_src Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com> --- arch/x86/kernel/mpparse.c | 197 +++++++++++++++++++-------------------------- 1 files changed, 83 insertions(+), 114 deletions(-) diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c index 47673e0..c86d33e 100644 --- a/arch/x86/kernel/mpparse.c +++ b/arch/x86/kernel/mpparse.c @@ -7,29 +7,29 @@ * (c) 2008 Alexey Starikovskiy <astarikovskiy@suse.de> */ -#include <linux/mm.h> -#include <linux/init.h> -#include <linux/delay.h> -#include <linux/bootmem.h> #include <linux/kernel_stat.h> #include <linux/mc146818rtc.h> +#include <linux/bootmem.h> #include <linux/bitops.h> -#include <linux/acpi.h> #include <linux/module.h> +#include <linux/delay.h> +#include <linux/acpi.h> +#include <linux/init.h> #include <linux/smp.h> +#include <linux/mm.h> -#include <asm/mtrr.h> -#include <asm/mpspec.h> +#include <asm/trampoline.h> +#include <asm/bios_ebda.h> #include <asm/pgalloc.h> #include <asm/io_apic.h> +#include <asm/mpspec.h> #include <asm/proto.h> -#include <asm/bios_ebda.h> -#include <asm/e820.h> -#include <asm/trampoline.h> #include <asm/setup.h> +#include <asm/apic.h> +#include <asm/e820.h> +#include <asm/mtrr.h> #include <asm/smp.h> -#include <asm/apic.h> /* * Checksum an MP configuration block. */ @@ -109,9 +109,6 @@ static void __init MP_bus_info(struct mpc_bus *m) } else printk(KERN_WARNING "Unknown bustype %s - ignoring\n", str); } -#endif - -#ifdef CONFIG_X86_IO_APIC static int bad_ioapic(unsigned long address) { @@ -224,8 +221,12 @@ static void __init MP_intsrc_info(struct mpc_intsrc *m) if (++mp_irq_entries == MAX_IRQ_SOURCES) panic("Max # of irq sources exceeded!!\n"); } +#else /* CONFIG_X86_IO_APIC */ +static inline void __init MP_bus_info(struct mpc_bus *m) {} +static inline void __init MP_ioapic_info(struct mpc_ioapic *m) {} +static inline void __init MP_intsrc_info(struct mpc_intsrc *m) {} +#endif /* CONFIG_X86_IO_APIC */ -#endif static void __init MP_lintsrc_info(struct mpc_lintsrc *m) { @@ -275,6 +276,12 @@ static int __init smp_check_mpc(struct mpc_table *mpc, char *oem, char *str) return 1; } +static void skip_entry(unsigned char **ptr, int *count, int size) +{ + *ptr += size; + *count += size; +} + static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) { char str[16]; @@ -310,55 +317,27 @@ static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early) while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - /* ACPI may have already provided this data */ - if (!acpi_lapic) - MP_processor_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + /* ACPI may have already provided this data */ + if (!acpi_lapic) + MP_processor_info((struct mpc_cpu *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; -#ifdef CONFIG_X86_IO_APIC - MP_bus_info(m); -#endif - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_bus_info((struct mpc_bus *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_ioapic *m = (struct mpc_ioapic *)mpt; - MP_ioapic_info(m); -#endif - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + MP_ioapic_info((struct mpc_ioapic *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - MP_intsrc_info(m); -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + MP_intsrc_info((struct mpc_intsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - MP_lintsrc_info(m); - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + MP_lintsrc_info((struct mpc_lintsrc *)&mpt); + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); @@ -848,7 +827,38 @@ static int __init get_MP_intsrc_index(struct mpc_intsrc *m) #define SPARE_SLOT_NUM 20 static struct mpc_intsrc __initdata *m_spare[SPARE_SLOT_NUM]; -#endif + +static void check_irq_src(struct mpc_intsrc *m, int *nr_m_spare) +{ + int i; + + apic_printk(APIC_VERBOSE, "OLD "); + print_MP_intsrc_info(m); + + i = get_MP_intsrc_index(m); + if (i > 0) { + assign_to_mpc_intsrc(&mp_irqs[i], m); + apic_printk(APIC_VERBOSE, "NEW "); + print_mp_irq_info(&mp_irqs[i]); + return; + } + if (!i) { + /* legacy, do nothing */ + return; + } + if (*nr_m_spare < SPARE_SLOT_NUM) { + /* + * not found (-1), or duplicated (-2) are invalid entries, + * we need to use the slot later + */ + m_spare[*nr_m_spare] = m; + *nr_m_spare += 1; + } +} +#else /* CONFIG_X86_IO_APIC */ +static inline void check_irq_src(struct mpc_intsrc *m, int *nr_m_spare) {} +#endif /* CONFIG_X86_IO_APIC */ + static int __init replace_intsrc_all(struct mpc_table *mpc, unsigned long mpc_new_phys, @@ -856,9 +866,8 @@ static int __init replace_intsrc_all(struct mpc_table *mpc, { #ifdef CONFIG_X86_IO_APIC int i; - int nr_m_spare = 0; #endif - + int nr_m_spare = 0; int count = sizeof(*mpc); unsigned char *mpt = ((unsigned char *)mpc) + count; @@ -866,61 +875,21 @@ static int __init replace_intsrc_all(struct mpc_table *mpc, while (count < mpc->length) { switch (*mpt) { case MP_PROCESSOR: - { - struct mpc_cpu *m = (struct mpc_cpu *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_cpu)); + break; case MP_BUS: - { - struct mpc_bus *m = (struct mpc_bus *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_bus)); + break; case MP_IOAPIC: - { - mpt += sizeof(struct mpc_ioapic); - count += sizeof(struct mpc_ioapic); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_ioapic)); + break; case MP_INTSRC: - { -#ifdef CONFIG_X86_IO_APIC - struct mpc_intsrc *m = (struct mpc_intsrc *)mpt; - - apic_printk(APIC_VERBOSE, "OLD "); - print_MP_intsrc_info(m); - i = get_MP_intsrc_index(m); - if (i > 0) { - assign_to_mpc_intsrc(&mp_irqs[i], m); - apic_printk(APIC_VERBOSE, "NEW "); - print_mp_irq_info(&mp_irqs[i]); - } else if (!i) { - /* legacy, do nothing */ - } else if (nr_m_spare < SPARE_SLOT_NUM) { - /* - * not found (-1), or duplicated (-2) - * are invalid entries, - * we need to use the slot later - */ - m_spare[nr_m_spare] = m; - nr_m_spare++; - } -#endif - mpt += sizeof(struct mpc_intsrc); - count += sizeof(struct mpc_intsrc); - break; - } + check_irq_src((struct mpc_intsrc *)&mpt, &nr_m_spare); + skip_entry(&mpt, &count, sizeof(struct mpc_intsrc)); + break; case MP_LINTSRC: - { - struct mpc_lintsrc *m = - (struct mpc_lintsrc *)mpt; - mpt += sizeof(*m); - count += sizeof(*m); - break; - } + skip_entry(&mpt, &count, sizeof(struct mpc_lintsrc)); + break; default: /* wrong mptable */ printk(KERN_ERR "Your mptable is wrong, contact your HW vendor!\n"); -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-03-16 14:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-14 16:13 [git-pull -tip] x86: cleanup patches Jaswinder Singh Rajput 2009-03-14 17:03 ` Ingo Molnar 2009-03-14 18:02 ` Jaswinder Singh Rajput 2009-03-16 12:18 ` Jaswinder Singh Rajput 2009-03-16 12:33 ` Jaswinder Singh Rajput 2009-03-16 13:18 ` Jaswinder Singh Rajput 2009-03-16 14:18 ` Jaswinder Singh Rajput
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox