Nathan Lynch <nath...@linux.ibm.com> writes:
> Laurent Dufour <lduf...@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
