Re: [PATCH] powerpc/rtas: upgrade internal arch spinlocks

[Nathan Lynch <nathanl@linux.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> Laurent Dufour <ldufour@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.

Agreed, I see no accesses of the rtas struct from code that can be built
as a module, and we can introduce nicer accessor functions in the future
if need arises. I will incorporate removal of EXPORT_SYMBOL(rtas).