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

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

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'll tentatively plan on doing that for a v2, pending further comments.