[PATCH v2 0/4] powerpc/rtas: exports and locking

[PATCH v2 0/4] powerpc/rtas: exports and locking

[Nathan Lynch]

This series began as a single patch[1] to convert the RTAS subsystem's
internal locks to raw spinlocks. The discussion of that patch
identified opportunities to update a few aspects of the RTAS API, so
the series begins with those and ends with a rebased version of the
original patch.

Changes since v1:
- Unexport the singleton 'rtas' struct.
- Remove lock and args fields from 'struct rtas_t', making them
  private to the RTAS subsystem.
- Convert all symbol exports in rtas.c to EXPORT_SYMBOL_GPL.

[1] https://lore.kernel.org/linuxppc-dev/20230110044255.122616-1-nathanl@linux.ibm.com/

Nathan Lynch (4):
  powerpc/rtas: unexport 'rtas' symbol
  powerpc/rtas: make all exports GPL
  powerpc/rtas: remove lock and args fields from global rtas struct
  powerpc/rtas: upgrade internal arch spinlocks

 arch/powerpc/include/asm/rtas-types.h |   2 -
 arch/powerpc/kernel/rtas.c            | 127 +++++++++++---------------
 2 files changed, 55 insertions(+), 74 deletions(-)

-- 
2.37.1

[PATCH v2 1/4] powerpc/rtas: unexport 'rtas' symbol

[Nathan Lynch]

No modular code needs access to the 'rtas' struct, so remove the
symbol export.

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index deded51a7978..6c5716b19d69 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -63,7 +63,6 @@ static inline void do_enter_rtas(unsigned long args)
 struct rtas_t rtas = {
 	.lock = __ARCH_SPIN_LOCK_UNLOCKED
 };
-EXPORT_SYMBOL(rtas);
 
 DEFINE_SPINLOCK(rtas_data_buf_lock);
 EXPORT_SYMBOL(rtas_data_buf_lock);
-- 
2.37.1

Re: [PATCH v2 1/4] powerpc/rtas: unexport 'rtas' symbol

[Andrew Donnellan]

On Tue, 2023-01-24 at 08:04 -0600, Nathan Lynch wrote:
> No modular code needs access to the 'rtas' struct, so remove the
> symbol export.
> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

A quick grep suggests that this is correct, and any modular code that
wants access to the rtas struct would clearly be doing something
extremely cursed that ought to be stopped.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH v2 2/4] powerpc/rtas: make all exports GPL

[Nathan Lynch]

The first symbol exports of RTAS functions and data came with the (now
removed) scanlog driver in 2003:

https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=f92e361842d5251e50562b09664082dcbd0548bb

At the time this was applied, EXPORT_SYMBOL_GPL() was very new, and
the exports of rtas_call() etc have remained non-GPL. As new APIs have
been added to the RTAS subsystem, their symbol exports have followed
the convention set by existing code.

However, the historical evidence is that RTAS function exports have
been added over time only to satisfy the needs of in-kernel users, and
these clients must have fairly intimate knowledge of how the APIs work
to use them safely. No out of tree users are known, and future ones
seem unlikely.

Arguably the default for RTAS symbols should have become
EXPORT_SYMBOL_GPL once it was available. Let's make it so now, and
exceptions can be evaluated as needed.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 6c5716b19d69..e60e2f5af7b9 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -65,10 +65,10 @@ struct rtas_t rtas = {
 };
 
 DEFINE_SPINLOCK(rtas_data_buf_lock);
-EXPORT_SYMBOL(rtas_data_buf_lock);
+EXPORT_SYMBOL_GPL(rtas_data_buf_lock);
 
 char rtas_data_buf[RTAS_DATA_BUF_SIZE] __cacheline_aligned;
-EXPORT_SYMBOL(rtas_data_buf);
+EXPORT_SYMBOL_GPL(rtas_data_buf);
 
 unsigned long rtas_rmo_buf;
 
@@ -77,7 +77,7 @@ unsigned long rtas_rmo_buf;
  * This is done like this so rtas_flash can be a module.
  */
 void (*rtas_flash_term_hook)(int);
-EXPORT_SYMBOL(rtas_flash_term_hook);
+EXPORT_SYMBOL_GPL(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
@@ -325,7 +325,7 @@ void rtas_progress(char *s, unsigned short hex)
  
 	spin_unlock(&progress_lock);
 }
