Re: [PATCH RFC v2 24/70] MIPS: asm: spinlock: Replace sub instruction with addiu

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

On 01/20/2015 01:04 AM, Maciej W. Rozycki wrote:
> On Fri, 16 Jan 2015, Markos Chandras wrote:
> 
>> sub $reg, imm is not a real MIPS instruction. The assembler replaces
>> that with 'addi $reg, -imm'. However, addi has been removed from R6,
>> so we replace the 'sub' instruction with 'addiu' instead.
>>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> ---
>>  arch/mips/include/asm/spinlock.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
>> index c6d06d383ef9..500050d3bda6 100644
>> --- a/arch/mips/include/asm/spinlock.h
>> +++ b/arch/mips/include/asm/spinlock.h
>> @@ -276,7 +276,7 @@ static inline void arch_read_unlock(arch_rwlock_t *rw)
>>  		do {
>>  			__asm__ __volatile__(
>>  			"1:	ll	%1, %2	# arch_read_unlock	\n"
>> -			"	sub	%1, 1				\n"
>> +			"	addiu	%1, -1				\n"
>>  			"	sc	%1, %0				\n"
>>  			: "=" GCC_OFF12_ASM() (rw->lock), "=&r" (tmp)
>>  			: GCC_OFF12_ASM() (rw->lock)
> 
>  This integer overflow trap is deliberate here -- have you seen the note 
> just above:
> 
> /* Note the use of sub, not subu which will make the kernel die with an
>    overflow exception if we ever try to unlock an rwlock that is already
>    unlocked or is being held by a writer.  */
> 
> ?
> 
>  What this shows really is a GAS bug fix for the SUB macro is needed 
> similar to what I suggested in 12/70 for ADDI (from the situation I infer 
> there is some real work to do in GAS in this area; adding Matthew as a 
> recipient to raise his awareness) so that it does not expand to ADDI where 
> the architecture or processor selected do not support it.  Instead a 
> longer sequence involving SUB has to be produced.
> 
>  However, regardless, I suggest code like:
> 
> /* There's no R6 ADDI instruction so use the ADD register version instead. */
> #ifdef CONFIG_CPU_MIPSR6
> #define GCC_ADDI_ASM() "r"
> #else
> #define GCC_ADDI_ASM() "I"
> #endif
> 
> 			__asm__ __volatile__(
> 			"1:	ll	%1, %2	# arch_read_unlock	\n"
> 			"	sub	%1, %3				\n"
> 			"	sc	%1, %0				\n"
> 			: "=" GCC_OFF12_ASM() (rw->lock), "=&r" (tmp)
> 			: GCC_OFF12_ASM() (rw->lock), GCC_ADDI_ASM() (1)
> 			: "memory");
> 
> (untested, but should work) so that there's still a single instruction 
> only in the LL/SC loop and consequently no increased lock contention risk.
> 
>  As a side note, this could be cleaned up to use a "+" input/output 
> constraint; such a clean-up will be welcome -- although to be complete, a 
> review of all the asms will be required (this may bump up the GCC version 
> requirement though, ISTR bugs in this area).
> 
>   Maciej
> 
CC'ing Matthew again for the GAS suggestions

-- 
markos