RE: [PATCH] tick: add check for the existence of broadcast clockevent device

[Thomas Gleixner]

Feng,

can you please fix your mail client to do proper line breaks around 78
characters ?

On Mon, 8 Jun 2009, Tang, Feng wrote:
> >From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
> >Why is that a problem ? You already have a special case for apbt0 in
> >the early setup code. So where is the problem when you have an apbt1
> >init call on CPU1 _before_ the local APIC is initialized on CPU1.
 
> Yes, your proposal makes good sense. But our apbt0 initialization is
> an x86 legacy one like HPET and is put into the same position where
> hpet_enable() exists, it keeps the impact to x86 arch dependent code
> be minimal, if we put the apbt1 init code into x86's cpu
> initialization code, I think it will cause much more complains,
> that's why we chose to follow hpet's method to use delayed init.

I understand that, but HPET does not rely on some magic events
happening. That's what I'm worried about. The boot code is fragile and
I prefer some explicit setup call over a fragile solution which
happens to work.

There is not much to complain about a platform specific function call
to set up special devices if there is a requirement for a call order.

In fact you can avoid setting up the local APIC timer at all. So what
you want is something like the patch below. You can set the
setup_secondary_clock pointer in the quirks structure when you detect
that you are running on such a system.

> The reason we proposed to add pointer check is there are already
> such checks in the broadcast code, adding these won't do harm to
> existing code.

I have no objections against the NULL pointer check in the broadcast
code, but I really wanted to understand why it's necessary. The patch
papers over the real problem and just works because CPU0 sends an
IPI. The correct solution is to use the real timer right away and not
rely on magic wakeups via IPIs. Ok ?

Thanks,

	tglx
---
Subject: x86: add quirk for secondary clock setup
From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Date: Mon, 08 Jun 2009 13:05:35 +0200

Some newer platforms require to use a separate per cpu clock event
device instead of the local APIC timer. Allow them to override the
setup for the secondary clock with a quirk.

Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
---
 arch/x86/include/asm/setup.h |    1 +
 arch/x86/kernel/smpboot.c    |   13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/include/asm/setup.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/setup.h
+++ linux-2.6/arch/x86/include/asm/setup.h
@@ -31,6 +31,7 @@ struct x86_quirks {
 	void (*smp_read_mpc_oem)(struct mpc_oemtable *oemtable,
 				unsigned short oemsize);
 	int (*setup_ioapic_ids)(void);
+	void (*setup_secondary_clock)(void);
 };
 
 extern void x86_quirk_pre_intr_init(void);
Index: linux-2.6/arch/x86/kernel/smpboot.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/smpboot.c
+++ linux-2.6/arch/x86/kernel/smpboot.c
@@ -263,6 +263,17 @@ static void __cpuinit smp_callin(void)
 }
 
 /*
+ * Setup secondary clock
+ */
+notrace static void __cpuinit __setup_secondary_clock(void)
+{
+	if (x86_quirks->setup_secondary_clock)
+		x86_quirks->setup_secondary_clock();
+	else
+		setup_secondary_clock();
+}
+
+/*
  * Activate a secondary processor.
  */
 notrace static void __cpuinit start_secondary(void *unused)
@@ -323,7 +334,7 @@ notrace static void __cpuinit start_seco
 	/* enable local interrupts */
 	local_irq_enable();
 
-	setup_secondary_clock();
+	__setup_secondary_clock();
 
 	wmb();
 	cpu_idle();
--
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/