-EXPORT_SYMBOL(rtas_progress);		/* needed by rtas_flash module */
+EXPORT_SYMBOL_GPL(rtas_progress);		/* needed by rtas_flash module */
 
 int rtas_token(const char *service)
 {
@@ -335,13 +335,13 @@ int rtas_token(const char *service)
 	tokp = of_get_property(rtas.dev, service, NULL);
 	return tokp ? be32_to_cpu(*tokp) : RTAS_UNKNOWN_SERVICE;
 }
-EXPORT_SYMBOL(rtas_token);
+EXPORT_SYMBOL_GPL(rtas_token);
 
 int rtas_service_present(const char *service)
 {
 	return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
 }
-EXPORT_SYMBOL(rtas_service_present);
+EXPORT_SYMBOL_GPL(rtas_service_present);
 
 #ifdef CONFIG_RTAS_ERROR_LOGGING
 
@@ -356,7 +356,7 @@ int rtas_get_error_log_max(void)
 {
 	return rtas_error_log_max;
 }
-EXPORT_SYMBOL(rtas_get_error_log_max);
+EXPORT_SYMBOL_GPL(rtas_get_error_log_max);
 
 static void __init init_error_log_max(void)
 {
@@ -584,7 +584,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 	}
 	return ret;
 }
-EXPORT_SYMBOL(rtas_call);
+EXPORT_SYMBOL_GPL(rtas_call);
 
 /**
  * rtas_busy_delay_time() - From an RTAS status value, calculate the
@@ -622,7 +622,7 @@ unsigned int rtas_busy_delay_time(int status)
 
 	return ms;
 }
-EXPORT_SYMBOL(rtas_busy_delay_time);
+EXPORT_SYMBOL_GPL(rtas_busy_delay_time);
 
 /**
  * rtas_busy_delay() - helper for RTAS busy and extended delay statuses
@@ -696,7 +696,7 @@ bool rtas_busy_delay(int status)
 
 	return ret;
 }
-EXPORT_SYMBOL(rtas_busy_delay);
+EXPORT_SYMBOL_GPL(rtas_busy_delay);
 
 static int rtas_error_rc(int rtas_rc)
 {
@@ -741,7 +741,7 @@ int rtas_get_power_level(int powerdomain, int *level)
 		return rtas_error_rc(rc);
 	return rc;
 }
-EXPORT_SYMBOL(rtas_get_power_level);
+EXPORT_SYMBOL_GPL(rtas_get_power_level);
 
 int rtas_set_power_level(int powerdomain, int level, int *setlevel)
 {
@@ -759,7 +759,7 @@ int rtas_set_power_level(int powerdomain, int level, int *setlevel)
 		return rtas_error_rc(rc);
 	return rc;
 }
-EXPORT_SYMBOL(rtas_set_power_level);
+EXPORT_SYMBOL_GPL(rtas_set_power_level);
 
 int rtas_get_sensor(int sensor, int index, int *state)
 {
@@ -777,7 +777,7 @@ int rtas_get_sensor(int sensor, int index, int *state)
 		return rtas_error_rc(rc);
 	return rc;
 }
-EXPORT_SYMBOL(rtas_get_sensor);
+EXPORT_SYMBOL_GPL(rtas_get_sensor);
 
 int rtas_get_sensor_fast(int sensor, int index, int *state)
 {
@@ -820,7 +820,7 @@ bool rtas_indicator_present(int token, int *maxindex)
 
 	return false;
 }
-EXPORT_SYMBOL(rtas_indicator_present);
+EXPORT_SYMBOL_GPL(rtas_indicator_present);
 
 int rtas_set_indicator(int indicator, int index, int new_value)
 {
@@ -838,7 +838,7 @@ int rtas_set_indicator(int indicator, int index, int new_value)
 		return rtas_error_rc(rc);
 	return rc;
 }
-EXPORT_SYMBOL(rtas_set_indicator);
+EXPORT_SYMBOL_GPL(rtas_set_indicator);
 
 /*
  * Ignoring RTAS extended delay
-- 
2.37.1

Re: [PATCH v2 2/4] powerpc/rtas: make all exports GPL

[Laurent Dufour]

On 24/01/2023 15:04:46, Nathan Lynch wrote:
> The first symbol exports of RTAS functions and data came with the (now
> removed) scanlog driver in 2003:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=f92e361842d5251e50562b09664082dcbd0548bb
> 
> At the time this was applied, EXPORT_SYMBOL_GPL() was very new, and
> the exports of rtas_call() etc have remained non-GPL. As new APIs have
> been added to the RTAS subsystem, their symbol exports have followed
> the convention set by existing code.
> 
> However, the historical evidence is that RTAS function exports have
> been added over time only to satisfy the needs of in-kernel users, and
> these clients must have fairly intimate knowledge of how the APIs work
> to use them safely. No out of tree users are known, and future ones
> seem unlikely.
> 
> Arguably the default for RTAS symbols should have become
> EXPORT_SYMBOL_GPL once it was available. Let's make it so now, and
> exceptions can be evaluated as needed.

I also think this is unlikely to happen. But in the case a non GPL driver
needs one of this symbol, I guess it will be hard to move backward once it
is upstream. Crossing fingers!

Reviewed-by: Laurent Dufour <laurent.dufour@fr.ibm.com>

> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 6c5716b19d69..e60e2f5af7b9 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -65,10 +65,10 @@ struct rtas_t rtas = {
>  };
>  
>  DEFINE_SPINLOCK(rtas_data_buf_lock);
> -EXPORT_SYMBOL(rtas_data_buf_lock);
> +EXPORT_SYMBOL_GPL(rtas_data_buf_lock);
>  
>  char rtas_data_buf[RTAS_DATA_BUF_SIZE] __cacheline_aligned;
> -EXPORT_SYMBOL(rtas_data_buf);
> +EXPORT_SYMBOL_GPL(rtas_data_buf);
>  
>  unsigned long rtas_rmo_buf;
>  
> @@ -77,7 +77,7 @@ unsigned long rtas_rmo_buf;
>   * This is done like this so rtas_flash can be a module.
>   */
>  void (*rtas_flash_term_hook)(int);
> -EXPORT_SYMBOL(rtas_flash_term_hook);
> +EXPORT_SYMBOL_GPL(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
> @@ -325,7 +325,7 @@ void rtas_progress(char *s, unsigned short hex)
>   
>  	spin_unlock(&progress_lock);
>  }
> -EXPORT_SYMBOL(rtas_progress);		/* needed by rtas_flash module */
> +EXPORT_SYMBOL_GPL(rtas_progress);		/* needed by rtas_flash module */
>  
>  int rtas_token(const char *service)
>  {
> @@ -335,13 +335,13 @@ int rtas_token(const char *service)
>  	tokp = of_get_property(rtas.dev, service, NULL);
>  	return tokp ? be32_to_cpu(*tokp) : RTAS_UNKNOWN_SERVICE;
>  }
> -EXPORT_SYMBOL(rtas_token);
> +EXPORT_SYMBOL_GPL(rtas_token);
>  
>  int rtas_service_present(const char *service)
>  {
>  	return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
>  }
> -EXPORT_SYMBOL(rtas_service_present);
> +EXPORT_SYMBOL_GPL(rtas_service_present);
>  
>  #ifdef CONFIG_RTAS_ERROR_LOGGING
>  
> @@ -356,7 +356,7 @@ int rtas_get_error_log_max(void)
>  {
>  	return rtas_error_log_max;
>  }
> -EXPORT_SYMBOL(rtas_get_error_log_max);
> +EXPORT_SYMBOL_GPL(rtas_get_error_log_max);
>  
>  static void __init init_error_log_max(void)
>  {
> @@ -584,7 +584,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>  	}
>  	return ret;
>  }
> -EXPORT_SYMBOL(rtas_call);
> +EXPORT_SYMBOL_GPL(rtas_call);
>  
>  /**
>   * rtas_busy_delay_time() - From an RTAS status value, calculate the
> @@ -622,7 +622,7 @@ unsigned int rtas_busy_delay_time(int status)
>  
>  	return ms;
>  }
> -EXPORT_SYMBOL(rtas_busy_delay_time);
> +EXPORT_SYMBOL_GPL(rtas_busy_delay_time);
>  
>  /**
>   * rtas_busy_delay() - helper for RTAS busy and extended delay statuses
> @@ -696,7 +696,7 @@ bool rtas_busy_delay(int status)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL(rtas_busy_delay);
> +EXPORT_SYMBOL_GPL(rtas_busy_delay);
>  
>  static int rtas_error_rc(int rtas_rc)
>  {
> @@ -741,7 +741,7 @@ int rtas_get_power_level(int powerdomain, int *level)
>  		return rtas_error_rc(rc);
>  	return rc;
>  }
> -EXPORT_SYMBOL(rtas_get_power_level);
> +EXPORT_SYMBOL_GPL(rtas_get_power_level);
>  
>  int rtas_set_power_level(int powerdomain, int level, int *setlevel)
>  {
> @@ -759,7 +759,7 @@ int rtas_set_power_level(int powerdomain, int level, int *setlevel)
>  		return rtas_error_rc(rc);
>  	return rc;
>  }
> -EXPORT_SYMBOL(rtas_set_power_level);
> +EXPORT_SYMBOL_GPL(rtas_set_power_level);
>  
>  int rtas_get_sensor(int sensor, int index, int *state)
>  {
> @@ -777,7 +777,7 @@ int rtas_get_sensor(int sensor, int index, int *state)
>  		return rtas_error_rc(rc);
>  	return rc;
>  }
> -EXPORT_SYMBOL(rtas_get_sensor);
> +EXPORT_SYMBOL_GPL(rtas_get_sensor);
>  
>  int rtas_get_sensor_fast(int sensor, int index, int *state)
>  {
> @@ -820,7 +820,7 @@ bool rtas_indicator_present(int token, int *maxindex)
>  
>  	return false;
>  }
> -EXPORT_SYMBOL(rtas_indicator_present);
> +EXPORT_SYMBOL_GPL(rtas_indicator_present);
>  
>  int rtas_set_indicator(int indicator, int index, int new_value)
>  {
> @@ -838,7 +838,7 @@ int rtas_set_indicator(int indicator, int index, int new_value)
>  		return rtas_error_rc(rc);
>  	return rc;
>  }
> -EXPORT_SYMBOL(rtas_set_indicator);
> +EXPORT_SYMBOL_GPL(rtas_set_indicator);
>  
>  /*
>   * Ignoring RTAS extended delay

Re: [PATCH v2 2/4] powerpc/rtas: make all exports GPL

[Andrew Donnellan]

On Tue, 2023-01-24 at 08:04 -0600, Nathan Lynch wrote:
> The first symbol exports of RTAS functions and data came with the
> (now
> removed) scanlog driver in 2003:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=f92e361842d5251e50562b09664082dcbd0548bb
> 
> At the time this was applied, EXPORT_SYMBOL_GPL() was very new, and
> the exports of rtas_call() etc have remained non-GPL. As new APIs
> have
> been added to the RTAS subsystem, their symbol exports have followed
> the convention set by existing code.
> 
> However, the historical evidence is that RTAS function exports have
> been added over time only to satisfy the needs of in-kernel users,
> and
> these clients must have fairly intimate knowledge of how the APIs
> work
> to use them safely. No out of tree users are known, and future ones
> seem unlikely.
> 
> Arguably the default for RTAS symbols should have become
> EXPORT_SYMBOL_GPL once it was available. Let's make it so now, and
> exceptions can be evaluated as needed.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Agreed.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>


-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH v2 3/4] powerpc/rtas: remove lock and args fields from global rtas struct

[Nathan Lynch]

Only code internal to the RTAS subsystem needs access to the central
lock and parameter block. Remove these from the globally visible
'rtas' struct and make them file-static in rtas.c.

Some changed lines in rtas_call() lack appropriate spacing around
operators and cause checkpatch errors; fix these as well.

Suggested-by: Laurent Dufour <ldufour@linux.ibm.com>
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/rtas-types.h |  2 --
 arch/powerpc/kernel/rtas.c            | 50 ++++++++++++++++-----------
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h
index 8df6235d64d1..f2ad4a96cbc5 100644
--- a/arch/powerpc/include/asm/rtas-types.h
+++ b/arch/powerpc/include/asm/rtas-types.h
@@ -18,8 +18,6 @@ struct rtas_t {
 	unsigned long entry;		/* physical address pointer */
 	unsigned long base;		/* physical address pointer */
 	unsigned long size;
