remove legacy code

PATCH In review Jan Kiszka 2 participants 7 messages Dec 29, 2007 – Dec 31, 2007 x86

lore

Share & tools

lore

Jan Kiszka

author

2007-12-29 23:13 UTC #0 lore raw

PATCH 4/5 remove legacy code

This patch gets rid of code that is no longer needed or valid:

- x86_64 APIC frequency is now delivered via ipipe_request_tickdev
- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64
- global_clock_event is no longer needed by ipipe_32.c
- ipipe_tune_timer was dead code in x86_64 (there was no prototype in whatever header), and this arch does not really have a legacy code base to maintain, so kill this function

Jan

---
 arch/x86/kernel/apic_64.c       |    6 ------
 arch/x86/kernel/ipipe_32.c      |    2 --
 arch/x86/kernel/ipipe_64.c      |   10 +---------
 include/asm-x86/ipipe_64.h      |   21 +++++++++++----------
 include/asm-x86/ipipe_base_64.h |    4 ----
 5 files changed, 12 insertions(+), 31 deletions(-)

arch/x86/kernel/apic_64.c +0 -6

Index: linux-2.6.24-rc6-xeno/arch/x86/kernel/apic_64.c
===================================================================
@@ -939,9 +939,6 @@ static void __init calibrate_APIC_clock(
 
 	printk(KERN_INFO "Detected %d.%03d MHz APIC timer.\n",
 		result / 1000 / 1000, result / 1000 % 1000);
-#ifdef CONFIG_IPIPE
-	__ipipe_apic_timer_freq = result;
-#endif
 
 	/* Calculate the scaled math multiplication factor */
 	lapic_clockevent.mult = div_sc(result, NSEC_PER_SEC, 32);

@@ -970,9 +967,6 @@ void __init setup_boot_APIC_clock (void)
 	}
 
 	printk(KERN_INFO "Using local APIC timer interrupts.\n");
-#ifdef CONFIG_IPIPE
-	__ipipe_tick_irq = ipipe_apic_vector_irq(LOCAL_TIMER_VECTOR);
-#endif
 	calibrate_APIC_clock();
 
 	/*

arch/x86/kernel/ipipe_64.c +1 -9

Index: linux-2.6.24-rc6-xeno/arch/x86/kernel/ipipe_64.c
===================================================================
@@ -46,8 +46,7 @@
 
 asmlinkage void preempt_schedule_irq(void);
 
-int __ipipe_tick_irq;
-unsigned long __ipipe_apic_timer_freq;
+int __ipipe_tick_irq = TIMER_IRQ;
 
 DEFINE_PER_CPU(struct pt_regs, __ipipe_tick_regs);

@@ -102,12 +101,6 @@ int ipipe_get_sysinfo(struct ipipe_sysin
 	return 0;
 }
 
-int ipipe_tune_timer(unsigned long ns, int flags)
-{
-	printk(KERN_WARNING "I-pipe: %s is deprecated - no action taken\n", __FUNCTION__);
-	return -ENOSYS;
-}
-
 asmlinkage unsigned int do_IRQ(struct pt_regs *regs);
 asmlinkage void smp_apic_timer_interrupt(struct pt_regs *regs);
 asmlinkage void smp_spurious_interrupt(struct pt_regs *regs);

@@ -792,7 +785,6 @@ EXPORT_SYMBOL(ipipe_critical_enter);
 EXPORT_SYMBOL(ipipe_critical_exit);
 EXPORT_SYMBOL(ipipe_trigger_irq);
 EXPORT_SYMBOL(ipipe_get_sysinfo);
-EXPORT_SYMBOL(ipipe_tune_timer);
 
 EXPORT_SYMBOL_GPL(irq_desc);
 struct task_struct *__switch_to(struct task_struct *prev_p,

include/asm-x86/ipipe_64.h +11 ~~-10~~

Index: linux-2.6.24-rc6-xeno/include/asm-x86/ipipe_64.h
===================================================================
@@ -79,13 +79,6 @@ extern unsigned cpu_khz;
 #define ipipe_tsc2ns(t)	(((t) * 1000UL) / (ipipe_cpu_freq() / 1000000UL))
 #define ipipe_tsc2us(t)	((t) / (ipipe_cpu_freq() / 1000000UL))
 
-/*
- * The following interface will be deprecated once generic clockevents
- * are supported by this architecture, at which point
- * ipipe_request_tickdev() should be used instead.
- */
-extern unsigned long __ipipe_apic_timer_freq;
-
 /* Private interface -- Internal use only */
 
 #define __ipipe_check_platform()	do { } while(0)

