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@xxxxxxxxxx>
>> ---
>> 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