-	arch_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 e60e2f5af7b9..0059bb2a8f04 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -60,9 +60,17 @@ static inline void do_enter_rtas(unsigned long args)
 	srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
 }
 
-struct rtas_t rtas = {
-	.lock = __ARCH_SPIN_LOCK_UNLOCKED
-};
+struct rtas_t rtas;
+
+/*
+ * Nearly all RTAS calls need to be serialized. All uses of the
+ * default rtas_args block must hold rtas_lock.
+ *
+ * Exceptions to the RTAS serialization requirement (e.g. stop-self)
+ * must use a separate rtas_args structure.
+ */
+static arch_spinlock_t rtas_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+static struct rtas_args rtas_args;
 
 DEFINE_SPINLOCK(rtas_data_buf_lock);
 EXPORT_SYMBOL_GPL(rtas_data_buf_lock);
@@ -90,13 +98,13 @@ static unsigned long lock_rtas(void)
 
 	local_irq_save(flags);
 	preempt_disable();
-	arch_spin_lock(&rtas.lock);
+	arch_spin_lock(&rtas_lock);
 	return flags;
 }
 
 static void unlock_rtas(unsigned long flags)
 {
-	arch_spin_unlock(&rtas.lock);
+	arch_spin_unlock(&rtas_lock);
 	local_irq_restore(flags);
 	preempt_enable();
 }
