| Submitter | David Miller |
|---|---|
| Date | April 6, 2010, 11:39 p.m. |
| Message ID | <20100406.163955.28809295.davem@davemloft.net> |
| Download | mbox | patch |
| Permalink | /patch/49555/ |
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
Comments
On Tue, 2010-04-06 at 16:39 -0700, David Miller wrote: > It disables up to PIL_NMI instead of just PIL_NORMAL_MAX. > > Signed-off-by: David S. Miller <davem@davemloft.net> > > diff --git a/arch/sparc/include/asm/irqflags_64.h b/arch/sparc/include/asm/irqflags_64.h > index 8b49bf9..fa1e00e 100644 > --- a/arch/sparc/include/asm/irqflags_64.h > +++ b/arch/sparc/include/asm/irqflags_64.h > @@ -49,6 +49,16 @@ static inline void raw_local_irq_disable(void) > ); > } > > +static inline void raw_local_irq_disable_nmi(void) > +{ > + __asm__ __volatile__( > + "wrpr %0, %%pil" > + : /* no outputs */ > + : "i" (PIL_NMI) > + : "memory" > + ); > +} > + Isn't this wrong when used from !NMI context? Should this thing do something like: if (rdpr() < PIL_NORMAL_MAX) wrpr(PIL_NORMAL_MAX); so that it only disables IRQs, but doesn't enable NMIs. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Peter Zijlstra <peterz@infradead.org> Date: Wed, 07 Apr 2010 08:52:26 +0200 > On Tue, 2010-04-06 at 16:39 -0700, David Miller wrote: >> @@ -49,6 +49,16 @@ static inline void raw_local_irq_disable(void) >> ); >> } >> >> +static inline void raw_local_irq_disable_nmi(void) >> +{ >> + __asm__ __volatile__( >> + "wrpr %0, %%pil" >> + : /* no outputs */ >> + : "i" (PIL_NMI) >> + : "memory" >> + ); >> +} >> + > > Isn't this wrong when used from !NMI context? > > Should this thing do something like: > > if (rdpr() < PIL_NORMAL_MAX) > wrpr(PIL_NORMAL_MAX); > > so that it only disables IRQs, but doesn't enable NMIs. It's immaterial, local_irq_restore() will do the right thing, and it's ok to disable NMIs in these few cases I think. I desperately want to avoid that "test and maybe change the value %pil value we write" business, and honestly that's the whole point of this exercise. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-04-07 at 00:06 -0700, David Miller wrote: > From: Peter Zijlstra <peterz@infradead.org> > Date: Wed, 07 Apr 2010 08:52:26 +0200 > > > On Tue, 2010-04-06 at 16:39 -0700, David Miller wrote: > >> @@ -49,6 +49,16 @@ static inline void raw_local_irq_disable(void) > >> ); > >> } > >> > >> +static inline void raw_local_irq_disable_nmi(void) > >> +{ > >> + __asm__ __volatile__( > >> + "wrpr %0, %%pil" > >> + : /* no outputs */ > >> + : "i" (PIL_NMI) > >> + : "memory" > >> + ); > >> +} > >> + > > > > Isn't this wrong when used from !NMI context? > > > > Should this thing do something like: > > > > if (rdpr() < PIL_NORMAL_MAX) > > wrpr(PIL_NORMAL_MAX); > > > > so that it only disables IRQs, but doesn't enable NMIs. > > It's immaterial, local_irq_restore() will do the right thing, > and it's ok to disable NMIs in these few cases I think. > > I desperately want to avoid that "test and maybe change the > value %pil value we write" business, and honestly that's > the whole point of this exercise. Sure, its your architecture.. but could you explain why you're trying to avoid that compare so desperately, the local_irq_save_nmi() calls are few so surely they could carry that overhead. Also, doesn't __raw_local_irq_save_flags() already do the read? So its really just the compare that's gone missing. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patch
diff --git a/arch/sparc/include/asm/irqflags_64.h b/arch/sparc/include/asm/irqflags_64.h index 8b49bf9..fa1e00e 100644 --- a/arch/sparc/include/asm/irqflags_64.h +++ b/arch/sparc/include/asm/irqflags_64.h @@ -49,6 +49,16 @@ static inline void raw_local_irq_disable(void) ); } +static inline void raw_local_irq_disable_nmi(void) +{ + __asm__ __volatile__( + "wrpr %0, %%pil" + : /* no outputs */ + : "i" (PIL_NMI) + : "memory" + ); +} + static inline void raw_local_irq_enable(void) { __asm__ __volatile__( @@ -83,9 +93,28 @@ static inline unsigned long __raw_local_irq_save(void) return flags; } +static inline unsigned long __raw_local_irq_save_nmi(void) +{ + unsigned long flags = __raw_local_save_flags(); + + raw_local_irq_disable_nmi(); + + return flags; +} + #define raw_local_irq_save(flags) \ do { (flags) = __raw_local_irq_save(); } while (0) +#define raw_local_irq_save_nmi(flags) \ + do { (flags) = __raw_local_irq_save_nmi(); } while (0) + +#define local_irq_save_nmi(flags) \ + do { \ + typecheck(unsigned long, flags); \ + raw_local_irq_save_nmi(flags); \ + trace_hardirqs_off(); \ + } while (0) + #endif /* (__ASSEMBLY__) */ #endif /* !(_ASM_IRQFLAGS_H) */
It disables up to PIL_NMI instead of just PIL_NORMAL_MAX. Signed-off-by: David S. Miller <davem@davemloft.net> -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html