@@ -125,6 +118,15 @@ static inline unsigned long __ipipe_ffnz
       return ul;
 }
 
+#define ipipe_update_tick_evtdev(evtdev)				\
+	do {								\
+		if (strcmp((evtdev)->name, "lapic") == 0)		\
+			__ipipe_tick_irq =				\
+				ipipe_apic_vector_irq(LOCAL_TIMER_VECTOR); \
+		else							\
+			__ipipe_tick_irq = TIMER_IRQ;			\
+	} while (0)
+
 struct irq_desc;
 
 void __ipipe_ack_edge_irq(unsigned irq, struct irq_desc *desc);

@@ -236,10 +238,9 @@ int __ipipe_check_tickdev(const char *de
 
 #else /* !CONFIG_IPIPE */
 
-#define task_hijacked(p)	0
+#define ipipe_update_tick_evtdev(evtdev)	do { } while (0)
+#define task_hijacked(p)			0
 
 #endif /* CONFIG_IPIPE */
 
-#define ipipe_update_tick_evtdev(evtdev)	do { } while (0)
-
 #endif	/* !__X86_IPIPE_64_H */

include/asm-x86/ipipe_base_64.h +0 -4

Index: linux-2.6.24-rc6-xeno/include/asm-x86/ipipe_base_64.h
===================================================================
@@ -182,8 +182,4 @@ static inline unsigned long __ipipe_test
 
 #endif /* !__ASSEMBLY__ */
 
-/* Arch-dependent features */
-
-#define __IPIPE_FEATURE_APIC_TIMER_FREQ  1
-
 #endif	/* !__X86_IPIPE_BASE_64_H */

arch/x86/kernel/ipipe_32.c +0 -2

Index: linux-2.6.24-rc6-xeno/arch/x86/kernel/ipipe_32.c
===================================================================
@@ -48,8 +48,6 @@
 #include <mach_ipi.h>
 #endif	/* CONFIG_X86_LOCAL_APIC */
 
-extern struct clock_event_device *global_clock_event;
-
 int __ipipe_tick_irq = TIMER_IRQ;
 
 DEFINE_PER_CPU(struct pt_regs, __ipipe_tick_regs);

Philippe Gerum

2007-12-30 10:52 UTC #1 lore raw

Quoted 3 lines · Jan Kiszka

This patch gets rid of code that is no longer needed or valid:
- x86_64 APIC frequency is now delivered via ipipe_request_tickdev

Ack.

Quoted 2 lines

- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64

Client RTOS may/do assume that timing is always delivered through the local APIC timer on x86_64. We could merge that and allow the 8253 to be used as the timer device, but this would not work in practice.

Quoted 1 line

- global_clock_event is no longer needed by ipipe_32.c

Ack.

Quoted 4 lines

- ipipe_tune_timer was dead code in x86_64 (there was no prototype in whatever header), and this arch does not really have a legacy code base to maintain, so kill this function

Ack.

Signature · 1 line

Philippe.

Jan Kiszka

author

2007-12-30 17:15 UTC #2 lore raw

Quoted 5 lines · Philippe Gerum

Quoted 3 lines · Jan Kiszka
This patch gets rid of code that is no longer needed or valid:
- x86_64 APIC frequency is now delivered via ipipe_request_tickdev

Quoted 8 lines

Ack.
Quoted 2 lines
- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64
Client RTOS may/do assume that timing is always delivered through the local APIC timer on x86_64. We could merge that and allow the 8253 to be used as the timer device, but this would not work in practice.

For forget HPET. There is no wiring of the timer IRQ to the APIC anymore. The result is that there is no difference between x86_32 and x86_64 anymore, and that's what the code reflects.

If the client RTOS is not yet fully prepared for this, this has to be fixed. And only if there is no valid Linux configuration (e.g. HPET off) that results in the same timer assignment as with 2.6.23, we need a workaround at I-pipe level IMHO.

Jan

Philippe Gerum

2007-12-30 19:55 UTC #3 lore raw

Quoted 7 lines · Jan Kiszka

Quoted 6 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
This patch gets rid of code that is no longer needed or valid:
- x86_64 APIC frequency is now delivered via ipipe_request_tickdev
Ack.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64
Client RTOS may/do assume that timing is always delivered through the local APIC timer on x86_64. We could merge that and allow the 8253 to be used as the timer device, but this would not work in practice.
For forget HPET. There is no wiring of the timer IRQ to the APIC anymore.

My point is that whatever timer IRQs the kernel routes to INT0, be them from the HPET in legacy route replacement mode or from the i8253 with some former routing, we currently rely on the local APIC timer vector. No matter what, HPET it's just unsupported by Xenomai right now, so we can't see any IRQ0 registered as our tick device interrupt.

If your point is about allowing HPET to be used, then your patch currently expects the HPET to be used in legacy replacement route mode, which is no more the default setup. In short, TIMER_IRQ may not be the right -constant- value in the common case.

The result is that there is no difference between x86_32 and

Quoted 7 lines

x86_64 anymore, and that's what the code reflects.
If the client RTOS is not yet fully prepared for this, this has to be fixed. And only if there is no valid Linux configuration (e.g. HPET off) that results in the same timer assignment as with 2.6.23, we need a workaround at I-pipe level IMHO.

Again, I see no basic issue in merging that patch, but the message is:
Xenomai still has to support more timer devices than the local APIC one on x86_64 (e.g. HPET), before this patch has any real upside for it. In short, it's ok with me, but I feel that we are still half-way from the complete support.

Signature · 1 line

Philippe.

Jan Kiszka

author

2007-12-30 23:31 UTC #4 lore raw

Quoted 13 lines · Philippe Gerum

Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
This patch gets rid of code that is no longer needed or valid:
- x86_64 APIC frequency is now delivered via ipipe_request_tickdev
Ack.
Quoted 2 lines
- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64
Client RTOS may/do assume that timing is always delivered through the local APIC timer on x86_64. We could merge that and allow the 8253 to be used as the timer device, but this would not work in practice.

Quoted 8 lines

Quoted 2 lines
For forget HPET. There is no wiring of the timer IRQ to the APIC anymore.
My point is that whatever timer IRQs the kernel routes to INT0, be them from the HPET in legacy route replacement mode or from the i8253 with some former routing, we currently rely on the local APIC timer vector. No matter what, HPET it's just unsupported by Xenomai right now, so we can't see any IRQ0 registered as our tick device interrupt.

__ipipe_tick_irq, as it is used now, is reflecting Linux' tick IRQ, not necessarily the one that Xenomai prefers.

Quoted 4 lines

If your point is about allowing HPET to be used, then your patch currently expects the HPET to be used in legacy replacement route mode, which is no more the default setup. In short, TIMER_IRQ may not be the

static struct clock_event_device hpet_clockevent = {
...
.irq = 0,

which is TIMER_IRQ.

Quoted 11 lines

right -constant- value in the common case.
The result is that there is no difference between x86_32 and
Quoted 7 lines
x86_64 anymore, and that's what the code reflects.
If the client RTOS is not yet fully prepared for this, this has to be fixed. And only if there is no valid Linux configuration (e.g. HPET off) that results in the same timer assignment as with 2.6.23, we need a workaround at I-pipe level IMHO.

Quoted 5 lines

Again, I see no basic issue in merging that patch, but the message is:
Xenomai still has to support more timer devices than the local APIC one on x86_64 (e.g. HPET), before this patch has any real upside for it. In short, it's ok with me, but I feel that we are still half-way from the complete support.

As long as the APIC is available to Xenomai, we should no longer worry about Linux using some other clockevent device. At least in theory. But I will carefully check again during this week if there are any regressions or .config limitation related to these hunks.

Jan

Philippe Gerum

2007-12-31 16:15 UTC #5 lore raw

Quoted 16 lines · Jan Kiszka

Quoted 15 lines · Philippe Gerum
Quoted 14 lines · Jan Kiszka
Quoted 11 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
This patch gets rid of code that is no longer needed or valid:
- x86_64 APIC frequency is now delivered via ipipe_request_tickdev
Ack.
Quoted 2 lines
- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64
Client RTOS may/do assume that timing is always delivered through the local APIC timer on x86_64. We could merge that and allow the 8253 to be used as the timer device, but this would not work in practice.
For forget HPET. There is no wiring of the timer IRQ to the APIC anymore.

Quoted 9 lines

Quoted 5 lines
My point is that whatever timer IRQs the kernel routes to INT0, be them from the HPET in legacy route replacement mode or from the i8253 with some former routing, we currently rely on the local APIC timer vector. No matter what, HPET it's just unsupported by Xenomai right now, so we can't see any IRQ0 registered as our tick device interrupt.
__ipipe_tick_irq, as it is used now, is reflecting Linux' tick IRQ, not necessarily the one that Xenomai prefers.

However, as a matter of fact, they are currently logically coupled. Let me detail my point, considering it from the Xenomai viewpoint, depending on the combinations of the two options we have been referring to:

** !CONFIG_X86_LOCAL_APIC && CONFIG_HPET_TIMER

=> Xenomai interposes on IRQ0 as normally generated by the HPET in LRR mode, but unfortunately makes the false assumption that the PIT is the active timer device and attempts to program it while managing timers in the real-time space, so no IRQ0 is ever generated. This leads to a complete lossage of clock event interrupts for both Linux and Xenomai. This is why we currently prevent such configuration from being set, at Kconfig-level.

** CONFIG_X86_LOCAL_APIC && CONFIG_HPET_TIMER

=> Xenomai always requests the LAPIC for timing because CONFIG_X86_LOCAL_APIC is defined, and always emulates host ticks by propagating forged interrupts to the LOCAL_TIMER_VECTOR (since v2.4.x, at least). Then, we have to consider how Linux's evtdev is set:
o if evtdev == HPET (and of course LAPIC is not dummy), then we would have two Linux timer ticks per period (one emulated by Xenomai to the local APIC timer vector, one real from the HPET to IRQ0) which would obviously be wrong,
o if evtdev == LAPIC, then things would work, Xenomai would both handle the host and the real-time ticking by managing the timer device for both domains.

** Therefore, a valid setup is either:

CONFIG_X86_LOCAL_APIC && CONFIG_HPET_TIMER, if evtdev == LAPIC
or
CONFIG_X86_LOCAL_APIC && !CONFIG_HPET_TIMER, implying evtdev == LAPIC

=> In which cases, Linux's clock evtdev must be LAPIC. If so, testing about the current evtdev name like your patch does, as if it could be different from "lapic", and in consequence as if we would allow a different interrupt source for the Linux tick than the LAPIC when CONFIG_X86_LOCAL_APIC is set, is _currently_ useless because not yet implemented at Xenomai level, even if the idea underlying the patch is generally right.

For this reason, I earlier merged your patch into the x86_32 section since we may have !CONFIG_X86_LOCAL_APIC, in which case the PIT may be used and we have to consider __ipipe_tick_irq as a true variable. However, I did not merge this change on purpose yet into the x86_64 section, mainly as a reminder that we currently assume that CONFIG_X86_LOCAL_APIC is always enabled on this arch, which raises the issues above.

Fortunately for us, the LAPIC has a better rating as a clock event device than the HPET, provided the Linux calibrator is happy with its stability. Therefore, we should have evtdev == LAPIC most of the time, at least with "decent x86 hardware" (which depending one one's mood, sometimes may look like a nice oxymoron).

This is why I basically agree with your patch because indeed, Xenomai will have to be made HPET-compliant at some point in time, but at the same time, I have to raise the concern that this patch won't be enough for proper timing in the _current_ Xenomai situation.

Quoted 10 lines

Quoted 3 lines
If your point is about allowing HPET to be used, then your patch currently expects the HPET to be used in legacy replacement route mode, which is no more the default setup. In short, TIMER_IRQ may not be the
static struct clock_event_device hpet_clockevent = {
...
.irq = 0,
which is TIMER_IRQ.

Yes, sorry for the noise, I was wrong. Using the HPET in LRR mode is still clearly wired into the 2.6.24 code.

Quoted 3 lines

As long as the APIC is available to Xenomai, we should no longer worry about Linux using some other clockevent device.

Provided we don't relay the host tick when Xenomai grabs the LAPIC whilst Linux is also notified from incoming timer ticks by interrupts from another device.

At least in theory. But

Quoted 3 lines

I will carefully check again during this week if there are any regressions or .config limitation related to these hunks.

Great, thanks.

Signature · 1 line

Philippe.

Philippe Gerum

2007-12-31 16:34 UTC #6 lore raw

Quoted 42 lines · Philippe Gerum

Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Philippe Gerum
Quoted 14 lines · Jan Kiszka
Quoted 11 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
This patch gets rid of code that is no longer needed or valid:
- x86_64 APIC frequency is now delivered via ipipe_request_tickdev
Ack.
Quoted 2 lines
- __ipipe_tick_irq is maintained via ipipe_update_tick_evtdev also on x86_64
Client RTOS may/do assume that timing is always delivered through the local APIC timer on x86_64. We could merge that and allow the 8253 to be used as the timer device, but this would not work in practice.
For forget HPET. There is no wiring of the timer IRQ to the APIC anymore.
Quoted 8 lines
Quoted 5 lines
My point is that whatever timer IRQs the kernel routes to INT0, be them from the HPET in legacy route replacement mode or from the i8253 with some former routing, we currently rely on the local APIC timer vector. No matter what, HPET it's just unsupported by Xenomai right now, so we can't see any IRQ0 registered as our tick device interrupt.
__ipipe_tick_irq, as it is used now, is reflecting Linux' tick IRQ, not necessarily the one that Xenomai prefers.
However, as a matter of fact, they are currently logically coupled. Let me detail my point, considering it from the Xenomai viewpoint, depending on the combinations of the two options we have been referring to:
** !CONFIG_X86_LOCAL_APIC && CONFIG_HPET_TIMER
=> Xenomai interposes on IRQ0 as normally generated by the HPET in LRR mode, but unfortunately makes the false assumption that the PIT is the active timer device and attempts to program it while managing timers in the real-time space, so no IRQ0 is ever generated. This leads to a complete lossage of clock event interrupts for both Linux and Xenomai. This is why we currently prevent such configuration from being set, at Kconfig-level.
** CONFIG_X86_LOCAL_APIC && CONFIG_HPET_TIMER

Quoted 7 lines

=> Xenomai always requests the LAPIC for timing because CONFIG_X86_LOCAL_APIC is defined, and always emulates host ticks by propagating forged interrupts to the LOCAL_TIMER_VECTOR (since v2.4.x, at least). Then, we have to consider how Linux's evtdev is set:
o if evtdev == HPET (and of course LAPIC is not dummy), then we would have two Linux timer ticks per period (one emulated by Xenomai to the local APIC timer vector, one real from the HPET to IRQ0)

Eek, ok, my brain is definitely in low power mode. if we don't use the same clock device than Linux, then we should neither emulate the periodic tick, nor receive any host tick programming requests anyway from Linux. Ok, forget about this. Let's be happy, and merge this.

We should only have to take care of the remaining issue #1.

Signature · 1 line

Philippe.