@@ -114,7 +122,7 @@ static void call_rtas_display_status(unsigned char c)
 		return;
 
 	s = lock_rtas();
-	rtas_call_unlocked(&rtas.args, 10, 1, 1, NULL, c);
+	rtas_call_unlocked(&rtas_args, 10, 1, 1, NULL, c);
 	unlock_rtas(s);
 }
 
@@ -386,7 +394,7 @@ static int rtas_last_error_token;
  *  most recent failed call to rtas.  Because the error text
  *  might go stale if there are any other intervening rtas calls,
  *  this routine must be called atomically with whatever produced
- *  the error (i.e. with rtas.lock still held from the previous call).
+ *  the error (i.e. with rtas_lock still held from the previous call).
  */
 static char *__fetch_rtas_last_error(char *altbuf)
 {
@@ -406,13 +414,13 @@ static char *__fetch_rtas_last_error(char *altbuf)
 	err_args.args[1] = cpu_to_be32(bufsz);
 	err_args.args[2] = 0;
 
-	save_args = rtas.args;
-	rtas.args = err_args;
+	save_args = rtas_args;
+	rtas_args = err_args;
 
-	do_enter_rtas(__pa(&rtas.args));
+	do_enter_rtas(__pa(&rtas_args));
 
-	err_args = rtas.args;
-	rtas.args = save_args;
+	err_args = rtas_args;
+	rtas_args = save_args;
 
 	/* Log the error in the unlikely case that there was one. */
 	if (unlikely(err_args.args[2] == 0)) {
@@ -534,7 +542,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 	va_list list;
 	int i;
 	unsigned long s;
-	struct rtas_args *rtas_args;
+	struct rtas_args *args;
 	char *buff_copy = NULL;
 	int ret;
 
@@ -559,21 +567,21 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 	s = lock_rtas();
 
 	/* We use the global rtas args buffer */
-	rtas_args = &rtas.args;
+	args = &rtas_args;
 
 	va_start(list, outputs);
-	va_rtas_call_unlocked(rtas_args, token, nargs, nret, list);
+	va_rtas_call_unlocked(args, token, nargs, nret, list);
 	va_end(list);
 
 	/* A -1 return code indicates that the last command couldn't
 	   be completed due to a hardware error. */
-	if (be32_to_cpu(rtas_args->rets[0]) == -1)
+	if (be32_to_cpu(args->rets[0]) == -1)
 		buff_copy = __fetch_rtas_last_error(NULL);
 
 	if (nret > 1 && outputs != NULL)
 		for (i = 0; i < nret-1; ++i)
-			outputs[i] = be32_to_cpu(rtas_args->rets[i+1]);
-	ret = (nret > 0)? be32_to_cpu(rtas_args->rets[0]): 0;
+			outputs[i] = be32_to_cpu(args->rets[i + 1]);
+	ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0;
 
 	unlock_rtas(s);
 
@@ -1269,9 +1277,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 
 	flags = lock_rtas();
 
-	rtas.args = args;
-	do_enter_rtas(__pa(&rtas.args));
-	args = rtas.args;
+	rtas_args = args;
+	do_enter_rtas(__pa(&rtas_args));
+	args = rtas_args;
 
 	/* A -1 return code indicates that the last command couldn't
 	   be completed due to a hardware error. */
-- 
2.37.1

Re: [PATCH v2 3/4] powerpc/rtas: remove lock and args fields from global rtas struct

[Laurent Dufour]

On 24/01/2023 15:04:47, Nathan Lynch wrote:
> Only code internal to the RTAS subsystem needs access to the central
> lock and parameter block. Remove these from the globally visible
> 'rtas' struct and make them file-static in rtas.c.
> 
> Some changed lines in rtas_call() lack appropriate spacing around
> operators and cause checkpatch errors; fix these as well.

Reviewed-by: Laurent Dufour <laurent.dufour@fr.ibm.com>

> 
> Suggested-by: Laurent Dufour <ldufour@linux.ibm.com>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/rtas-types.h |  2 --
>  arch/powerpc/kernel/rtas.c            | 50 ++++++++++++++++-----------
>  2 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h
> index 8df6235d64d1..f2ad4a96cbc5 100644
> --- a/arch/powerpc/include/asm/rtas-types.h
> +++ b/arch/powerpc/include/asm/rtas-types.h
> @@ -18,8 +18,6 @@ struct rtas_t {
>  	unsigned long entry;		/* physical address pointer */
>  	unsigned long base;		/* physical address pointer */
>  	unsigned long size;
> -	arch_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 e60e2f5af7b9..0059bb2a8f04 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -60,9 +60,17 @@ static inline void do_enter_rtas(unsigned long args)
>  	srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
>  }
>  
> -struct rtas_t rtas = {
> -	.lock = __ARCH_SPIN_LOCK_UNLOCKED
> -};
> +struct rtas_t rtas;
> +
> +/*
> + * Nearly all RTAS calls need to be serialized. All uses of the
> + * default rtas_args block must hold rtas_lock.
> + *
> + * Exceptions to the RTAS serialization requirement (e.g. stop-self)
> + * must use a separate rtas_args structure.
> + */
> +static arch_spinlock_t rtas_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +static struct rtas_args rtas_args;
>  
>  DEFINE_SPINLOCK(rtas_data_buf_lock);
>  EXPORT_SYMBOL_GPL(rtas_data_buf_lock);
> @@ -90,13 +98,13 @@ static unsigned long lock_rtas(void)
>  
>  	local_irq_save(flags);
>  	preempt_disable();
> -	arch_spin_lock(&rtas.lock);
> +	arch_spin_lock(&rtas_lock);
>  	return flags;
>  }
>  
>  static void unlock_rtas(unsigned long flags)
>  {
> -	arch_spin_unlock(&rtas.lock);
> +	arch_spin_unlock(&rtas_lock);
>  	local_irq_restore(flags);
>  	preempt_enable();
>  }
> @@ -114,7 +122,7 @@ static void call_rtas_display_status(unsigned char c)
>  		return;
>  
>  	s = lock_rtas();
> -	rtas_call_unlocked(&rtas.args, 10, 1, 1, NULL, c);
> +	rtas_call_unlocked(&rtas_args, 10, 1, 1, NULL, c);
>  	unlock_rtas(s);
>  }
>  
> @@ -386,7 +394,7 @@ static int rtas_last_error_token;
>   *  most recent failed call to rtas.  Because the error text
>   *  might go stale if there are any other intervening rtas calls,
>   *  this routine must be called atomically with whatever produced
> - *  the error (i.e. with rtas.lock still held from the previous call).
> + *  the error (i.e. with rtas_lock still held from the previous call).
>   */
>  static char *__fetch_rtas_last_error(char *altbuf)
>  {
> @@ -406,13 +414,13 @@ static char *__fetch_rtas_last_error(char *altbuf)
>  	err_args.args[1] = cpu_to_be32(bufsz);
>  	err_args.args[2] = 0;
>  
> -	save_args = rtas.args;
> -	rtas.args = err_args;
> +	save_args = rtas_args;
> +	rtas_args = err_args;
>  
> -	do_enter_rtas(__pa(&rtas.args));
> +	do_enter_rtas(__pa(&rtas_args));
>  
> -	err_args = rtas.args;
> -	rtas.args = save_args;
> +	err_args = rtas_args;
> +	rtas_args = save_args;
>  
>  	/* Log the error in the unlikely case that there was one. */
>  	if (unlikely(err_args.args[2] == 0)) {
> @@ -534,7 +542,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>  	va_list list;
>  	int i;
>  	unsigned long s;
> -	struct rtas_args *rtas_args;
> +	struct rtas_args *args;
>  	char *buff_copy = NULL;
>  	int ret;
>  
> @@ -559,21 +567,21 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>  	s = lock_rtas();
>  
>  	/* We use the global rtas args buffer */
> -	rtas_args = &rtas.args;
> +	args = &rtas_args;
>  
>  	va_start(list, outputs);
> -	va_rtas_call_unlocked(rtas_args, token, nargs, nret, list);
> +	va_rtas_call_unlocked(args, token, nargs, nret, list);
>  	va_end(list);
>  
>  	/* A -1 return code indicates that the last command couldn't
>  	   be completed due to a hardware error. */
> -	if (be32_to_cpu(rtas_args->rets[0]) == -1)
> +	if (be32_to_cpu(args->rets[0]) == -1)
>  		buff_copy = __fetch_rtas_last_error(NULL);
>  
>  	if (nret > 1 && outputs != NULL)
>  		for (i = 0; i < nret-1; ++i)
> -			outputs[i] = be32_to_cpu(rtas_args->rets[i+1]);
> -	ret = (nret > 0)? be32_to_cpu(rtas_args->rets[0]): 0;
> +			outputs[i] = be32_to_cpu(args->rets[i + 1]);
> +	ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0;
>  
>  	unlock_rtas(s);
>  
> @@ -1269,9 +1277,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  
>  	flags = lock_rtas();
>  
> -	rtas.args = args;
> -	do_enter_rtas(__pa(&rtas.args));
> -	args = rtas.args;
> +	rtas_args = args;
> +	do_enter_rtas(__pa(&rtas_args));
> +	args = rtas_args;
>  
>  	/* A -1 return code indicates that the last command couldn't
>  	   be completed due to a hardware error. */

Re: [PATCH v2 3/4] powerpc/rtas: remove lock and args fields from global rtas struct

[Andrew Donnellan]

On Tue, 2023-01-24 at 08:04 -0600, Nathan Lynch wrote:
> Only code internal to the RTAS subsystem needs access to the central
> lock and parameter block. Remove these from the globally visible
> 'rtas' struct and make them file-static in rtas.c.
> 
> Some changed lines in rtas_call() lack appropriate spacing around
> operators and cause checkpatch errors; fix these as well.
> 
> Suggested-by: Laurent Dufour <ldufour@linux.ibm.com>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

[PATCH v2 4/4] powerpc/rtas: upgrade internal arch spinlocks

[Nathan Lynch]

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 <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 52 ++++++++++----------------------------
 1 file changed, 14 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 0059bb2a8f04..c02edec3c860 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -69,7 +69,7 @@ struct rtas_t rtas;
  * Exceptions to the RTAS serialization requirement (e.g. stop-self)
  * must use a separate rtas_args structure.
  */
-static arch_spinlock_t rtas_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+static DEFINE_RAW_SPINLOCK(rtas_lock);
 static struct rtas_args rtas_args;
 
 DEFINE_SPINLOCK(rtas_data_buf_lock);
@@ -87,28 +87,6 @@ unsigned long rtas_rmo_buf;
 void (*rtas_flash_term_hook)(int);
 EXPORT_SYMBOL_GPL(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(&rtas_lock);
-	return flags;
-}
-
-static void unlock_rtas(unsigned long flags)
-{
-	arch_spin_unlock(&rtas_lock);
-	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
@@ -116,14 +94,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(&rtas_lock, flags);
 	rtas_call_unlocked(&rtas_args, 10, 1, 1, NULL, c);
-	unlock_rtas(s);
+	raw_spin_unlock_irqrestore(&rtas_lock, flags);
 }
 
 static void call_rtas_display_status_delay(char c)
@@ -541,7 +519,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 *args;
 	char *buff_copy = NULL;
 	int ret;
@@ -564,8 +542,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 		return -1;
 	}
 
-	s = lock_rtas();
-
+	raw_spin_lock_irqsave(&rtas_lock, flags);
 	/* We use the global rtas args buffer */
 	args = &rtas_args;
 
@@ -583,7 +560,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 			outputs[i] = be32_to_cpu(args->rets[i + 1]);
 	ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0;
 
-	unlock_rtas(s);
+	raw_spin_unlock_irqrestore(&rtas_lock, flags);
 
 	if (buff_copy) {
 		log_error(buff_copy, ERR_TYPE_RTAS_LOG, 0);
@@ -1275,7 +1252,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 
 	buff_copy = get_errorlog_buffer();
 
-	flags = lock_rtas();
+	raw_spin_lock_irqsave(&rtas_lock, flags);
 
 	rtas_args = args;
 	do_enter_rtas(__pa(&rtas_args));
@@ -1286,7 +1263,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 	if (be32_to_cpu(args.rets[0]) == -1)
 		errbuf = __fetch_rtas_last_error(buff_copy);
 
-	unlock_rtas(flags);
+	raw_spin_unlock_irqrestore(&rtas_lock, flags);
 
 	if (buff_copy) {
 		if (errbuf)
@@ -1408,19 +1385,18 @@ int __init early_init_dt_scan_rtas(unsigned long node,
 	return 1;
 }
 
-static arch_spinlock_t timebase_lock;
+static DEFINE_RAW_SPINLOCK(timebase_lock);
 static u64 timebase = 0;
 
 void rtas_give_timebase(void)
 {
 	unsigned long flags;
 
-	local_irq_save(flags);
+	raw_spin_lock_irqsave(&timebase_lock, flags);
 	hard_irq_disable();
-	arch_spin_lock(&timebase_lock);
 	rtas_call(rtas_token("freeze-time-base"), 0, 1, NULL);
 	timebase = get_tb();
-	arch_spin_unlock(&timebase_lock);
+	raw_spin_unlock(&timebase_lock);
 
 	while (timebase)
 		barrier();
@@ -1432,8 +1408,8 @@ void rtas_take_timebase(void)
 {
 	while (!timebase)
 		barrier();
-	arch_spin_lock(&timebase_lock);
+	raw_spin_lock(&timebase_lock);
 	set_tb(timebase >> 32, timebase & 0xffffffff);
 	timebase = 0;
-	arch_spin_unlock(&timebase_lock);
+	raw_spin_unlock(&timebase_lock);
 }
-- 
2.37.1

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

[Andrew Donnellan]

On Tue, 2023-01-24 at 08:04 -0600, 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 <nathanl@linux.ibm.com>

I'm no locking expert but this looks reasonable from a quick read-
through.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH v2 0/4] powerpc/rtas: exports and locking

[Nathan Lynch]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> This series began as a single patch[1] to convert the RTAS subsystem's
> internal locks to raw spinlocks. The discussion of that patch
> identified opportunities to update a few aspects of the RTAS API, so
> the series begins with those and ends with a rebased version of the
> original patch.
>
> Changes since v1:
> - Unexport the singleton 'rtas' struct.
> - Remove lock and args fields from 'struct rtas_t', making them
>   private to the RTAS subsystem.
> - Convert all symbol exports in rtas.c to EXPORT_SYMBOL_GPL.
>
> [1] https://lore.kernel.org/linuxppc-dev/20230110044255.122616-1-nathanl@linux.ibm.com/
>
> Nathan Lynch (4):
>   powerpc/rtas: unexport 'rtas' symbol
>   powerpc/rtas: make all exports GPL
>   powerpc/rtas: remove lock and args fields from global rtas struct
>   powerpc/rtas: upgrade internal arch spinlocks
>
>  arch/powerpc/include/asm/rtas-types.h |   2 -
>  arch/powerpc/kernel/rtas.c            | 127 +++++++++++---------------
>  2 files changed, 55 insertions(+), 74 deletions(-)

Note this series conflicts with my earlier series "[PATCH v2 0/4] RTAS
function table and tracepoints":

https://lore.kernel.org/linuxppc-dev/20221212230154.851325-1-nathanl@linux.ibm.com/

I'll plan on rebasing the tracepoint series, which is more
disruptive/ambitious, on this one. Let me know if I should do
otherwise.

To be transparent, I have a fair amount of RTAS-oriented but otherwise
loosely related work in progress and I'm struggling to keep it organized
and establish a submission/review cadence. Having conflicting series
pending probably is not great :-(

Should I maintain a single stack of patches over time to avoid conflicts
like this, even though there may not be a unifying theme beyond it all
being generally RTAS-related?

Re: [PATCH v2 0/4] powerpc/rtas: exports and locking

[Michael Ellerman]

On Tue, 24 Jan 2023 08:04:44 -0600, Nathan Lynch wrote:
> This series began as a single patch[1] to convert the RTAS subsystem's
> internal locks to raw spinlocks. The discussion of that patch
> identified opportunities to update a few aspects of the RTAS API, so
> the series begins with those and ends with a rebased version of the
> original patch.
> 
> Changes since v1:
> - Unexport the singleton 'rtas' struct.
> - Remove lock and args fields from 'struct rtas_t', making them
>   private to the RTAS subsystem.
> - Convert all symbol exports in rtas.c to EXPORT_SYMBOL_GPL.
> 
> [...]

Applied to powerpc/next.

[1/4] powerpc/rtas: unexport 'rtas' symbol
      https://git.kernel.org/powerpc/c/5ff92e2f274dc42a9e534473121273cd209d3501
[2/4] powerpc/rtas: make all exports GPL
      https://git.kernel.org/powerpc/c/9bce6243848dfd0ff7c2be6e8d82ab9b1e6c7858
[3/4] powerpc/rtas: remove lock and args fields from global rtas struct
      https://git.kernel.org/powerpc/c/599af49155467148afaf0bc3c0114bd80fd4491f
[4/4] powerpc/rtas: upgrade internal arch spinlocks
      https://git.kernel.org/powerpc/c/12fd66651df6c807b7b6f420ee0fd420f54991f4

cheers