[PATCH] arch/mips/kernel/irq_cpu.c interrupt safety?

[PATCH] arch/mips/kernel/irq_cpu.c interrupt safety?

[William Jhun]

The mips_cpu_irq_*() routines in arch/mips/kernel/irq_cpu.c seem to not be
safe; {clear,set}_cp0_*() don't provide interrupt safety while changing the cp0
register. Is this not wrong? Is there a case where an interrupt handler may
change CP0 status? If so, the patch below (against linux_2_4) simply disables
interrupts during these operations.

Thanks,
Will

---
Index: arch/mips/kernel/irq_cpu.c
===================================================================
RCS file: /cvs/linux/arch/mips/kernel/irq_cpu.c,v
retrieving revision 1.2.2.2
diff -c -r1.2.2.2 irq_cpu.c
*** arch/mips/kernel/irq_cpu.c	2002/04/09 02:27:12	1.2.2.2
--- arch/mips/kernel/irq_cpu.c	2002/05/21 01:16:30
***************
*** 31,45 ****
  
  static void mips_cpu_irq_enable(unsigned int irq)
  {
  	clear_cp0_cause( 1 << (irq - mips_cpu_irq_base + 8));
  	set_cp0_status(1 << (irq - mips_cpu_irq_base + 8));
  }
  
! static void mips_cpu_irq_disable(unsigned int irq)
  {
  	clear_cp0_status(1 << (irq - mips_cpu_irq_base + 8));
  }
  
  static unsigned int mips_cpu_irq_startup(unsigned int irq)
  {
  	mips_cpu_irq_enable(irq);
--- 31,56 ----
  
  static void mips_cpu_irq_enable(unsigned int irq)
  {
+ 	unsigned long flags;
+ 	save_and_cli(flags);
  	clear_cp0_cause( 1 << (irq - mips_cpu_irq_base + 8));
  	set_cp0_status(1 << (irq - mips_cpu_irq_base + 8));
+ 	restore_flags(flags);
  }
  
! static void __mips_cpu_irq_disable(unsigned int irq)
  {
  	clear_cp0_status(1 << (irq - mips_cpu_irq_base + 8));
  }
  
+ static void mips_cpu_irq_disable(unsigned int irq)
+ {
+ 	unsigned long flags;
+ 	save_and_cli(flags);
+ 	__mips_cpu_irq_disable(irq);
+ 	restore_flags(flags);
+ }
+ 
  static unsigned int mips_cpu_irq_startup(unsigned int irq)
  {
  	mips_cpu_irq_enable(irq);
***************
*** 51,63 ****
  
  static void mips_cpu_irq_ack(unsigned int irq)
  {
! 	/* although we attemp to clear the IP bit in cause reigster, I think
  	 * usually it is cleared by device (irq source)
  	 */
  	clear_cp0_cause(1 << (irq - mips_cpu_irq_base + 8));
  
  	/* disable this interrupt - so that we safe proceed to the handler */
! 	mips_cpu_irq_disable(irq);
  }
  
  static void mips_cpu_irq_end(unsigned int irq)
--- 62,78 ----
  
  static void mips_cpu_irq_ack(unsigned int irq)
  {
! 	unsigned long flags;
! 	save_and_cli(flags);
! 
! 	/* although we attempt to clear the IP bit in cause register, I think
  	 * usually it is cleared by device (irq source)
  	 */
  	clear_cp0_cause(1 << (irq - mips_cpu_irq_base + 8));
  
  	/* disable this interrupt - so that we safe proceed to the handler */
! 	__mips_cpu_irq_disable(irq);
! 	restore_flags(flags);
  }
  
  static void mips_cpu_irq_end(unsigned int irq)

Re: Please disregard; was: [PATCH] arch/mips/kernel/irq_cpu.c interrupt safety?

[William Jhun]

Please disregard this, I jumped the gun on this one. These routines are
only ever called from arch/mips/kernel/irq.c, which always safely
calls these routines under the irq_desc's spinlock (or in the case of
ack(), do_IRQ() is already invoked with interrupts turned off).

I'll be sure to grep a little more carefully next time. ;o)

Will

On Mon, May 20, 2002 at 06:41:51PM -0700, William Jhun wrote:
> The mips_cpu_irq_*() routines in arch/mips/kernel/irq_cpu.c seem to not be
> safe; {clear,set}_cp0_*() don't provide interrupt safety while changing the cp0
> register. Is this not wrong? Is there a case where an interrupt handler may
> change CP0 status? If so, the patch below (against linux_2_4) simply disables
> interrupts during these operations.
> 
> Thanks,
> Will