Re: [PATCH] powerpc/rtas: upgrade internal arch spinlocks
Michael Ellerman writes:
> Nathan Lynch writes:
>> Laurent Dufour 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).
Re: [PATCH] powerpc/rtas: upgrade internal arch spinlocks
Nathan Lynch writes:
> Laurent Dufour 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
Re: [PATCH] powerpc/rtas: upgrade internal arch spinlocks
Laurent Dufour 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.
Re: [PATCH] powerpc/rtas: upgrade internal arch spinlocks
On 10/01/2023 05:42:55, Nathan Lynch wrote:
> At the time commit f97bb36f705d ("powerpc/rtas: Turn rtas lock into a
> raw spinlock") was written, the spinlock lockup detection code called
> __delay(), which will not make progress if the timebase is not
> advancing. Since the interprocessor timebase synchronization sequence
> for chrp, cell, and some now-unsupported Power models can temporarily
> freeze the timebase through an RTAS function (freeze-time-base), the
> lock that serializes most RTAS calls was converted to arch_spinlock_t
> to prevent kernel hangs in the lockup detection code.
>
> However, commit bc88c10d7e69 ("locking/spinlock/debug: Remove spinlock
> lockup detection code") removed that inconvenient property from the
> lock debug code several years ago. So now it should be safe to
> reintroduce generic locks into the RTAS support code, primarily to
> increase lockdep coverage.
>
> Making rtas.lock a spinlock_t would violate lock type nesting rules
> because it can be acquired while holding raw locks, e.g. pci_lock and
> irq_desc->lock. So convert it to raw_spinlock_t. There's no apparent
> reason not to upgrade timebase_lock as well.
>
> Signed-off-by: Nathan Lynch
> ---
> arch/powerpc/include/asm/rtas-types.h | 2 +-
> arch/powerpc/kernel/rtas.c| 52 ---
> 2 files changed, 15 insertions(+), 39 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/rtas-types.h
> b/arch/powerpc/include/asm/rtas-types.h
> index 8df6235d64d1..a58f96eb2d19 100644
> --- 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.
Otherwise, looks good to me.
Reviewed-by: Laurent Dufour
>
> @@ -80,28 +80,6 @@ unsigned long rtas_rmo_buf;
> void (*rtas_flash_term_hook)(int);
> EXPORT_SYMBOL(rtas_flash_term_hook);
>
> -/* RTAS use home made raw locking instead of spin_lock_irqsave
> - * because those can be called from within really nasty contexts
> - * such as having the timebase stopped which would lockup with
> - * normal locks and spinlock debugging enabled
> - */
> -static unsigned long lock_rtas(void)
> -{
> - unsigned long flags;
> -
> - local_irq_save(flags);
> - preempt_disable();
> - arch_spin_lock();
> - return flags;
> -}
> -
> -static void unlock_rtas(unsigned long flags)
> -{
> - arch_spin_unlock();
> - local_irq_restore(flags);
> - preempt_enable();
> -}
> -> /*
> * call_rtas_display_status and call_rtas_display_status_delay
> * are designed only for very early low-level debugging, which
> @@ -109,14 +87,14 @@ static void unlock_rtas(unsigned long flags)
> */
> static void call_rtas_display_status(unsigned char c)
> {
> - unsigned long s;
> + unsigned long flags;
>
> if (!rtas.base)
> return;
>
> - s = lock_rtas();
> + raw_spin_lock_irqsave(, flags);
> rtas_call_unlocked(, 10, 1, 1, NULL, c);
> - unlock_rtas(s);
> + raw_spin_unlock_irqrestore(, flags);
> }
>
> static void call_rtas_display_status_delay(char c)
> @@ -534,7 +512,7 @@ int rtas_call(int token, int nargs, int nret, int
> *outputs, ...)
> {
> va_list list;
> int i;
> - unsigned long s;
> + unsigned long flags;
> struct rtas_args *rtas_args;
> char *buff_copy = NULL;
> int ret;
> @@ -557,8 +535,7 @@ int rtas_call(int token, int nargs, int nret, int
> *outputs, ...)
> return -1;
> }
>
> - s = lock_rtas();
> -
> + raw_spin_lock_irqsave(, flags);
> /* We use the global rtas args buffer */
> rtas_args =
>
> @@ -576,7 +553,7 @@ int rtas_call(int token, int nargs, int nret, int
> *outputs, ...)
> outputs[i] = be32_to_cpu(rtas_args->rets[i+1]);
> ret = (nret > 0)? be32_to_cpu(rtas_args->rets[0]): 0;
>
> - unlock_rtas(s);
> + raw_spin_unlock_irqrestore(, flags);
>
