[PATCH] use msleep() for RTAS delays

[PATCH] use msleep() for RTAS delays

[John Rose]

The current use of udelay() for RTAS extended delay conditions can
result in CPU soft lockups.  The use of msleep() won't tie up the CPU.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

Thanks-
John

diff -puN arch/powerpc/kernel/rtas.c~msleep_rtas arch/powerpc/kernel/rtas.c
--- 2_6_linus/arch/powerpc/kernel/rtas.c~msleep_rtas	2006-05-31 14:07:51.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c	2006-05-31 14:10:09.000000000 -0500
@@ -447,10 +447,10 @@ int rtas_set_power_level(int powerdomain
 	while (1) {
 		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
 		if (rc == RTAS_BUSY)
-			udelay(1);
+			msleep(1);
 		else if (rtas_is_extended_busy(rc)) {
 			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
+			msleep(wait_time);
 		} else
 			break;
 	}
@@ -472,10 +472,10 @@ int rtas_get_sensor(int sensor, int inde
 	while (1) {
 		rc = rtas_call(token, 2, 2, state, sensor, index);
 		if (rc == RTAS_BUSY)
-			udelay(1);
+			msleep(1);
 		else if (rtas_is_extended_busy(rc)) {
 			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
+			msleep(wait_time);
 		} else
 			break;
 	}
@@ -497,12 +497,11 @@ int rtas_set_indicator(int indicator, in
 	while (1) {
 		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
 		if (rc == RTAS_BUSY)
-			udelay(1);
+			msleep(1);
 		else if (rtas_is_extended_busy(rc)) {
 			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		}
-		else
+			msleep(wait_time);
+		} else
 			break;
 	}
 
diff -puN arch/powerpc/kernel/rtas_flash.c~msleep_rtas arch/powerpc/kernel/rtas_flash.c
--- 2_6_linus/arch/powerpc/kernel/rtas_flash.c~msleep_rtas	2006-05-31 14:10:47.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-05-31 14:19:30.000000000 -0500
@@ -16,7 +16,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/proc_fs.h>
-#include <asm/delay.h>
+#include <linux/delay.h>
 #include <asm/uaccess.h>
 #include <asm/rtas.h>
 #include <asm/abs_addr.h>
@@ -372,10 +372,10 @@ static void manage_flash(struct rtas_man
 		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 
 			       1, NULL, args_buf->op);
 		if (rc == RTAS_RC_BUSY)
-			udelay(1);
+			msleep(1);
 		else if (rtas_is_extended_busy(rc)) {
 			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
+			msleep(wait_time);
 		} else
 			break;
 	}
@@ -465,10 +465,10 @@ static void validate_flash(struct rtas_v
 		spin_unlock(&rtas_data_buf_lock);
 			
 		if (rc == RTAS_RC_BUSY)
-			udelay(1);
+			msleep(1);
 		else if (rtas_is_extended_busy(rc)) {
 			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
+			msleep(wait_time);
 		} else
 			break;
 	}

_

Re: [PATCH] use msleep() for RTAS delays

[Benjamin Herrenschmidt]

On Wed, 2006-05-31 at 14:32 -0500, John Rose wrote:
> The current use of udelay() for RTAS extended delay conditions can
> result in CPU soft lockups.  The use of msleep() won't tie up the CPU.

What about putting this whole thing in a helper ? There is a few more
things I've seen floating around implementing the exact same logic (for
example Jake's MSI patches).

We could either do something like

for (;;) {
	rc = rtas_call(...);
	if (!rtas_check_busy(rc))
		break;
}

Or something inside rtas_call

rtas_call_waitbusy(...);

Ben.

> Signed-off-by: John Rose <johnrose@austin.ibm.com>
> 
> ---
> 
> Thanks-
> John
> 
> diff -puN arch/powerpc/kernel/rtas.c~msleep_rtas arch/powerpc/kernel/rtas.c
> --- 2_6_linus/arch/powerpc/kernel/rtas.c~msleep_rtas	2006-05-31 14:07:51.000000000 -0500
> +++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c	2006-05-31 14:10:09.000000000 -0500
> @@ -447,10 +447,10 @@ int rtas_set_power_level(int powerdomain
>  	while (1) {
>  		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
>  		if (rc == RTAS_BUSY)
> -			udelay(1);
> +			msleep(1);
>  		else if (rtas_is_extended_busy(rc)) {
>  			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> +			msleep(wait_time);
>  		} else
>  			break;
>  	}
> @@ -472,10 +472,10 @@ int rtas_get_sensor(int sensor, int inde
>  	while (1) {
>  		rc = rtas_call(token, 2, 2, state, sensor, index);
>  		if (rc == RTAS_BUSY)
> -			udelay(1);
> +			msleep(1);
>  		else if (rtas_is_extended_busy(rc)) {
>  			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> +			msleep(wait_time);
>  		} else
>  			break;
>  	}
> @@ -497,12 +497,11 @@ int rtas_set_indicator(int indicator, in
>  	while (1) {
>  		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
>  		if (rc == RTAS_BUSY)
> -			udelay(1);
> +			msleep(1);
>  		else if (rtas_is_extended_busy(rc)) {
>  			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		}
> -		else
> +			msleep(wait_time);
> +		} else
>  			break;
>  	}
>  
> diff -puN arch/powerpc/kernel/rtas_flash.c~msleep_rtas arch/powerpc/kernel/rtas_flash.c
> --- 2_6_linus/arch/powerpc/kernel/rtas_flash.c~msleep_rtas	2006-05-31 14:10:47.000000000 -0500
> +++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-05-31 14:19:30.000000000 -0500
> @@ -16,7 +16,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/proc_fs.h>
> -#include <asm/delay.h>
> +#include <linux/delay.h>
>  #include <asm/uaccess.h>
>  #include <asm/rtas.h>
>  #include <asm/abs_addr.h>
> @@ -372,10 +372,10 @@ static void manage_flash(struct rtas_man
>  		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 
>  			       1, NULL, args_buf->op);
>  		if (rc == RTAS_RC_BUSY)
> -			udelay(1);
> +			msleep(1);
>  		else if (rtas_is_extended_busy(rc)) {
>  			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> +			msleep(wait_time);
>  		} else
>  			break;
>  	}
> @@ -465,10 +465,10 @@ static void validate_flash(struct rtas_v
>  		spin_unlock(&rtas_data_buf_lock);
>  			
>  		if (rc == RTAS_RC_BUSY)
> -			udelay(1);
> +			msleep(1);
>  		else if (rtas_is_extended_busy(rc)) {
>  			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> +			msleep(wait_time);
>  		} else
>  			break;
>  	}
> 
> _
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] use msleep() for RTAS delays

[Paul Mackerras]

Benjamin Herrenschmidt writes:

> We could either do something like
> 
> for (;;) {
> 	rc = rtas_call(...);
> 	if (!rtas_check_busy(rc))
> 		break;

rtas_check_busy might need some state, such as the number of times we
have seen an RTAS_BUSY return value.

> Or something inside rtas_call
> 
> rtas_call_waitbusy(...);

That's a good idea...

Paul.

Re: [PATCH] use msleep() for RTAS delays

[Arnd Bergmann]

On Thursday 01 June 2006 07:39, Paul Mackerras wrote:
> 
> > rtas_call_waitbusy(...);
> 
> That's a good idea...

Do we also need an atomic version of that? Maybe there are
places where we actually need to call a slow rtas call
under a spinlock.

At the very least, rtas_call_waitbusy() should have a
might_sleep() in it to find those places.

	Arnd <><

Re: [PATCH] use msleep() for RTAS delays

[John Rose]

Hi Ben-

> What about putting this whole thing in a helper ? There is a few more
> things I've seen floating around implementing the exact same logic (for
> example Jake's MSI patches).

I don't mind writing this up.  The previous patch fixes a reported bug
with minimal reorg.  If possible, I'd like to restructure on top of
that.

> Or something inside rtas_call
> 
> rtas_call_waitbusy(...);

Certain RTAS calls expect identical inputs between successive calls,
while others (ibm,change-msi) expect modified "sequence" inputs, etc. 
Given this inconsistency, we probably can't handle busy rcs inside
rtas_call().

Thanks-
John

Re: [PATCH] use msleep() for RTAS delays

[John Rose]

> I don't mind writing this up.  The previous patch fixes a reported bug
> with minimal reorg.  If possible, I'd like to restructure on top of
> that.

On second thought, why not reorg now!  I'll have a patch shortly.

Thanks-
John

[PATCH] reorg RTAS delay code

[John Rose]

This patch attempts to handle RTAS "busy" return codes in a more simple
and consistent manner.  Typical callers of RTAS shouldn't have to
manage wait times and delay calls.

This patch also changes the kernel to use msleep() rather than udelay()
when a runtime delay is necessary.  This will avoid CPU soft lockups
for extended delay conditions.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

This obsoletes the patch in the parent thread, "use msleep for RTAS 
delays".  Sniff tested on POWER5.

Thanks-
John

 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c   |   30 +++---
 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c       |   86 ++++++++------------
 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c |   25 -----
 2_6_linus-johnrose/include/asm-powerpc/rtas.h       |    8 -
 4 files changed, 57 insertions(+), 92 deletions(-)

diff -puN arch/powerpc/kernel/rtas.c~rtas_delay_reorg arch/powerpc/kernel/rtas.c
--- 2_6_linus/arch/powerpc/kernel/rtas.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c	2006-06-02 15:22:37.000000000 -0500
@@ -370,24 +370,35 @@ int rtas_call(int token, int nargs, int 
 	return ret;
 }
 
-/* Given an RTAS status code of 990n compute the hinted delay of 10^n
- * (last digit) milliseconds.  For now we bound at n=5 (100 sec).
+/* For RTAS_BUSY (-2), delay for 1 millisecond.  For an extended busy status
+ * code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
  */
-unsigned int rtas_extended_busy_delay_time(int status)
+unsigned int rtas_busy_delay_time(int status)
 {
-	int order = status - 9900;
-	unsigned long ms;
+	int order;
+	unsigned int ms = 0;
 
-	if (order < 0)
-		order = 0;	/* RTC depends on this for -2 clock busy */
-	else if (order > 5)
-		order = 5;	/* bound */
+	if (status == RTAS_BUSY) {
+		ms = 1;
+	} else if (status >= 9900 && status <= 9905) {
+		order = status - 9900;
+		for (ms = 1; order > 0; order--)
+			ms *= 10;
+	}
+
+	return ms;
+}
 
-	/* Use microseconds for reasonable accuracy */
-	for (ms = 1; order > 0; order--)
-		ms *= 10;
+/* For an RTAS busy status code, perform the hinted delay. */
+unsigned int rtas_busy_delay(int status)
+{
+	unsigned int ms;
 
-	return ms; 
+	ms = rtas_busy_delay_time(status);
+	if (ms)
+		msleep(ms);
+
+	return ms;
 }
 
 int rtas_error_rc(int rtas_rc)
@@ -438,22 +449,14 @@ int rtas_get_power_level(int powerdomain
 int rtas_set_power_level(int powerdomain, int level, int *setlevel)
 {
 	int token = rtas_token("set-power-level");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do
 		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -463,22 +466,14 @@ int rtas_set_power_level(int powerdomain
 int rtas_get_sensor(int sensor, int index, int *state)
 {
 	int token = rtas_token("get-sensor-state");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do
 		rc = rtas_call(token, 2, 2, state, sensor, index);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -488,23 +483,14 @@ int rtas_get_sensor(int sensor, int inde
 int rtas_set_indicator(int indicator, int index, int new_value)
 {
 	int token = rtas_token("set-indicator");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do
 		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		}
-		else
-			break;
-	}
+	while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -552,16 +538,14 @@ void rtas_os_term(char *str)
 
 	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
 
-	do {
+	do
 		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
 				   __pa(rtas_os_term_buf));
+	while (rtas_busy_delay(status));
 
-		if (status == RTAS_BUSY)
-			udelay(1);
-		else if (status != 0)
-			printk(KERN_EMERG "ibm,os-term call failed %d\n",
+	if (status != 0)
+		printk(KERN_EMERG "ibm,os-term call failed %d\n",
 			       status);
-	} while (status == RTAS_BUSY);
 }
 
 static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
@@ -789,7 +773,7 @@ EXPORT_SYMBOL(rtas_token);
 EXPORT_SYMBOL(rtas_call);
 EXPORT_SYMBOL(rtas_data_buf);
 EXPORT_SYMBOL(rtas_data_buf_lock);
-EXPORT_SYMBOL(rtas_extended_busy_delay_time);
+EXPORT_SYMBOL(rtas_busy_delay_time);
 EXPORT_SYMBOL(rtas_get_sensor);
 EXPORT_SYMBOL(rtas_get_power_level);
 EXPORT_SYMBOL(rtas_set_power_level);
diff -puN arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg arch/powerpc/kernel/rtas-rtc.c
--- 2_6_linus/arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c	2006-06-02 15:09:43.000000000 -0500
@@ -14,19 +14,20 @@
 unsigned long __init rtas_get_boot_time(void)
 {
 	int ret[8];
-	int error, wait_time;
+	int error;
+	unsigned int wait_time;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
 	do {
 		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
-			wait_time = rtas_extended_busy_delay_time(error);
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			/* This is boot time so we spin. */
 			udelay(wait_time*1000);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
 	if (error != 0 && printk_ratelimit()) {
 		printk(KERN_WARNING "error: reading the clock failed (%d)\n",
@@ -44,24 +45,25 @@ unsigned long __init rtas_get_boot_time(
 void rtas_get_rtc_time(struct rtc_time *rtc_tm)
 {
         int ret[8];
-	int error, wait_time;
+	int error;
+	unsigned int wait_time;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
 	do {
 		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			if (in_interrupt() && printk_ratelimit()) {
 				memset(rtc_tm, 0, sizeof(struct rtc_time));
 				printk(KERN_WARNING "error: reading clock"
 				       " would delay interrupt\n");
 				return;	/* delay not allowed */
 			}
-			wait_time = rtas_extended_busy_delay_time(error);
 			msleep(wait_time);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
         if (error != 0 && printk_ratelimit()) {
                 printk(KERN_WARNING "error: reading the clock failed (%d)\n",
@@ -88,14 +90,14 @@ int rtas_set_rtc_time(struct rtc_time *t
 				  tm->tm_year + 1900, tm->tm_mon + 1,
 				  tm->tm_mday, tm->tm_hour, tm->tm_min,
 				  tm->tm_sec, 0);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			if (in_interrupt())
 				return 1;	/* probably decrementer */
-			wait_time = rtas_extended_busy_delay_time(error);
 			msleep(wait_time);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
         if (error != 0 && printk_ratelimit())
                 printk(KERN_WARNING "error: setting the clock failed (%d)\n",
diff -puN include/asm-powerpc/rtas.h~rtas_delay_reorg include/asm-powerpc/rtas.h
--- 2_6_linus/include/asm-powerpc/rtas.h~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/include/asm-powerpc/rtas.h	2006-06-02 15:09:43.000000000 -0500
@@ -177,12 +177,8 @@ extern unsigned long rtas_get_boot_time(
 extern void rtas_get_rtc_time(struct rtc_time *rtc_time);
 extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
 
-/* Given an RTAS status code of 9900..9905 compute the hinted delay */
-unsigned int rtas_extended_busy_delay_time(int status);
-static inline int rtas_is_extended_busy(int status)
-{
-	return status >= 9900 && status <= 9909;
-}
+extern unsigned int rtas_busy_delay_time(int status);
+extern unsigned int rtas_busy_delay(int status);
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
 
diff -puN arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg arch/powerpc/kernel/rtas_flash.c
--- 2_6_linus/arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-06-02 15:09:43.000000000 -0500
@@ -365,20 +365,12 @@ static int rtas_excl_release(struct inod
 
 static void manage_flash(struct rtas_manage_flash_t *args_buf)
 {
-	unsigned int wait_time;
 	s32 rc;
 
-	while (1) {
+	do
 		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 
 			       1, NULL, args_buf->op);
-		if (rc == RTAS_RC_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	while (rtas_busy_delay(rc));
 
 	args_buf->status = rc;
 }
@@ -451,27 +443,18 @@ static ssize_t manage_flash_write(struct
 static void validate_flash(struct rtas_validate_flash_t *args_buf)
 {
 	int token = rtas_token("ibm,validate-flash-image");
-	unsigned int wait_time;
 	int update_results;
 	s32 rc;	
 
 	rc = 0;
-	while(1) {
+	do {
 		spin_lock(&rtas_data_buf_lock);
 		memcpy(rtas_data_buf, args_buf->buf, VALIDATE_BUF_SIZE);
 		rc = rtas_call(token, 2, 2, &update_results, 
 			       (u32) __pa(rtas_data_buf), args_buf->buf_size);
 		memcpy(args_buf->buf, rtas_data_buf, VALIDATE_BUF_SIZE);
 		spin_unlock(&rtas_data_buf_lock);
-			
-		if (rc == RTAS_RC_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	args_buf->status = rc;
 	args_buf->update_results = update_results;

_

Re: [PATCH] reorg RTAS delay code

[Nathan Lynch]

> +/* For an RTAS busy status code, perform the hinted delay. */
> +unsigned int rtas_busy_delay(int status)
> +{
> +	unsigned int ms;
>  
> -	return ms; 
> +	ms = rtas_busy_delay_time(status);
> +	if (ms)
> +		msleep(ms);
> +
> +	return ms;
>  }

Can you put a might_sleep() at the beginning of this function so that
we can reliably catch unsafe uses of it?  Otherwise we'll get warnings
only when a delay is actually executed.


> @@ -438,22 +449,14 @@ int rtas_get_power_level(int powerdomain
>  int rtas_set_power_level(int powerdomain, int level, int *setlevel)
>  {
>  	int token = rtas_token("set-power-level");
> -	unsigned int wait_time;
>  	int rc;
>  
>  	if (token == RTAS_UNKNOWN_SERVICE)
>  		return -ENOENT;
>  
> -	while (1) {
> +	do
>  		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
> -		if (rc == RTAS_BUSY)
> -			udelay(1);
> -		else if (rtas_is_extended_busy(rc)) {
> -			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		} else
> -			break;
> -	}
> +	while (rtas_busy_delay(rc));

Coding style nit -- am I alone in thinking that do/while without the
brackets looks weird?  Given the single-statement body of the loop, I
guess the brackets aren't necessary, but omitting the brackets doesn't
save lines in this case.

[PATCH] reorg RTAS delay code

[John Rose]

This patch attempts to handle RTAS "busy" return codes in a more simple
and consistent manner.  Typical callers of RTAS shouldn't have to
manage wait times and delay calls.

This patch also changes the kernel to use msleep() rather than udelay()
when a runtime delay is necessary.  This will avoid CPU soft lockups
for extended delay conditions.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

Resend - added the suggested might_sleep() and braces.

Thanks-
John

 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c   |   30 +++----
 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c       |   85 ++++++++------------
 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c |   25 -----
 2_6_linus-johnrose/include/asm-powerpc/rtas.h       |    8 -
 4 files changed, 57 insertions(+), 91 deletions(-)

diff -puN arch/powerpc/kernel/rtas.c~rtas_delay_reorg arch/powerpc/kernel/rtas.c
--- 2_6_linus/arch/powerpc/kernel/rtas.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c	2006-06-05 15:00:03.000000000 -0500
@@ -370,24 +370,36 @@ int rtas_call(int token, int nargs, int 
 	return ret;
 }
 
-/* Given an RTAS status code of 990n compute the hinted delay of 10^n
- * (last digit) milliseconds.  For now we bound at n=5 (100 sec).
+/* For RTAS_BUSY (-2), delay for 1 millisecond.  For an extended busy status
+ * code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
  */
-unsigned int rtas_extended_busy_delay_time(int status)
+unsigned int rtas_busy_delay_time(int status)
 {
-	int order = status - 9900;
-	unsigned long ms;
+	int order;
+	unsigned int ms = 0;
 
-	if (order < 0)
-		order = 0;	/* RTC depends on this for -2 clock busy */
-	else if (order > 5)
-		order = 5;	/* bound */
+	if (status == RTAS_BUSY) {
+		ms = 1;
+	} else if (status >= 9900 && status <= 9905) {
+		order = status - 9900;
+		for (ms = 1; order > 0; order--)
+			ms *= 10;
+	}
+
+	return ms;
+}
+
+/* For an RTAS busy status code, perform the hinted delay. */
+unsigned int rtas_busy_delay(int status)
+{
+	unsigned int ms;
 
-	/* Use microseconds for reasonable accuracy */
-	for (ms = 1; order > 0; order--)
-		ms *= 10;
+	might_sleep();
+	ms = rtas_busy_delay_time(status);
+	if (ms)
+		msleep(ms);
 
-	return ms; 
+	return ms;
 }
 
 int rtas_error_rc(int rtas_rc)
@@ -438,22 +450,14 @@ int rtas_get_power_level(int powerdomain
 int rtas_set_power_level(int powerdomain, int level, int *setlevel)
 {
 	int token = rtas_token("set-power-level");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do {
 		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -463,22 +467,14 @@ int rtas_set_power_level(int powerdomain
 int rtas_get_sensor(int sensor, int index, int *state)
 {
 	int token = rtas_token("get-sensor-state");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do {
 		rc = rtas_call(token, 2, 2, state, sensor, index);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -488,23 +484,14 @@ int rtas_get_sensor(int sensor, int inde
 int rtas_set_indicator(int indicator, int index, int new_value)
 {
 	int token = rtas_token("set-indicator");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do {
 		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		}
-		else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -555,13 +542,11 @@ void rtas_os_term(char *str)
 	do {
 		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
 				   __pa(rtas_os_term_buf));
+	} while (rtas_busy_delay(status));
 
-		if (status == RTAS_BUSY)
-			udelay(1);
-		else if (status != 0)
-			printk(KERN_EMERG "ibm,os-term call failed %d\n",
+	if (status != 0)
+		printk(KERN_EMERG "ibm,os-term call failed %d\n",
 			       status);
-	} while (status == RTAS_BUSY);
 }
 
 static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
@@ -789,7 +774,7 @@ EXPORT_SYMBOL(rtas_token);
 EXPORT_SYMBOL(rtas_call);
 EXPORT_SYMBOL(rtas_data_buf);
 EXPORT_SYMBOL(rtas_data_buf_lock);
-EXPORT_SYMBOL(rtas_extended_busy_delay_time);
+EXPORT_SYMBOL(rtas_busy_delay_time);
 EXPORT_SYMBOL(rtas_get_sensor);
 EXPORT_SYMBOL(rtas_get_power_level);
 EXPORT_SYMBOL(rtas_set_power_level);
diff -puN arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg arch/powerpc/kernel/rtas-rtc.c
--- 2_6_linus/arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c	2006-06-02 15:09:43.000000000 -0500
@@ -14,19 +14,20 @@
 unsigned long __init rtas_get_boot_time(void)
 {
 	int ret[8];
-	int error, wait_time;
+	int error;
+	unsigned int wait_time;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
 	do {
 		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
-			wait_time = rtas_extended_busy_delay_time(error);
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			/* This is boot time so we spin. */
 			udelay(wait_time*1000);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
 	if (error != 0 && printk_ratelimit()) {
 		printk(KERN_WARNING "error: reading the clock failed (%d)\n",
@@ -44,24 +45,25 @@ unsigned long __init rtas_get_boot_time(
 void rtas_get_rtc_time(struct rtc_time *rtc_tm)
 {
         int ret[8];
-	int error, wait_time;
+	int error;
+	unsigned int wait_time;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
 	do {
 		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			if (in_interrupt() && printk_ratelimit()) {
 				memset(rtc_tm, 0, sizeof(struct rtc_time));
 				printk(KERN_WARNING "error: reading clock"
 				       " would delay interrupt\n");
 				return;	/* delay not allowed */
 			}
-			wait_time = rtas_extended_busy_delay_time(error);
 			msleep(wait_time);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
         if (error != 0 && printk_ratelimit()) {
                 printk(KERN_WARNING "error: reading the clock failed (%d)\n",
@@ -88,14 +90,14 @@ int rtas_set_rtc_time(struct rtc_time *t
 				  tm->tm_year + 1900, tm->tm_mon + 1,
 				  tm->tm_mday, tm->tm_hour, tm->tm_min,
 				  tm->tm_sec, 0);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			if (in_interrupt())
 				return 1;	/* probably decrementer */
-			wait_time = rtas_extended_busy_delay_time(error);
 			msleep(wait_time);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
         if (error != 0 && printk_ratelimit())
                 printk(KERN_WARNING "error: setting the clock failed (%d)\n",
diff -puN include/asm-powerpc/rtas.h~rtas_delay_reorg include/asm-powerpc/rtas.h
--- 2_6_linus/include/asm-powerpc/rtas.h~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/include/asm-powerpc/rtas.h	2006-06-02 15:09:43.000000000 -0500
@@ -177,12 +177,8 @@ extern unsigned long rtas_get_boot_time(
 extern void rtas_get_rtc_time(struct rtc_time *rtc_time);
 extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
 
-/* Given an RTAS status code of 9900..9905 compute the hinted delay */
-unsigned int rtas_extended_busy_delay_time(int status);
-static inline int rtas_is_extended_busy(int status)
-{
-	return status >= 9900 && status <= 9909;
-}
+extern unsigned int rtas_busy_delay_time(int status);
+extern unsigned int rtas_busy_delay(int status);
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
 
diff -puN arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg arch/powerpc/kernel/rtas_flash.c
--- 2_6_linus/arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-06-05 15:00:44.000000000 -0500
@@ -365,20 +365,12 @@ static int rtas_excl_release(struct inod
 
 static void manage_flash(struct rtas_manage_flash_t *args_buf)
 {
-	unsigned int wait_time;
 	s32 rc;
 
-	while (1) {
+	do {
 		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 
 			       1, NULL, args_buf->op);
-		if (rc == RTAS_RC_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	args_buf->status = rc;
 }
@@ -451,27 +443,18 @@ static ssize_t manage_flash_write(struct
 static void validate_flash(struct rtas_validate_flash_t *args_buf)
 {
 	int token = rtas_token("ibm,validate-flash-image");
-	unsigned int wait_time;
 	int update_results;
 	s32 rc;	
 
 	rc = 0;
-	while(1) {
+	do {
 		spin_lock(&rtas_data_buf_lock);
 		memcpy(rtas_data_buf, args_buf->buf, VALIDATE_BUF_SIZE);
 		rc = rtas_call(token, 2, 2, &update_results, 
 			       (u32) __pa(rtas_data_buf), args_buf->buf_size);
 		memcpy(args_buf->buf, rtas_data_buf, VALIDATE_BUF_SIZE);
 		spin_unlock(&rtas_data_buf_lock);
-			
-		if (rc == RTAS_RC_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	args_buf->status = rc;
 	args_buf->update_results = update_results;

_

Re: [PATCH] reorg RTAS delay code

[Nathan Lynch]

John Rose wrote:
> This patch attempts to handle RTAS "busy" return codes in a more simple
> and consistent manner.  Typical callers of RTAS shouldn't have to
> manage wait times and delay calls.
> 
> This patch also changes the kernel to use msleep() rather than udelay()
> when a runtime delay is necessary.  This will avoid CPU soft lockups
> for extended delay conditions.
> 
> Signed-off-by: John Rose <johnrose@austin.ibm.com>
> 
> ---
> 
> Resend - added the suggested might_sleep() and braces.

FWIW:

Acked-by: Nathan Lynch <ntl@pobox.com>

>  2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c   |   30 +++----
>  2_6_linus-johnrose/arch/powerpc/kernel/rtas.c       |   85 ++++++++------------
>  2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c |   25 -----
>  2_6_linus-johnrose/include/asm-powerpc/rtas.h       |    8 -
>  4 files changed, 57 insertions(+), 91 deletions(-)
> 
> diff -puN arch/powerpc/kernel/rtas.c~rtas_delay_reorg arch/powerpc/kernel/rtas.c
> --- 2_6_linus/arch/powerpc/kernel/rtas.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
> +++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c	2006-06-05 15:00:03.000000000 -0500
> @@ -370,24 +370,36 @@ int rtas_call(int token, int nargs, int 
>  	return ret;
>  }
>  
> -/* Given an RTAS status code of 990n compute the hinted delay of 10^n
> - * (last digit) milliseconds.  For now we bound at n=5 (100 sec).
> +/* For RTAS_BUSY (-2), delay for 1 millisecond.  For an extended busy status
> + * code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
>   */
> -unsigned int rtas_extended_busy_delay_time(int status)
> +unsigned int rtas_busy_delay_time(int status)
>  {
> -	int order = status - 9900;
> -	unsigned long ms;
> +	int order;
> +	unsigned int ms = 0;
>  
> -	if (order < 0)
> -		order = 0;	/* RTC depends on this for -2 clock busy */
> -	else if (order > 5)
> -		order = 5;	/* bound */
> +	if (status == RTAS_BUSY) {
> +		ms = 1;
> +	} else if (status >= 9900 && status <= 9905) {
> +		order = status - 9900;
> +		for (ms = 1; order > 0; order--)
> +			ms *= 10;
> +	}
> +
> +	return ms;
> +}
> +
> +/* For an RTAS busy status code, perform the hinted delay. */
> +unsigned int rtas_busy_delay(int status)
> +{
> +	unsigned int ms;
>  
> -	/* Use microseconds for reasonable accuracy */
> -	for (ms = 1; order > 0; order--)
> -		ms *= 10;
> +	might_sleep();
> +	ms = rtas_busy_delay_time(status);
> +	if (ms)
> +		msleep(ms);
>  
> -	return ms; 
> +	return ms;
>  }
>  
>  int rtas_error_rc(int rtas_rc)
> @@ -438,22 +450,14 @@ int rtas_get_power_level(int powerdomain
>  int rtas_set_power_level(int powerdomain, int level, int *setlevel)
>  {
>  	int token = rtas_token("set-power-level");
> -	unsigned int wait_time;
>  	int rc;
>  
>  	if (token == RTAS_UNKNOWN_SERVICE)
>  		return -ENOENT;
>  
> -	while (1) {
> +	do {
>  		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
> -		if (rc == RTAS_BUSY)
> -			udelay(1);
> -		else if (rtas_is_extended_busy(rc)) {
> -			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		} else
> -			break;
> -	}
> +	} while (rtas_busy_delay(rc));
>  
>  	if (rc < 0)
>  		return rtas_error_rc(rc);
> @@ -463,22 +467,14 @@ int rtas_set_power_level(int powerdomain
>  int rtas_get_sensor(int sensor, int index, int *state)
>  {
>  	int token = rtas_token("get-sensor-state");
> -	unsigned int wait_time;
>  	int rc;
>  
>  	if (token == RTAS_UNKNOWN_SERVICE)
>  		return -ENOENT;
>  
> -	while (1) {
> +	do {
>  		rc = rtas_call(token, 2, 2, state, sensor, index);
> -		if (rc == RTAS_BUSY)
> -			udelay(1);
> -		else if (rtas_is_extended_busy(rc)) {
> -			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		} else
> -			break;
> -	}
> +	} while (rtas_busy_delay(rc));
>  
>  	if (rc < 0)
>  		return rtas_error_rc(rc);
> @@ -488,23 +484,14 @@ int rtas_get_sensor(int sensor, int inde
>  int rtas_set_indicator(int indicator, int index, int new_value)
>  {
>  	int token = rtas_token("set-indicator");
> -	unsigned int wait_time;
>  	int rc;
>  
>  	if (token == RTAS_UNKNOWN_SERVICE)
>  		return -ENOENT;
>  
> -	while (1) {
> +	do {
>  		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
> -		if (rc == RTAS_BUSY)
> -			udelay(1);
> -		else if (rtas_is_extended_busy(rc)) {
> -			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		}
> -		else
> -			break;
> -	}
> +	} while (rtas_busy_delay(rc));
>  
>  	if (rc < 0)
>  		return rtas_error_rc(rc);
> @@ -555,13 +542,11 @@ void rtas_os_term(char *str)
>  	do {
>  		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
>  				   __pa(rtas_os_term_buf));
> +	} while (rtas_busy_delay(status));
>  
> -		if (status == RTAS_BUSY)
> -			udelay(1);
> -		else if (status != 0)
> -			printk(KERN_EMERG "ibm,os-term call failed %d\n",
> +	if (status != 0)
> +		printk(KERN_EMERG "ibm,os-term call failed %d\n",
>  			       status);
> -	} while (status == RTAS_BUSY);
>  }
>  
>  static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
> @@ -789,7 +774,7 @@ EXPORT_SYMBOL(rtas_token);
>  EXPORT_SYMBOL(rtas_call);
>  EXPORT_SYMBOL(rtas_data_buf);
>  EXPORT_SYMBOL(rtas_data_buf_lock);
> -EXPORT_SYMBOL(rtas_extended_busy_delay_time);
> +EXPORT_SYMBOL(rtas_busy_delay_time);
>  EXPORT_SYMBOL(rtas_get_sensor);
>  EXPORT_SYMBOL(rtas_get_power_level);
>  EXPORT_SYMBOL(rtas_set_power_level);
> diff -puN arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg arch/powerpc/kernel/rtas-rtc.c
> --- 2_6_linus/arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
> +++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c	2006-06-02 15:09:43.000000000 -0500
> @@ -14,19 +14,20 @@
>  unsigned long __init rtas_get_boot_time(void)
>  {
>  	int ret[8];
> -	int error, wait_time;
> +	int error;
> +	unsigned int wait_time;
>  	u64 max_wait_tb;
>  
>  	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
>  	do {
>  		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
> -		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
> -			wait_time = rtas_extended_busy_delay_time(error);
> +
> +		wait_time = rtas_busy_delay_time(error);
> +		if (wait_time) {
>  			/* This is boot time so we spin. */
>  			udelay(wait_time*1000);
> -			error = RTAS_CLOCK_BUSY;
>  		}
> -	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
> +	} while (wait_time && (get_tb() < max_wait_tb));
>  
>  	if (error != 0 && printk_ratelimit()) {
>  		printk(KERN_WARNING "error: reading the clock failed (%d)\n",
> @@ -44,24 +45,25 @@ unsigned long __init rtas_get_boot_time(
>  void rtas_get_rtc_time(struct rtc_time *rtc_tm)
>  {
>          int ret[8];
> -	int error, wait_time;
> +	int error;
> +	unsigned int wait_time;
>  	u64 max_wait_tb;
>  
>  	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
>  	do {
>  		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
> -		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
> +
> +		wait_time = rtas_busy_delay_time(error);
> +		if (wait_time) {
>  			if (in_interrupt() && printk_ratelimit()) {











>  				memset(rtc_tm, 0, sizeof(struct rtc_time));
>  				printk(KERN_WARNING "error: reading clock"
>  				       " would delay interrupt\n");
>  				return;	/* delay not allowed */
>  			}
> -			wait_time = rtas_extended_busy_delay_time(error);
>  			msleep(wait_time);
> -			error = RTAS_CLOCK_BUSY;
>  		}
> -	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
> +	} while (wait_time && (get_tb() < max_wait_tb));
>  
>          if (error != 0 && printk_ratelimit()) {
>                  printk(KERN_WARNING "error: reading the clock failed (%d)\n",
> @@ -88,14 +90,14 @@ int rtas_set_rtc_time(struct rtc_time *t
>  				  tm->tm_year + 1900, tm->tm_mon + 1,
>  				  tm->tm_mday, tm->tm_hour, tm->tm_min,
>  				  tm->tm_sec, 0);
> -		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
> +
> +		wait_time = rtas_busy_delay_time(error);
> +		if (wait_time) {
>  			if (in_interrupt())
>  				return 1;	/* probably decrementer */
> -			wait_time = rtas_extended_busy_delay_time(error);
>  			msleep(wait_time);
> -			error = RTAS_CLOCK_BUSY;
>  		}
> -	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
> +	} while (wait_time && (get_tb() < max_wait_tb));
>  
>          if (error != 0 && printk_ratelimit())
>                  printk(KERN_WARNING "error: setting the clock failed (%d)\n",
> diff -puN include/asm-powerpc/rtas.h~rtas_delay_reorg include/asm-powerpc/rtas.h
> --- 2_6_linus/include/asm-powerpc/rtas.h~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
> +++ 2_6_linus-johnrose/include/asm-powerpc/rtas.h	2006-06-02 15:09:43.000000000 -0500
> @@ -177,12 +177,8 @@ extern unsigned long rtas_get_boot_time(
>  extern void rtas_get_rtc_time(struct rtc_time *rtc_time);
>  extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
>  
> -/* Given an RTAS status code of 9900..9905 compute the hinted delay */
> -unsigned int rtas_extended_busy_delay_time(int status);
> -static inline int rtas_is_extended_busy(int status)
> -{
> -	return status >= 9900 && status <= 9909;
> -}
> +extern unsigned int rtas_busy_delay_time(int status);
> +extern unsigned int rtas_busy_delay(int status);
>  
>  extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
>  
> diff -puN arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg arch/powerpc/kernel/rtas_flash.c
> --- 2_6_linus/arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg	2006-06-02 15:09:43.000000000 -0500
> +++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-06-05 15:00:44.000000000 -0500
> @@ -365,20 +365,12 @@ static int rtas_excl_release(struct inod
>  
>  static void manage_flash(struct rtas_manage_flash_t *args_buf)
>  {
> -	unsigned int wait_time;
>  	s32 rc;
>  
> -	while (1) {
> +	do {
>  		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 
>  			       1, NULL, args_buf->op);
> -		if (rc == RTAS_RC_BUSY)
> -			udelay(1);
> -		else if (rtas_is_extended_busy(rc)) {
> -			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		} else
> -			break;
> -	}
> +	} while (rtas_busy_delay(rc));
>  
>  	args_buf->status = rc;
>  }
> @@ -451,27 +443,18 @@ static ssize_t manage_flash_write(struct
>  static void validate_flash(struct rtas_validate_flash_t *args_buf)
>  {
>  	int token = rtas_token("ibm,validate-flash-image");
> -	unsigned int wait_time;
>  	int update_results;
>  	s32 rc;	
>  
>  	rc = 0;
> -	while(1) {
> +	do {
>  		spin_lock(&rtas_data_buf_lock);
>  		memcpy(rtas_data_buf, args_buf->buf, VALIDATE_BUF_SIZE);
>  		rc = rtas_call(token, 2, 2, &update_results, 
>  			       (u32) __pa(rtas_data_buf), args_buf->buf_size);
>  		memcpy(args_buf->buf, rtas_data_buf, VALIDATE_BUF_SIZE);
>  		spin_unlock(&rtas_data_buf_lock);
> -			
> -		if (rc == RTAS_RC_BUSY)
> -			udelay(1);
> -		else if (rtas_is_extended_busy(rc)) {
> -			wait_time = rtas_extended_busy_delay_time(rc);
> -			udelay(wait_time * 1000);
> -		} else
> -			break;
> -	}
> +	} while (rtas_busy_delay(rc));
>  
>  	args_buf->status = rc;
>  	args_buf->update_results = update_results;
> 
> _
> 
> 

Re: [PATCH] reorg RTAS delay code

[Anton Blanchard]

Hi John,

> This patch attempts to handle RTAS "busy" return codes in a more simple
> and consistent manner.  Typical callers of RTAS shouldn't have to
> manage wait times and delay calls.
> 
> This patch also changes the kernel to use msleep() rather than udelay()
> when a runtime delay is necessary.  This will avoid CPU soft lockups
> for extended delay conditions.

Looks like you missed one:

WARNING: ".rtas_extended_busy_delay_time" [arch/powerpc/platforms/pseries/scanlog.ko] undefined!

Anton

Re: [PATCH] reorg RTAS delay code

[Anton Blanchard]

 
> Looks like you missed one:
> 
> WARNING: ".rtas_extended_busy_delay_time" [arch/powerpc/platforms/pseries/scanlog.ko] undefined!

And a missing export:

WARNING: ".rtas_busy_delay" [arch/powerpc/kernel/rtas_flash.ko] undefined!

ANton

Re: [PATCH] reorg RTAS delay code

[John Rose]

This patch attempts to handle RTAS "busy" return codes in a more simple
and consistent manner.  Typical callers of RTAS shouldn't have to
manage wait times and delay calls.

This patch also changes the kernel to use msleep() rather than udelay()
when a runtime delay is necessary.  This will avoid CPU soft lockups
for extended delay conditions.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

Resend - Addressed module build breaks.  Thanks Anton.

Thanks-
John

---

 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c           |   30 ++--
 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c               |   86 +++++-------
 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c         |   25 ---
 2_6_linus-johnrose/arch/powerpc/platforms/pseries/scanlog.c |    6 
 2_6_linus-johnrose/include/asm-powerpc/rtas.h               |    8 -
 5 files changed, 61 insertions(+), 94 deletions(-)

diff -puN arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg arch/powerpc/kernel/rtas-rtc.c
--- 2_6_linus/arch/powerpc/kernel/rtas-rtc.c~rtas_delay_reorg	2006-06-12 11:21:29.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas-rtc.c	2006-06-12 11:21:29.000000000 -0500
@@ -14,19 +14,20 @@
 unsigned long __init rtas_get_boot_time(void)
 {
 	int ret[8];
-	int error, wait_time;
+	int error;
+	unsigned int wait_time;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
 	do {
 		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
-			wait_time = rtas_extended_busy_delay_time(error);
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			/* This is boot time so we spin. */
 			udelay(wait_time*1000);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
 	if (error != 0 && printk_ratelimit()) {
 		printk(KERN_WARNING "error: reading the clock failed (%d)\n",
@@ -44,24 +45,25 @@ unsigned long __init rtas_get_boot_time(
 void rtas_get_rtc_time(struct rtc_time *rtc_tm)
 {
         int ret[8];
-	int error, wait_time;
+	int error;
+	unsigned int wait_time;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
 	do {
 		error = rtas_call(rtas_token("get-time-of-day"), 0, 8, ret);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			if (in_interrupt() && printk_ratelimit()) {
 				memset(rtc_tm, 0, sizeof(struct rtc_time));
 				printk(KERN_WARNING "error: reading clock"
 				       " would delay interrupt\n");
 				return;	/* delay not allowed */
 			}
-			wait_time = rtas_extended_busy_delay_time(error);
 			msleep(wait_time);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
         if (error != 0 && printk_ratelimit()) {
                 printk(KERN_WARNING "error: reading the clock failed (%d)\n",
@@ -88,14 +90,14 @@ int rtas_set_rtc_time(struct rtc_time *t
 				  tm->tm_year + 1900, tm->tm_mon + 1,
 				  tm->tm_mday, tm->tm_hour, tm->tm_min,
 				  tm->tm_sec, 0);
-		if (error == RTAS_CLOCK_BUSY || rtas_is_extended_busy(error)) {
+
+		wait_time = rtas_busy_delay_time(error);
+		if (wait_time) {
 			if (in_interrupt())
 				return 1;	/* probably decrementer */
-			wait_time = rtas_extended_busy_delay_time(error);
 			msleep(wait_time);
-			error = RTAS_CLOCK_BUSY;
 		}
-	} while (error == RTAS_CLOCK_BUSY && (get_tb() < max_wait_tb));
+	} while (wait_time && (get_tb() < max_wait_tb));
 
         if (error != 0 && printk_ratelimit())
                 printk(KERN_WARNING "error: setting the clock failed (%d)\n",
diff -puN arch/powerpc/kernel/rtas.c~rtas_delay_reorg arch/powerpc/kernel/rtas.c
--- 2_6_linus/arch/powerpc/kernel/rtas.c~rtas_delay_reorg	2006-06-12 11:21:29.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas.c	2006-06-12 11:21:29.000000000 -0500
@@ -370,24 +370,36 @@ int rtas_call(int token, int nargs, int 
 	return ret;
 }
 
-/* Given an RTAS status code of 990n compute the hinted delay of 10^n
- * (last digit) milliseconds.  For now we bound at n=5 (100 sec).
+/* For RTAS_BUSY (-2), delay for 1 millisecond.  For an extended busy status
+ * code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
  */
-unsigned int rtas_extended_busy_delay_time(int status)
+unsigned int rtas_busy_delay_time(int status)
 {
-	int order = status - 9900;
-	unsigned long ms;
+	int order;
+	unsigned int ms = 0;
 
-	if (order < 0)
-		order = 0;	/* RTC depends on this for -2 clock busy */
-	else if (order > 5)
-		order = 5;	/* bound */
+	if (status == RTAS_BUSY) {
+		ms = 1;
+	} else if (status >= 9900 && status <= 9905) {
+		order = status - 9900;
+		for (ms = 1; order > 0; order--)
+			ms *= 10;
+	}
+
+	return ms;
+}
+
+/* For an RTAS busy status code, perform the hinted delay. */
+unsigned int rtas_busy_delay(int status)
+{
+	unsigned int ms;
 
-	/* Use microseconds for reasonable accuracy */
-	for (ms = 1; order > 0; order--)
-		ms *= 10;
+	might_sleep();
+	ms = rtas_busy_delay_time(status);
+	if (ms)
+		msleep(ms);
 
-	return ms; 
+	return ms;
 }
 
 int rtas_error_rc(int rtas_rc)
@@ -438,22 +450,14 @@ int rtas_get_power_level(int powerdomain
 int rtas_set_power_level(int powerdomain, int level, int *setlevel)
 {
 	int token = rtas_token("set-power-level");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do {
 		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -463,22 +467,14 @@ int rtas_set_power_level(int powerdomain
 int rtas_get_sensor(int sensor, int index, int *state)
 {
 	int token = rtas_token("get-sensor-state");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do {
 		rc = rtas_call(token, 2, 2, state, sensor, index);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -488,23 +484,14 @@ int rtas_get_sensor(int sensor, int inde
 int rtas_set_indicator(int indicator, int index, int new_value)
 {
 	int token = rtas_token("set-indicator");
-	unsigned int wait_time;
 	int rc;
 
 	if (token == RTAS_UNKNOWN_SERVICE)
 		return -ENOENT;
 
-	while (1) {
+	do {
 		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
-		if (rc == RTAS_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		}
-		else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -555,13 +542,11 @@ void rtas_os_term(char *str)
 	do {
 		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
 				   __pa(rtas_os_term_buf));
+	} while (rtas_busy_delay(status));
 
-		if (status == RTAS_BUSY)
-			udelay(1);
-		else if (status != 0)
-			printk(KERN_EMERG "ibm,os-term call failed %d\n",
+	if (status != 0)
+		printk(KERN_EMERG "ibm,os-term call failed %d\n",
 			       status);
-	} while (status == RTAS_BUSY);
 }
 
 static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
@@ -789,7 +774,8 @@ EXPORT_SYMBOL(rtas_token);
 EXPORT_SYMBOL(rtas_call);
 EXPORT_SYMBOL(rtas_data_buf);
 EXPORT_SYMBOL(rtas_data_buf_lock);
-EXPORT_SYMBOL(rtas_extended_busy_delay_time);
+EXPORT_SYMBOL(rtas_busy_delay);
+EXPORT_SYMBOL(rtas_busy_delay_time);
 EXPORT_SYMBOL(rtas_get_sensor);
 EXPORT_SYMBOL(rtas_get_power_level);
 EXPORT_SYMBOL(rtas_set_power_level);
diff -puN arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg arch/powerpc/kernel/rtas_flash.c
--- 2_6_linus/arch/powerpc/kernel/rtas_flash.c~rtas_delay_reorg	2006-06-12 11:21:29.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-06-12 11:21:29.000000000 -0500
@@ -365,20 +365,12 @@ static int rtas_excl_release(struct inod
 
 static void manage_flash(struct rtas_manage_flash_t *args_buf)
 {
-	unsigned int wait_time;
 	s32 rc;
 
-	while (1) {
+	do {
 		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 
 			       1, NULL, args_buf->op);
-		if (rc == RTAS_RC_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	args_buf->status = rc;
 }
@@ -451,27 +443,18 @@ static ssize_t manage_flash_write(struct
 static void validate_flash(struct rtas_validate_flash_t *args_buf)
 {
 	int token = rtas_token("ibm,validate-flash-image");
-	unsigned int wait_time;
 	int update_results;
 	s32 rc;	
 
 	rc = 0;
-	while(1) {
+	do {
 		spin_lock(&rtas_data_buf_lock);
 		memcpy(rtas_data_buf, args_buf->buf, VALIDATE_BUF_SIZE);
 		rc = rtas_call(token, 2, 2, &update_results, 
 			       (u32) __pa(rtas_data_buf), args_buf->buf_size);
 		memcpy(args_buf->buf, rtas_data_buf, VALIDATE_BUF_SIZE);
 		spin_unlock(&rtas_data_buf_lock);
-			
-		if (rc == RTAS_RC_BUSY)
-			udelay(1);
-		else if (rtas_is_extended_busy(rc)) {
-			wait_time = rtas_extended_busy_delay_time(rc);
-			udelay(wait_time * 1000);
-		} else
-			break;
-	}
+	} while (rtas_busy_delay(rc));
 
 	args_buf->status = rc;
 	args_buf->update_results = update_results;
diff -puN include/asm-powerpc/rtas.h~rtas_delay_reorg include/asm-powerpc/rtas.h
--- 2_6_linus/include/asm-powerpc/rtas.h~rtas_delay_reorg	2006-06-12 11:21:29.000000000 -0500
+++ 2_6_linus-johnrose/include/asm-powerpc/rtas.h	2006-06-12 11:21:29.000000000 -0500
@@ -177,12 +177,8 @@ extern unsigned long rtas_get_boot_time(
 extern void rtas_get_rtc_time(struct rtc_time *rtc_time);
 extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
 
-/* Given an RTAS status code of 9900..9905 compute the hinted delay */
-unsigned int rtas_extended_busy_delay_time(int status);
-static inline int rtas_is_extended_busy(int status)
-{
-	return status >= 9900 && status <= 9909;
-}
+extern unsigned int rtas_busy_delay_time(int status);
+extern unsigned int rtas_busy_delay(int status);
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
 
diff -puN arch/powerpc/platforms/pseries/scanlog.c~rtas_delay_reorg arch/powerpc/platforms/pseries/scanlog.c
--- 2_6_linus/arch/powerpc/platforms/pseries/scanlog.c~rtas_delay_reorg	2006-06-12 11:21:29.000000000 -0500
+++ 2_6_linus-johnrose/arch/powerpc/platforms/pseries/scanlog.c	2006-06-12 11:21:29.000000000 -0500
@@ -107,9 +107,9 @@ static ssize_t scanlog_read(struct file 
 			/* Break to sleep default time */
 			break;
 		    default:
-			if (status > 9900 && status <= 9905) {
-				wait_time = rtas_extended_busy_delay_time(status);
-			} else {
+			/* Assume extended busy */
+			wait_time = rtas_busy_delay_time(status);
+			if (!wait_time) {
 				printk(KERN_ERR "scanlog: unknown error from rtas: %d\n", status);
 				return -EIO;
 			}

_

[PATCH] powerpc: RTAS delay, fix module build breaks

[John Rose]

Export both news RTAS delay functions, and change the scanlog module to
use the new delay functions.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

Respun against the powerpc git tree.  Thanks Paul.

 2_6_ppc-johnrose/arch/powerpc/kernel/rtas.c               |    1 +
 2_6_ppc-johnrose/arch/powerpc/platforms/pseries/scanlog.c |    6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff -puN arch/powerpc/kernel/rtas.c~finish_delay_reorg arch/powerpc/kernel/rtas.c
--- 2_6_ppc/arch/powerpc/kernel/rtas.c~finish_delay_reorg	2006-06-15 17:27:20.000000000 -0500
+++ 2_6_ppc-johnrose/arch/powerpc/kernel/rtas.c	2006-06-15 17:28:11.000000000 -0500
@@ -797,6 +797,7 @@ EXPORT_SYMBOL(rtas_call);
 EXPORT_SYMBOL(rtas_data_buf);
 EXPORT_SYMBOL(rtas_data_buf_lock);
 EXPORT_SYMBOL(rtas_busy_delay_time);
+EXPORT_SYMBOL(rtas_busy_delay);
 EXPORT_SYMBOL(rtas_get_sensor);
 EXPORT_SYMBOL(rtas_get_power_level);
 EXPORT_SYMBOL(rtas_set_power_level);
diff -puN arch/powerpc/platforms/pseries/scanlog.c~finish_delay_reorg arch/powerpc/platforms/pseries/scanlog.c
--- 2_6_ppc/arch/powerpc/platforms/pseries/scanlog.c~finish_delay_reorg	2006-06-15 17:28:30.000000000 -0500
+++ 2_6_ppc-johnrose/arch/powerpc/platforms/pseries/scanlog.c	2006-06-15 17:29:30.000000000 -0500
@@ -107,9 +107,9 @@ static ssize_t scanlog_read(struct file 
 			/* Break to sleep default time */
 			break;
 		    default:
-			if (status > 9900 && status <= 9905) {
-				wait_time = rtas_extended_busy_delay_time(status);
-			} else {
+			/* Assume extended busy */
+			wait_time = rtas_busy_delay_time(status);
+			if (!wait_time) {
 				printk(KERN_ERR "scanlog: unknown error from rtas: %d\n", status);
 				return -EIO;
 			}

_

Re: [PATCH] reorg RTAS delay code

[Nathan Lynch]

Hi folks-

John Rose wrote:
> This patch attempts to handle RTAS "busy" return codes in a more simple
> and consistent manner.  Typical callers of RTAS shouldn't have to
> manage wait times and delay calls.
> 
> This patch also changes the kernel to use msleep() rather than udelay()
> when a runtime delay is necessary.  This will avoid CPU soft lockups
> for extended delay conditions.

...

> +/* For an RTAS busy status code, perform the hinted delay. */
> +unsigned int rtas_busy_delay(int status)
> +{
> +	unsigned int ms;
>  
> -	/* Use microseconds for reasonable accuracy */
> -	for (ms = 1; order > 0; order--)
> -		ms *= 10;
> +	might_sleep();
> +	ms = rtas_busy_delay_time(status);
> +	if (ms)
> +		msleep(ms);
>  
> -	return ms; 
> +	return ms;
>  }

So I managed to test this with cpu hotplug recently and the
might_sleep warning triggers in the cpu offline path (I had to
reconstruct this from xmon due to the kernel crashing later on for a
different reason, so it might be a little off):

BUG: sleeping function called from invalid context at arch/powerpc/kernel/rtas.c:463.
in_atomic():1, irqs_disabled():1.
Call Trace:
[C0000000AAD379A0] [C000000000010ADC] .show_stack+0x68/0x1b4 (unreliable)
[C0000000AAD37A50] [C000000000050C98] .__might_sleep+0xd0/0xec
[C0000000AAD37AE0] [C00000000001DF5C] .rtas_busy_delay+0x20/0x54
[C0000000AAD37B70] [C00000000001E2D0] .rtas_set_indicator+0x70/0xd4
[C0000000AAD37C10] [C0000000000491C8] .xics_migrate_irqs_away+0x78/0x228
[C0000000AAD37CD0] [C000000000047C14] .pSeries_cpu_disable+0x98/0xb4
[C0000000AAD37D50] [C000000000029A5C] .__cpu_disable+0x4c/0x60
[C0000000AAD37DC0] [C000000000080834] .take_cpu_down+0x10/0x38
[C0000000AAD37E40] [C00000000008D1C0] .do_stop+0x198/0x248
[C0000000AAD37EE0] [C000000000078124] .kthread+0x124/0x174
[C0000000AAD37F90] [C000000000026568] .kernel_thread+0x4c/0x68


As it turns out, set-indicator is not allowed to return a busy delay
in this context, so we're actually safe here.  Maybe we need an
rtas_set_indicator_fast which could take that into account, e.g.

int rtas_set_indicator_fast(int indicator, int index, int new_value)
{
	int token = rtas_token("set-indicator");
	int rc;

	rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);

	WARN_ON(rc == -2 || rc >= 9000 || rc <= 9005);

	if (rc < 0)
		return rtas_error_rc(rc);
	return rc;
}

Re: [PATCH] reorg RTAS delay code

[Haren Myneni]

Nathan Lynch wrote:

>Hi folks-
>
>John Rose wrote:
>  
>
>>This patch attempts to handle RTAS "busy" return codes in a more simple
>>and consistent manner.  Typical callers of RTAS shouldn't have to
>>manage wait times and delay calls.
>>
>>This patch also changes the kernel to use msleep() rather than udelay()
>>when a runtime delay is necessary.  This will avoid CPU soft lockups
>>for extended delay conditions.
>>    
>>
>
>...
>
>  
>
>>+/* For an RTAS busy status code, perform the hinted delay. */
>>+unsigned int rtas_busy_delay(int status)
>>+{
>>+	unsigned int ms;
>> 
>>-	/* Use microseconds for reasonable accuracy */
>>-	for (ms = 1; order > 0; order--)
>>-		ms *= 10;
>>+	might_sleep();
>>+	ms = rtas_busy_delay_time(status);
>>+	if (ms)
>>+		msleep(ms);
>> 
>>-	return ms; 
>>+	return ms;
>> }
>>    
>>
>
>So I managed to test this with cpu hotplug recently and the
>might_sleep warning triggers in the cpu offline path (I had to
>reconstruct this from xmon due to the kernel crashing later on for a
>different reason, so it might be a little off):
>
>BUG: sleeping function called from invalid context at arch/powerpc/kernel/rtas.c:463.
>in_atomic():1, irqs_disabled():1.
>Call Trace:
>[C0000000AAD379A0] [C000000000010ADC] .show_stack+0x68/0x1b4 (unreliable)
>[C0000000AAD37A50] [C000000000050C98] .__might_sleep+0xd0/0xec
>[C0000000AAD37AE0] [C00000000001DF5C] .rtas_busy_delay+0x20/0x54
>[C0000000AAD37B70] [C00000000001E2D0] .rtas_set_indicator+0x70/0xd4
>[C0000000AAD37C10] [C0000000000491C8] .xics_migrate_irqs_away+0x78/0x228
>[C0000000AAD37CD0] [C000000000047C14] .pSeries_cpu_disable+0x98/0xb4
>[C0000000AAD37D50] [C000000000029A5C] .__cpu_disable+0x4c/0x60
>[C0000000AAD37DC0] [C000000000080834] .take_cpu_down+0x10/0x38
>[C0000000AAD37E40] [C00000000008D1C0] .do_stop+0x198/0x248
>[C0000000AAD37EE0] [C000000000078124] .kthread+0x124/0x174
>[C0000000AAD37F90] [C000000000026568] .kernel_thread+0x4c/0x68
>
>
>As it turns out, set-indicator is not allowed to return a busy delay
>in this context, so we're actually safe here.  Maybe we need an
>rtas_set_indicator_fast which could take that into account, e.g.
>
>int rtas_set_indicator_fast(int indicator, int index, int new_value)
>{
>	int token = rtas_token("set-indicator");
>	int rc;
>
>	rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
>
>	WARN_ON(rc == -2 || rc >= 9000 || rc <= 9005);
>
>	if (rc < 0)
>		return rtas_error_rc(rc);
>	return rc;
>}
>
>
>  
>
Hi, I am also noticing the similar stack trace from __might_sleep() for 
kdump boot. Before attempts kexec boot,  kdump code calls 
rtas_set_indicator() from xics_teardown_cpu().  Also, might_sleep() 
calls might_resched() -> cond_resched().  What about when the preemption 
is enabled? will the CPU get scheduled on another task?

 Can we have separate function rtas_set_indicator_fast() as mentioned 
above or

int rtas_set_indicator(int indicator, int index, int new_value, int 
sleep_on_delay)
{
        int token = rtas_token("set-indicator");
        int rc;

        if (token == RTAS_UNKNOWN_SERVICE)
                return -ENOENT;

        rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);

        if (!sleep_on_delay)
                WARN_ON(rc == -2 || rc >= 9000 || rc <= 9005);
        else
                while (rtas_busy_delay(rc))
                        rc = rtas_call(token, 3, 1, NULL, indicator, index,
                                new_value);

        if (rc < 0)
                return rtas_error_rc(rc);
        return rc;
}

Thanks
Haren

>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@ozlabs.org
>https://ozlabs.org/mailman/listinfo/linuxppc-dev
>  
>