Re: [PATCH 2/2 v3] SGI RTC: add generic timer system interrupt

[Dimitri Sivanich <sivanich@sgi.com>]

I will address your concerns with this in forthcoming patchsets.

On Thu, Nov 20, 2008 at 03:12:08PM -0800, Andrew Morton wrote:
> On Wed, 19 Nov 2008 15:26:31 -0600
> Dimitri Sivanich <sivanich@sgi.com> wrote:
> 
> > This patch allocates a system interrupt vector for platform specific use
> > in implementing timer interrupts.
> > 
> > Signed-off-by: Dimitri Sivanich <sivanich@sgi.com>
> > 
> > ---
> > 
> > Refreshing this to use 0xed as the vector.  Have repackaged it to be more
> > generic.
> > 
> >  arch/x86/include/asm/hw_irq.h      |    1 
> >  arch/x86/include/asm/irq.h         |    2 +
> >  arch/x86/include/asm/irq_vectors.h |    5 +++
> >  arch/x86/kernel/entry_64.S         |    4 ++
> >  arch/x86/kernel/irqinit_64.c       |   45 +++++++++++++++++++++++++++++++++
> >  5 files changed, 57 insertions(+)
> > 
> > Index: linux/arch/x86/kernel/entry_64.S
> > ===================================================================
> > --- linux.orig/arch/x86/kernel/entry_64.S	2008-11-19 14:59:15.000000000 -0600
> > +++ linux/arch/x86/kernel/entry_64.S	2008-11-19 15:02:16.000000000 -0600
> > @@ -865,6 +865,10 @@ ENTRY(apic_timer_interrupt)
> >  	apicinterrupt LOCAL_TIMER_VECTOR,smp_apic_timer_interrupt
> >  END(apic_timer_interrupt)
> >  
> > +ENTRY(generic_timer_interrupt)
> > +	apicinterrupt GENERIC_TIMER_VECTOR,smp_generic_timer_interrupt
> > +END(generic_timer_interrupt)
> > +
> >  ENTRY(uv_bau_message_intr1)
> >  	apicinterrupt 220,uv_bau_message_interrupt
> >  END(uv_bau_message_intr1)
> > Index: linux/arch/x86/kernel/irqinit_64.c
> > ===================================================================
> > --- linux.orig/arch/x86/kernel/irqinit_64.c	2008-11-19 14:59:15.000000000 -0600
> > +++ linux/arch/x86/kernel/irqinit_64.c	2008-11-19 15:09:36.000000000 -0600
> > @@ -23,6 +23,7 @@
> >  #include <asm/desc.h>
> >  #include <asm/apic.h>
> >  #include <asm/i8259.h>
> > +#include <asm/idle.h>
> >  
> >  /*
> >   * Common place to define all x86 IRQ vectors
> > @@ -161,6 +162,47 @@ void __init init_ISA_irqs(void)
> >  
> >  void init_IRQ(void) __attribute__((weak, alias("native_init_IRQ")));
> >  
> > +
> > +/* Function pointer for generic timer interrupt handling */
> > +static void (*generic_timer_interrupt_extension)(void);
> > +
> > +int
> > +generic_timer_reg_extension(void (*fn)(void))
> > +{
> > +	if (generic_timer_interrupt_extension)
> > +		return 1;
> > +
> > +	generic_timer_interrupt_extension = fn;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(generic_timer_reg_extension);
> 
> ah-hah, there it is!  These patches are in the wrong order, no?
> 
> 
> > +void
> > +generic_timer_unreg_extension(void)
> > +{
> > +	if (generic_timer_interrupt_extension)
> > +		generic_timer_interrupt_extension = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(generic_timer_unreg_extension);
> > +
> > +void smp_generic_timer_interrupt(struct pt_regs *regs)
> > +{
> > +	struct pt_regs *old_regs = set_irq_regs(regs);
> > +
> > +	ack_APIC_irq();
> > +
> > +	exit_idle();
> > +
> > +	irq_enter();
> > +
> > +	if (generic_timer_interrupt_extension)
> > +		generic_timer_interrupt_extension();
> > +
> > +	irq_exit();
> > +
> > +	set_irq_regs(old_regs);
> > +}
> 
> OK, I guess that answers my earlier question.
> 
> These function names are awful.  I thought that
> "generic_timer_reg_extension" had something to to with hardware
> registers in the timer.  I see now that you abbreviated "register" to
> "reg".
> 
> Please rename these to
> 
> register_generic_timer_extension()
> unregister_generic_timer_extension()
> 
> 
> 
> > >  static void __init smp_intr_init(void)
> >  {
> >  #ifdef CONFIG_SMP
> > @@ -202,6 +244,9 @@ static void __init apic_intr_init(void)
> >  	/* self generated IPI for local APIC timer */
> >  	alloc_intr_gate(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
> >  
> > +	/* IPI for platform specific timers */
> > +	alloc_intr_gate(GENERIC_TIMER_VECTOR, generic_timer_interrupt);
> > +
> >  	/* IPI vectors for APIC spurious and error interrupts */
> >  	alloc_intr_gate(SPURIOUS_APIC_VECTOR, spurious_interrupt);
> >  	alloc_intr_gate(ERROR_APIC_VECTOR, error_interrupt);
> > Index: linux/arch/x86/include/asm/irq_vectors.h
> > ===================================================================
> > --- linux.orig/arch/x86/include/asm/irq_vectors.h	2008-11-19 14:59:15.000000000 -0600
> > +++ linux/arch/x86/include/asm/irq_vectors.h	2008-11-19 15:02:16.000000000 -0600
> > @@ -92,6 +92,11 @@
> >  #define LOCAL_PERFMON_VECTOR	0xee
> >  
> >  /*
> > + * Generic timer vector for platform specific use
> > + */
> > +#define GENERIC_TIMER_VECTOR	0xed
> > +
> > +/*
> >   * First APIC vector available to drivers: (vectors 0x30-0xee) we
> >   * start at 0x31(0x41) to spread out vectors evenly between priority
> >   * levels. (0x80 is the syscall vector)
> > Index: linux/arch/x86/include/asm/hw_irq.h
> > ===================================================================
> > --- linux.orig/arch/x86/include/asm/hw_irq.h	2008-11-19 14:59:15.000000000 -0600
> > +++ linux/arch/x86/include/asm/hw_irq.h	2008-11-19 15:02:16.000000000 -0600
> > @@ -29,6 +29,7 @@
> >  
> >  /* Interrupt handlers registered during init_IRQ */
> >  extern void apic_timer_interrupt(void);
> > +extern void generic_timer_interrupt(void);
> >  extern void error_interrupt(void);
> >  extern void spurious_interrupt(void);
> >  extern void thermal_interrupt(void);
> > Index: linux/arch/x86/include/asm/irq.h
> > ===================================================================
> > --- linux.orig/arch/x86/include/asm/irq.h	2008-11-19 14:59:15.000000000 -0600
> > +++ linux/arch/x86/include/asm/irq.h	2008-11-19 15:02:16.000000000 -0600
> > @@ -38,6 +38,8 @@ extern void fixup_irqs(cpumask_t map);
> >  
> >  extern unsigned int do_IRQ(struct pt_regs *regs);
> >  extern void init_IRQ(void);
> > +extern int generic_timer_reg_extension(void (*fn)(void));
> > +extern void generic_timer_unreg_extension(void);
> >  extern void native_init_IRQ(void);
> >  
> >  /* Interrupt vector management */