Nathan Lynch [off-list ref] writes:

Laurent Dufour [off-list ref] writes:

quoted

On 10/01/2023 05:42:55, Nathan Lynch wrote:

quoted

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