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

From: Thomas Gleixner
Date: Mon Mar 16 2009 - 18:34:32 EST

On Tue, 17 Mar 2009, Jaswinder Singh Rajput wrote:

I think I made it entirely clear that blindly adding comments to
#endif / #else constructs is not what is considered a useful change.

> +static void early_init_intel_fam15(void)
> +{
> +#ifdef CONFIG_KMEMCHECK
> + u64 misc_enable;
> +
> + rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> +
> + if (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING) {
> + pr_info("kmemcheck: Disabling fast string operations\n");
> +
> + misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> + wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> + }
> +#endif /* CONFIG_KMEMCHECK */
> +}

Useless

> #ifdef CONFIG_X86_64
> set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> -#else
> +#else /* CONFIG_X86_64 */
> /* Netburst reports 64 bytes clflush size, but does IO in 128 bytes */
> if (c->x86 == 15 && c->x86_cache_alignment == 64)
> c->x86_cache_alignment = 128;
> -#endif
> +#endif /* CONFIG_X86_64 */

Annoying.

>
> #ifdef CONFIG_X86_32
> @@ -125,9 +131,11 @@ int __cpuinit ppro_with_ram_bug(void)
> boot_cpu_data.x86 == 6 &&
> boot_cpu_data.x86_model == 1 &&
> boot_cpu_data.x86_mask < 8) {
> - printk(KERN_INFO "Pentium Pro with Errata#50 detected. Taking evasive action.\n");
> + pr_info("Pentium Pro with Errata#50 detected. "
> + "Taking evasive action.\n");
> return 1;
> }
> +
> return 0;
> }
>
> @@ -143,7 +151,7 @@ static void __cpuinit trap_init_f00f_bug(void)
> idt_descr.address = fix_to_virt(FIX_F00F_IDT);
> load_idt(&idt_descr);
> }
> -#endif
> +#endif /* CONFIG_X86_F00F_BUG */

Another add a comment whether it makes sense or not

> static void __cpuinit intel_smp_check(struct cpuinfo_x86 *c)
> {
> @@ -164,7 +172,22 @@ static void __cpuinit intel_smp_check(struct cpuinfo_x86 *c)
> WARN_ONCE(1, "WARNING: SMP operation may be unreliable"
> "with B stepping processors.\n");
> }
> -#endif
> +#endif /* CONFIG_SMP */
> +}

Ditto.


> static void __cpuinit intel_workarounds(struct cpuinfo_x86 *c)
> @@ -173,8 +196,9 @@ static void __cpuinit intel_workarounds(struct cpuinfo_x86 *c)
>
> #ifdef CONFIG_X86_F00F_BUG
> /*
> - * All current models of Pentium and Pentium with MMX technology CPUs
> - * have the F0 0F bug, which lets nonprivileged users lock up the system.
> + * All current models of Pentium and Pentium with MMX technology
> + * CPUs have the F0 0F bug, which lets nonprivileged users lock
> + * up the system.
> * Note that the workaround only should be initialized once...
> */
> c->f00f_bug = 0;
> @@ -184,17 +208,18 @@ static void __cpuinit intel_workarounds(struct cpuinfo_x86 *c)
> c->f00f_bug = 1;
> if (!f00f_workaround_enabled) {
> trap_init_f00f_bug();
> - printk(KERN_NOTICE "Intel Pentium with F0 0F bug - workaround enabled.\n");
> + printk(KERN_NOTICE "Intel Pentium with F0 0F bug - "
> + "workaround enabled.\n");
> f00f_workaround_enabled = 1;
> }
> }
> -#endif
> +#endif /* CONFIG_X86_F00F_BUG */

Same here.

> @@ -238,36 +263,39 @@ static void __cpuinit intel_workarounds(struct cpuinfo_x86 *c)
> movsl_mask.mask = 7;
> break;
> }
> -#endif
> +#endif /* CONFIG_X86_INTEL_USERCOPY */

Ditto.

> #ifdef CONFIG_X86_NUMAQ
> numaq_tsc_disable();
> -#endif
> +#endif /* CONFIG_X86_NUMAQ */

Sigh.

