Impact: cleanup, fix style problems, more readable
Fixes style problems:
WARNING: Use #include <linux/smp.h> instead of <asm/smp.h>
WARNING: Use #include <linux/acpi.h> instead of <asm/acpi.h>
WARNING: suspect code indent for conditional statements (8, 17)
WARNING: space prohibited between function name and open parenthesis '('
WARNING: line over 80 characters
total: 0 errors, 11 warnings
Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/kernel/mpparse.c | 32 ++++++++++++++++++++------------
1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index 45e3b69..5977fe8 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -16,14 +16,14 @@
#include <linux/bitops.h>
#include <linux/acpi.h>
#include <linux/module.h>
+#include <linux/smp.h>
+#include <linux/acpi.h>
-#include <asm/smp.h>
#include <asm/mtrr.h>
#include <asm/mpspec.h>
#include <asm/pgalloc.h>
#include <asm/io_apic.h>
#include <asm/proto.h>
-#include <asm/acpi.h>
#include <asm/bios_ebda.h>
#include <asm/e820.h>
#include <asm/trampoline.h>
@@ -95,8 +95,8 @@ static void __init MP_bus_info(struct mpc_config_bus *m)
#endif
if (strncmp(str, BUSTYPE_ISA, sizeof(BUSTYPE_ISA) - 1) == 0) {
- set_bit(m->mpc_busid, mp_bus_not_pci);
-#if defined(CONFIG_EISA) || defined (CONFIG_MCA)
+ set_bit(m->mpc_busid, mp_bus_not_pci);
+#if defined(CONFIG_EISA) || defined(CONFIG_MCA)
mp_bus_id_to_type[m->mpc_busid] = MP_BUS_ISA;
#endif
} else if (strncmp(str, BUSTYPE_PCI, sizeof(BUSTYPE_PCI) - 1) == 0) {
@@ -104,7 +104,7 @@ static void __init MP_bus_info(struct mpc_config_bus *m)
x86_quirks->mpc_oem_pci_bus(m);
clear_bit(m->mpc_busid, mp_bus_not_pci);
-#if defined(CONFIG_EISA) || defined (CONFIG_MCA)
+#if defined(CONFIG_EISA) || defined(CONFIG_MCA)
mp_bus_id_to_type[m->mpc_busid] = MP_BUS_PCI;
} else if (strncmp(str, BUSTYPE_EISA, sizeof(BUSTYPE_EISA) - 1) == 0) {
mp_bus_id_to_type[m->mpc_busid] = MP_BUS_EISA;
@@ -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);
}
@@ -464,9 +466,10 @@ static void __init construct_default_ioirq_mptable(int mpc_default_type)
if (ELCR_fallback) {
/*
- * If the ELCR indicates a level-sensitive interrupt, we
- * copy that information over to the MP table in the
- * irqflag field (level sensitive, active high polarity).
+ * If the ELCR indicates a level-sensitive interrupt,
+ * we copy that information over to the MP table in
+ * the irqflag field
+ * (level sensitive, active high polarity).
*/
if (ELCR_trigger(i))
intsrc.mpc_irqflag = 13;
@@ -948,12 +951,17 @@ static int __init replace_intsrc_all(struct mp_config_table *mpc,
(struct mpc_config_intsrc *)mpt;
count += sizeof(struct mpc_config_intsrc);
if (!mpc_new_phys) {
- 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);
} else {
if (count <= mpc_new_length)
- printk(KERN_INFO "No spare slots, try to append..., new mpc_length %x\n", count);
+ printk(KERN_INFO "No spare slots, "
+ "try to append..., "
+ "new mpc_length %x\n", count);
else {
- printk(KERN_ERR "mpc_new_length %lx is too small\n", mpc_new_length);
+ printk(KERN_ERR "mpc_new_length %lx is"
+ " too small\n", mpc_new_length);
goto out;
}
}
--
1.5.5.1
* Jaswinder Singh Rajput <[email protected]> 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
Hello Ingo,
On Fri, 2009-01-02 at 18:21 +0100, Ingo Molnar wrote:
> 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.
OK, today it will be late for me, I will send this tomorrow.
Thanks,
JSR
Hello Ingo,
On Fri, 2009-01-02 at 18:21 +0100, Ingo Molnar wrote:
> 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.
>
I send "[PATCH v2] x86: mpparse.c fix style problems" excluding :
WARNING: line over 80 characters
I will solve this in tomorrow patches. Once above patch will be applied
it will easier for me to focus on one thing.
Thanks,
JSR
Hello Ingo,
On Fri, Jan 2, 2009 at 10:51 PM, Ingo Molnar <[email protected]> wrote:
>
> 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)
>
I send [PATCH -tip 0/7] x86 rename all fields mpc_X to X based on this.
Currently I rename fields of few mpc_X structures from
arch/x86/include/asm/mpspec_def.h. In this file struct
intel_mp_floating is still remaining with mpf_X fields, Is this also
need to be fixed ? if yes then mpf_intel name is OK ?
Similarly in arch/x86/include/asm/io_apic.h :
1. struct mp_config_ioapic with mp_X fields (what name will be best ?)
2. struct mp_config_intsrc with mp_X fields
These are also used in arch/x86/kernel/mpparse.c
Please let me know should I fix mp_X things first or should I proceed
further in mpparse.c as per your suggestions.
Thank you,
JSR
* Jaswinder Singh Rajput <[email protected]> wrote:
> Hello Ingo,
>
> On Fri, Jan 2, 2009 at 10:51 PM, Ingo Molnar <[email protected]> wrote:
> >
> > 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)
> >
>
> I send [PATCH -tip 0/7] x86 rename all fields mpc_X to X based on this.
>
> Currently I rename fields of few mpc_X structures from
> arch/x86/include/asm/mpspec_def.h. In this file struct
> intel_mp_floating is still remaining with mpf_X fields, Is this also
> need to be fixed ? if yes then mpf_intel name is OK ?
>
> Similarly in arch/x86/include/asm/io_apic.h :
> 1. struct mp_config_ioapic with mp_X fields (what name will be best ?)
> 2. struct mp_config_intsrc with mp_X fields
>
> These are also used in arch/x86/kernel/mpparse.c
>
> Please let me know should I fix mp_X things first or should I proceed
> further in mpparse.c as per your suggestions.
i think those should be standardized to mpc_X too, and the fields should
lose their mp_ prefix.
Ingo
On Sun, Jan 4, 2009 at 10:53 PM, Ingo Molnar <[email protected]> wrote:
>>
>> Similarly in arch/x86/include/asm/io_apic.h :
>> 1. struct mp_config_ioapic with mp_X fields (what name will be best ?)
>> 2. struct mp_config_intsrc with mp_X fields
>>
>> These are also used in arch/x86/kernel/mpparse.c
>>
>> Please let me know should I fix mp_X things first or should I proceed
>> further in mpparse.c as per your suggestions.
>
> i think those should be standardized to mpc_X too, and the fields should
> lose their mp_ prefix.
>
Earlier we was having 2 sets :-
struct mpc_config_ioapic (mpspec) and mp_config_ioapic (ioapic)
struct mpc_config_intsrc (mpspec) and mp_config_intsrc (ioapic)
We moved struct mpc_config_X to struct mpc_X
I want to know what should be name for mp_config_ioapic. Is struct
mp_ioapic is OK ?
JSR
Hello Ingo,
On Sun, Jan 4, 2009 at 10:53 PM, Ingo Molnar <[email protected]> wrote:
>
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
>>
>> Currently I rename fields of few mpc_X structures from
>> arch/x86/include/asm/mpspec_def.h. In this file struct
>> intel_mp_floating is still remaining with mpf_X fields, Is this also
>> need to be fixed ? if yes then mpf_intel name is OK ?
>>
I send :
[PATCH -tip 1/2] x86: rename intel_mp_floating to mpf_intel
[PATCH -tip 2/2] x86: rename all fields of mpf_intel mpf_X to X
Is this OK ?
>> Similarly in arch/x86/include/asm/io_apic.h :
>> 1. struct mp_config_ioapic with mp_X fields (what name will be best ?)
>> 2. struct mp_config_intsrc with mp_X fields
>>
>> These are also used in arch/x86/kernel/mpparse.c
>>
>> Please let me know should I fix mp_X things first or should I proceed
>> further in mpparse.c as per your suggestions.
>
> i think those should be standardized to mpc_X too, and the fields should
> lose their mp_ prefix.
>
struct mpc_ioapic and struct mpc_intsrc is based on MultiProcessor
Specification Version 1.4
But I am not able to find the specification Manual of:
struct mp_config_ioapic and struct mp_config_intsrc.
(arch/x86/include/asm/io_apic.h)
Can you please point me out why the fields of struct mp_config_ioapic
and struct mp_config_intsrc are different from MultiProcessor
Specification Version 1.4
Thanks
--
JSR