Re: [git-pull -tip] x86: cleanup patches

[Jaswinder Singh Rajput <jaswinder@kernel.org>]

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