[PATCH] x86: mpparse.c fix style problems

[PATCH] x86: mpparse.c fix style problems

[Jaswinder Singh Rajput]

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 <jaswinderrajput@gmail.com>
---
 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

Re: [PATCH] x86: mpparse.c fix style problems

[Ingo Molnar]

* 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

Re: [PATCH] x86: mpparse.c fix style problems

[Jaswinder Singh Rajput]

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

Re: [PATCH] x86: mpparse.c fix style problems

[Jaswinder Singh Rajput]

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

Re: [PATCH] x86: mpparse.c fix style problems

[Jaswinder Singh Rajput]

Hello Ingo,

On Fri, Jan 2, 2009 at 10:51 PM, Ingo Molnar <mingo@elte.hu> 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

Re: [PATCH] x86: mpparse.c fix style problems

[Ingo Molnar]

* Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:

> Hello Ingo,
> 
> On Fri, Jan 2, 2009 at 10:51 PM, Ingo Molnar <mingo@elte.hu> 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

Re: [PATCH] x86: mpparse.c fix style problems

[Jaswinder Singh Rajput]

On Sun, Jan 4, 2009 at 10:53 PM, Ingo Molnar <mingo@elte.hu> 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

Re: [PATCH] x86: mpparse.c fix style problems

[Jaswinder Singh Rajput]

Hello Ingo,

On Sun, Jan 4, 2009 at 10:53 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Jaswinder Singh Rajput <jaswinderlinux@gmail.com> 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