[PATCH] powerpc/rtas: upgrade internal arch spinlocks

[Michael Ellerman]

Nathan Lynch <nathanl at linux.ibm.com> writes:
> Laurent Dufour <ldufour at linux.ibm.com> writes:
>> On 10/01/2023 05:42:55, Nathan Lynch wrote:
>>> --- a/arch/powerpc/include/asm/rtas-types.h
>>> +++ b/arch/powerpc/include/asm/rtas-types.h
>>> @@ -18,7 +18,7 @@ struct rtas_t {
>>>  	unsigned long entry;		/* physical address pointer */
>>>  	unsigned long base;		/* physical address pointer */
>>>  	unsigned long size;
>>> -	arch_spinlock_t lock;
>>> +	raw_spinlock_t lock;
>>>  	struct rtas_args args;
>>>  	struct device_node *dev;	/* virtual address pointer */
>>>  };
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index deded51a7978..a834726f18e3 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -61,7 +61,7 @@ static inline void do_enter_rtas(unsigned long args)
>>>  }
>>>  
>>>  struct rtas_t rtas = {
>>> -	.lock = __ARCH_SPIN_LOCK_UNLOCKED
>>> +	.lock = __RAW_SPIN_LOCK_UNLOCKED(rtas.lock),
>>>  };
>>>  EXPORT_SYMBOL(rtas);
>>
>> This is not the scope of this patch, but the RTAS's lock is externalized
>> through the structure rtas_t, while it is only used in that file.
>>
>> I think, this would be good, in case of future change about that lock, and
>> in order to not break KABI, to move it out of that structure, and to define
>> it statically in that file.
>
> Thanks for pointing this out.
>
> /* rtas-types.h */
> struct rtas_t {
> 	unsigned long entry;		/* physical address pointer */
> 	unsigned long base;		/* physical address pointer */
> 	unsigned long size;
> 	raw_spinlock_t lock;
> 	struct rtas_args args;
> 	struct device_node *dev;	/* virtual address pointer */
> };
>
> /* rtas.h */
> extern struct rtas_t rtas;
>
> There's C and asm code outside of rtas.c that accesses rtas.entry,
> rtas.base, rtas.size, and rtas.dev. But as you say, rtas.lock is used
> only in rtas.c, and it's hard to imagine any legitimate external
> use. This applies to the args member as well, since accesses must occur
> under the lock.
>
> Making the lock and args private to rtas.c seems desirable on its own,
> so I think that should be done first as a cleanup, followed by the
> riskier arch -> raw lock conversion.

I don't see any reason why `rtas` is exported at all.

There might have been in the past, but I don't see one any more.

So I'd be happy if we removed the EXPORT entirely. If it breaks
something we can always put it back.

cheers