Re: [PATCH 12/13] powerpc: Add a Kconfig and a functions to set new soft_enabled mask

[Nicholas Piggin <npiggin@gmail.com>]

On Thu, 15 Sep 2016 18:32:02 +0530
Madhavan Srinivasan <maddy@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 <maddy@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 <npiggin@gmail.com>