On Thu, 15 Sep 2016 18:32:02 +0530 Madhavan Srinivasan <ma...@linux.vnet.ibm.com> wrote:
> New Kconfig is added "CONFIG_IRQ_DEBUG_SUPPORT" to add warn_on > to alert the invalid transitions. Have also moved the code under > the CONFIG_TRACE_IRQFLAGS in arch_local_irq_restore() to new Kconfig > as suggested. I can't tempt you to put the Kconfig changes into their own patch? :) > To support disabling and enabling of irq with PMI, set of > new powerpc_local_irq_pmu_save() and powerpc_local_irq_restore() > functions are added. And powerpc_local_irq_save() implemented, > by adding a new soft_enabled manipulation function soft_enabled_or_return(). > Local_irq_pmu_* macros are provided to access these powerpc_local_irq_pmu* > functions which includes trace_hardirqs_on|off() to match what we > have in include/linux/irqflags.h. > > Signed-off-by: Madhavan Srinivasan <ma...@linux.vnet.ibm.com> > @@ -81,6 +81,20 @@ static inline notrace unsigned long > soft_enabled_set_return(unsigned long enable > return flags; > } > > +static inline notrace unsigned long soft_enabled_or_return(unsigned long > enable) > +{ > + unsigned long flags, zero; > + > + asm volatile( > + "mr %1,%3; lbz %0,%2(13); or %1,%0,%1; stb %1,%2(13)" > + : "=r" (flags), "=&r"(zero) > + : "i" (offsetof(struct paca_struct, soft_enabled)),\ > + "r" (enable) > + : "memory"); > + > + return flags; > +} Another candidate for builtin_constification using immediates. And do you actually need the initial mr instruction there? > @@ -105,7 +119,7 @@ static inline unsigned long arch_local_irq_save(void) > > static inline bool arch_irqs_disabled_flags(unsigned long flags) > { > - return flags == IRQ_DISABLE_MASK_LINUX; > + return (flags); > } > > static inline bool arch_irqs_disabled(void) This part logically belongs in patch 11, but it also needs to be changed, I think? Keep in mind it's the generic kernel asking whether it has "Linux interrupts" disabled. return flags & IRQ_DISABLE_MASK_LINUX; > @@ -113,6 +127,59 @@ static inline bool arch_irqs_disabled(void) > return arch_irqs_disabled_flags(arch_local_save_flags()); > } > > +static inline void powerpc_local_irq_pmu_restore(unsigned long flags) > +{ > + arch_local_irq_restore(flags); > +} > + > +static inline unsigned long powerpc_local_irq_pmu_disable(void) > +{ > + return soft_enabled_or_return(IRQ_DISABLE_MASK_LINUX | > IRQ_DISABLE_MASK_PMU); > +} > + > +static inline unsigned long powerpc_local_irq_pmu_save(void) > +{ > + return powerpc_local_irq_pmu_disable(); > +} > + > +#define raw_local_irq_pmu_save(flags) > \ > + do { \ > + typecheck(unsigned long, flags); \ > + flags = powerpc_local_irq_pmu_save(); \ > + } while(0) > + > +#define raw_local_irq_pmu_restore(flags) \ > + do { \ > + typecheck(unsigned long, flags); \ > + powerpc_local_irq_pmu_restore(flags); \ > + } while(0) > + > +#ifdef CONFIG_TRACE_IRQFLAGS > +#define local_irq_pmu_save(flags) \ > + do { \ > + raw_local_irq_pmu_save(flags); \ > + trace_hardirqs_off(); \ > + } while(0) > +#define local_irq_pmu_restore(flags) \ > + do { \ > + if (raw_irqs_disabled_flags(flags)) { \ > + raw_local_irq_pmu_restore(flags);\ > + trace_hardirqs_off(); \ > + } else { \ > + trace_hardirqs_on(); \ > + raw_local_irq_pmu_restore(flags);\ > + } \ > + } while(0) > +#else > +#define local_irq_pmu_save(flags) \ > + do { \ > + raw_local_irq_pmu_save(flags); \ > + } while(0) > +#define local_irq_pmu_restore(flags) \ > + do { raw_local_irq_pmu_restore(flags); } while (0) > +#endif /* CONFIG_TRACE_IRQFLAGS */ > + > + This looks pretty good. When I suggested powerpc_ prefix, I intended in these functions here, so it wouldn't match with the local_irq_ namespace of generic kernel. But that was just an idea. If you prefer to do it this way, could you just drop the powerpc_ wrappers entirely? A comment above that says it comes from the generic Linux local_irq code might be an idea too. Provided at least the arch_irqs_disabled_flags comment gets addressed: Reviewed-by: Nicholas Piggin <npig...@gmail.com>