Michael Ellerman <m...@ellerman.id.au> writes: > 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.
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).