Re: [PATCH 2/3] MIPS: HTW: Prevent accidental HTW start due to nested htw_{start,stop}

[Markos Chandras <Markos.Chandras@imgtec.com>]

On 01/26/2015 11:47 AM, Markos Chandras wrote:
> On 01/26/2015 11:36 AM, James Hogan wrote:
> 
>>
>>> +		raw_current_cpu_data.htw_seq++;				\
>>
>> not "if (!raw_current_cpu_data.htw_seq++)) {"?
> Why?
> 
> on _stop() calls you just increment it. The _start() will do the right
> thing then.
> 
> I think what you suggest it to move the if() condition from the _start()
> to _stop().
> 
>>
>>>  		write_c0_pwctl(read_c0_pwctl() &			\
>>>  			       ~(1 << MIPS_PWCTL_PWEN_SHIFT));		\
>>>  		back_to_back_c0_hazard();				\
>>> +		local_irq_restore(flags);				\
>>>  	}								\
>>>  } while(0)
>>>  
>>>  #define htw_start()							\
>>>  do {									\
>>> +	unsigned long flags;						\
>>> +									\
>>>  	if (cpu_has_htw) {						\
>>> -		write_c0_pwctl(read_c0_pwctl() |			\
>>> -			       (1 << MIPS_PWCTL_PWEN_SHIFT));		\
>>> -		back_to_back_c0_hazard();				\
>>> +		local_irq_save(flags);					\
>>> +		if (!--raw_current_cpu_data.htw_seq) {			\
>>> +			write_c0_pwctl(read_c0_pwctl() |		\
>>> +				       (1 << MIPS_PWCTL_PWEN_SHIFT));	\
>>> +			back_to_back_c0_hazard();			\
>>> +		}							\
>>> +		local_irq_restore(flags);				\
>>>  	}								\
>>>  } while(0)
>>>  
>>> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
>>> index dc49cf30c2db..5d6e59f20750 100644
>>> --- a/arch/mips/kernel/cpu-probe.c
>>> +++ b/arch/mips/kernel/cpu-probe.c
>>> @@ -367,8 +367,10 @@ static inline unsigned int decode_config3(struct cpuinfo_mips *c)
>>>  	if (config3 & MIPS_CONF3_MSA)
>>>  		c->ases |= MIPS_ASE_MSA;
>>>  	/* Only tested on 32-bit cores */
>>> -	if ((config3 & MIPS_CONF3_PW) && config_enabled(CONFIG_32BIT))
>>> +	if ((config3 & MIPS_CONF3_PW) && config_enabled(CONFIG_32BIT)) {
>>> +		c->htw_seq = 0;
>>
>> is that necessary, since cpu_data[] is global?
> 
> I checked and it is not placed in the .data instead of .bss section so i
> was not sure if it is initialized properly.
> 
sorry i meant it's placed in ".data" instead of ".bss" section.

-- 
markos