> intel_smp_check(c);
> }
> -#else
> +#else /* CONFIG_X86_32 */
> +
> static void __cpuinit intel_workarounds(struct cpuinfo_x86 *c)
> {
> }
> -#endif
> +#endif /* CONFIG_X86_32 */

Nice. That one is really useful.

> static void __cpuinit srat_detect_node(void)
> {
> #if defined(CONFIG_NUMA) && defined(CONFIG_X86_64)
> - unsigned node;
> - int cpu = smp_processor_id();
> int apicid = hard_smp_processor_id();
> + int cpu = smp_processor_id();
> + unsigned node;
>
> - /* Don't do the funky fallback heuristics the AMD version employs
> - for now. */
> + /*
> + * Don't do the funky fallback heuristics the AMD version
> + * employs for now.
> + */
> node = apicid_to_node[apicid];
> if (node == NUMA_NO_NODE || !node_online(node))
> node = first_node(node_online_map);
> numa_set_node(cpu, node);
>
> - printk(KERN_INFO "CPU %d/0x%x -> Node %d\n", cpu, apicid, node);
> -#endif
> + pr_info("CPU %d/0x%x -> Node %d\n", cpu, apicid, node);
> +#endif /* CONFIG_NUMA && CONFIG_X86_64 */

Useless again.

>
> #ifdef CONFIG_X86_64
> - if (c->x86 == 15)
> - c->x86_cache_alignment = c->x86_clflush_size * 2;
> - if (c->x86 == 6)
> set_cpu_cap(c, X86_FEATURE_REP_GOOD);
> -#else
> +#else /* CONFIG_X86_64 */

Ditto
/*
> * Names for the Pentium II/Celeron processors
> * detectable only by also checking the cache size.
> * Dixon is NOT a Celeron.
> */
> - if (c->x86 == 6) {
> - char *p = NULL;
>
> switch (c->x86_model) {
> case 5:
> @@ -403,13 +421,19 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c)
>
> if (p)
> strcpy(c->x86_model_id, p);
> - }
>
> - if (c->x86 == 15)
> - set_cpu_cap(c, X86_FEATURE_P4);
> - if (c->x86 == 6)
> set_cpu_cap(c, X86_FEATURE_P3);
> -#endif
> +#endif /* CONFIG_X86_64 */

Total nonsense. This is the end of the #else path which should be
marked as /* !CONFIG_X86_64 */

> + break;
> +
> + case 15:
> +#ifdef CONFIG_X86_64
> + c->x86_cache_alignment = c->x86_clflush_size * 2;
> +#else /* CONFIG_X86_64 */
> + set_cpu_cap(c, X86_FEATURE_P4);
> +#endif /* CONFIG_X86_64 */

Groan.

> + break;
> + }
>
> if (!cpu_has(c, X86_FEATURE_XTOPOLOGY)) {
> /*
> @@ -419,7 +443,7 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c)
> c->x86_max_cores = intel_num_cpu_cores(c);
> #ifdef CONFIG_X86_32
> detect_ht(c);
> -#endif
> +#endif /* CONFIG_X86_32 */

crap++;

>
> static const struct cpu_dev __cpuinitconst intel_cpu_dev = {
> .c_vendor = "Intel",
> @@ -498,11 +508,10 @@ static const struct cpu_dev __cpuinitconst intel_cpu_dev = {
> },
> },
> .c_size_cache = intel_size_cache,
> -#endif
> +#endif /* CONFIG_X86_32 */

maybe_useful++;


> #ifdef CONFIG_X86_32
> static void pit_disable_clocksource(void);
> -#else
> +#else /* CONFIG_X86_32 */
> static inline void pit_disable_clocksource(void) { }
> -#endif
> +#endif /* CONFIG_X86_32 */

crap++;

> /*
> * HPET replaces the PIT, when enabled. So we need to know, which of
> @@ -40,7 +40,7 @@ static void init_pit_timer(enum clock_event_mode mode,
> {
> spin_lock(&i8253_lock);
>
> - switch(mode) {
> + switch (mode) {
> case CLOCK_EVT_MODE_PERIODIC:
> /* binary, mode 2, LSB/MSB, ch 0 */
> outb_pit(0x34, PIT_MODE);
> @@ -95,7 +95,7 @@ static int pit_next_event(unsigned long delta, struct clock_event_device *evt)
> * registered. This mechanism replaces the previous #ifdef LOCAL_APIC -
> * !using_apic_timer decisions in do_timer_interrupt_hook()
> */
> -static struct clock_event_device pit_clockevent = {
> +static struct clock_event_device pit_dev = {

Why do we need to change the name of the variable ? Just for fun or
is pit_dev more self explaining than pit_clockevent ?

> #ifndef CONFIG_X86_64
> @@ -133,11 +130,11 @@ void __init setup_pit_timer(void)
> */
> static cycle_t pit_read(void)
> {
> + static int old_count;
> unsigned long flags;
> + static u32 old_jifs;

Doh. Can we keep the statics together and not just at random places ?

> int count;
> u32 jifs;
> - static int old_count;
> - static u32 old_jifs;
>
> spin_lock_irqsave(&i8253_lock, flags);
> /*
> @@ -179,9 +176,9 @@ static cycle_t pit_read(void)
> * Previous attempts to handle these cases intelligently were
> * buggy, so we just do the simple thing now.
> */
> - if (count > old_count && jifs == old_jifs) {
> + if (count > old_count && jifs == old_jifs)
> count = old_count;
> - }
> +

We trade a brace against a newline. What's the win here ? Oh, I
forgot there is some magic rule which requires this.

> -static struct clocksource clocksource_pit = {
> +static struct clocksource pit_src = {

very intuitive naming and a bunch of changes which are just useless.

> + return clocksource_register(&pit_src);
> }
> arch_initcall(init_pit_clocksource);

Hint: You forgot to annotate the #endif here.

> @@ -22,10 +22,6 @@ static int __initdata io_delay_override;
> void native_io_delay(void)
> {
> switch (io_delay_type) {
> - default:
> - case CONFIG_IO_DELAY_TYPE_0X80:
> - asm volatile ("outb %al, $0x80");
> - break;
> case CONFIG_IO_DELAY_TYPE_0XED:
> asm volatile ("outb %al, $0xed");
> break;
> @@ -40,6 +36,10 @@ void native_io_delay(void)
> udelay(2);
> case CONFIG_IO_DELAY_TYPE_NONE:
> break;
> + case CONFIG_IO_DELAY_TYPE_0X80:
> + default:
> + asm volatile ("outb %al, $0x80");
> + break;
> }
> }

Did you bother to compare the assembly output ? Definitely not. The
ordering has a reason. Darn, this is not a playground for "oh this
looks nicer" changes.

> static int __init
> @@ -92,16 +93,19 @@ create_setup_data_node(struct dentry *parent, int no,
> error = -ENOMEM;
> goto err_return;
> }
> +
> type = debugfs_create_x32("type", S_IRUGO, d, &node->type);
> if (!type) {
> error = -ENOMEM;
> goto err_dir;
> }
> +
> data = debugfs_create_file("data", S_IRUGO, d, node, &fops_setup_data);
> if (!data) {
> error = -ENOMEM;
> goto err_type;
> }
> +

Yuck. Instead of adding blindly newlines you could have cleaned up
the "error = -ENOMEM" repetitive crap.

> if (PageHighMem(pg))
> iounmap(data);
> +
> if (error)
> goto err_dir;
> +
> no++;
> }
> +
> return 0;

Sigh. This way I see only half of the code on a screen.

> -#endif
> +#endif /* CONFIG_DEBUG_BOOT_PARAMS */

maybe_usefull++;

> -#endif
> -
> -#ifdef CONFIG_X86_IO_APIC

The first #ifdef change which makes really sense.

> static int bad_ioapic(unsigned long address)
> {
> @@ -125,6 +122,7 @@ static int bad_ioapic(unsigned long address)
> " found in table, skipping!\n");
> return 1;
> }
> +

Can we stop this please ? These changes are just annoying.

> - struct mpc_bus *m = (struct mpc_bus *)mpt;
> -#ifdef CONFIG_X86_IO_APIC
> - MP_bus_info(m);
> -#endif

Good catch.

Can you please remove all the useless changes and concentrate on the
useful ones ?

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/