[RFC 00/10] powerpc/rtas: improved busy and extended delay status handling

[RFC 00/10] powerpc/rtas: improved busy and extended delay status handling

[Nathan Lynch]

This is an attempt at providing clearer names as discussed here:

https://github.com/linuxppc/issues/issues/164

as well as providing better behavior for RTAS_BUSY (-2) and small
extended delay values, which in my experience seem more common than
the larger ones. In testing PREEMPT_NONE kernels with CPUs busy, I see
the elapsed time for memory add operations roughly halved, while
memory remove operations' elapsed time shrinks by about ~25%. This is
achieved without significantly more time spent on CPU:

(- is before, + is after)

  Performance counter stats for 'drmgr -c mem -a -q 10' (10 runs):

-             1,898      probe:rtas_call           #    0.003 M/sec                    ( +-  2.20% )
-            751.57 msec task-clock                #    0.289 CPUs utilized            ( +-  1.56% )
+             1,969      probe:rtas_call           #    0.003 M/sec                    ( +-  2.69% )
+            766.20 msec task-clock                #    0.688 CPUs utilized            ( +-  1.99% )

-             2.605 +- 0.148 seconds time elapsed  ( +-  5.70% )
+            1.1129 +- 0.0660 seconds time elapsed  ( +-  5.93% )

  Performance counter stats for 'drmgr -c mem -r -q 10' (10 runs):

-               673      probe:rtas_call           #    0.002 M/sec                    ( +-  0.55% )
-            318.36 msec task-clock                #    0.234 CPUs utilized            ( +-  0.42% )
+               692      probe:rtas_call           #    0.002 M/sec                    ( +-  0.73% )
+            320.87 msec task-clock                #    0.309 CPUs utilized            ( +-  0.34% )

-             1.362 +- 0.100 seconds time elapsed  ( +-  7.37% )
+            1.0372 +- 0.0468 seconds time elapsed  ( +-  4.51% )

Questions / concerns / to do:
* I don't love the new API function names.
* Introduces three new APIs when two likely would suffice.
* Need to convert eeh_pseries and scanlog.
* rtas_busy_delay() and rtas_busy_delay_time() not yet removed.

Nathan Lynch (10):
  powerpc/rtas: new APIs for busy and extended delay statuses
  powerpc/rtas: do not schedule in rtas_os_term()
  powerpc/rtas-rtc: convert get-time-of-day to rtas_force_spin_if_busy()
  powerpc/rtas-rtc: convert set-time-of-day to rtas_sched_if_busy()
  powerpc/pseries/fadump: convert to rtas_sched_if_busy()
  powerpc/pseries/msi: convert to rtas_sched_if_busy()
  powerpc/pseries/iommu: convert to rtas_sched_if_busy()
  powerpc/pseries/dlpar: convert to rtas_sched_if_busy()
  powerpc/rtas: convert to rtas_sched_if_busy()
  powerpc/rtas_flash: convert to rtas_sched_if_busy()

 arch/powerpc/include/asm/rtas.h              |   4 +
 arch/powerpc/kernel/rtas-rtc.c               |  38 +---
 arch/powerpc/kernel/rtas.c                   | 178 ++++++++++++++++++-
 arch/powerpc/kernel/rtas_flash.c             |   4 +-
 arch/powerpc/platforms/pseries/dlpar.c       |   2 +-
 arch/powerpc/platforms/pseries/iommu.c       |   2 +-
 arch/powerpc/platforms/pseries/msi.c         |   4 +-
 arch/powerpc/platforms/pseries/rtas-fadump.c |  22 +--
 8 files changed, 190 insertions(+), 64 deletions(-)

-- 
2.30.2

[RFC 01/10] powerpc/rtas: new APIs for busy and extended delay statuses

[Nathan Lynch]

Add new APIs for handling busy (-2) and extended delay
hint (9900...9905) statuses from RTAS. These are intended to be
drop-in replacements for existing uses of rtas_busy_delay().

A problem with rtas_busy_delay() and rtas_busy_delay_time() is that
they consider -2/busy to be equivalent to 9900 (wait 1ms). In fact,
the OS should call again as soon as it wants on -2, which at least on
PowerVM means RTAS is returning only to uphold the general requirement
that RTAS must return control to the OS in a "timely fashion" (250us).

Combine this with the fact that msleep(1) actually sleeps for more
like 20ms in practice: on busy VMs we schedule away for much longer
than necessary on -2 and 9900.

This is fixed in rtas_sched_if_busy(), which uses usleep_range() for
small delay hints, and only schedules away on -2 if there is other
work available. It also refuses to sleep longer than one second
regardless of the hinted value, on the assumption that even longer
running operations can tolerate polling at 1HZ.

rtas_spin_if_busy() and rtas_force_spin_if_busy() are provided for
atomic contexts which need to handle busy status and extended delay
hints.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/rtas.h |   4 +
 arch/powerpc/kernel/rtas.c      | 168 ++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9dc97d2f9d27..555ff3290f92 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -266,6 +266,10 @@ extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
 extern unsigned int rtas_busy_delay_time(int status);
 extern unsigned int rtas_busy_delay(int status);
 
+bool rtas_sched_if_busy(int status);
+bool rtas_spin_if_busy(int status);
+bool rtas_force_spin_if_busy(int status);
+
 extern int early_init_dt_scan_rtas(unsigned long node,
 		const char *uname, int depth, void *data);
 
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 6bada744402b..4a1dfbfa51ba 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -519,6 +519,174 @@ unsigned int rtas_busy_delay(int status)
 }
 EXPORT_SYMBOL(rtas_busy_delay);
 
+/**
+ * rtas_force_spin_if_busy() - Consume a busy or extended delay status
+ *                             in atomic context.
+ * @status: Return value from rtas_call() or similar function.
+ *
+ * Use this function when you cannot avoid using an RTAS function
+ * which may return an extended delay hint in atomic context. If
+ * possible, use rtas_spin_if_busy() or rtas_sched_if_busy() instead
+ * of this function.
+ *
+ * Return: True if @status is -2 or 990x, in which case
+ *         rtas_spin_if_busy() will have delayed an appropriate amount
+ *         of time, and the caller should call the RTAS function
+ *         again. False otherwise.
+ */
+bool rtas_force_spin_if_busy(int status)
+{
+	bool was_busy = true;
+
+	switch (status) {
+	case RTAS_BUSY:
+		/* OK to call again immediately; do nothing. */
+		break;
+	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
+		mdelay(1);
+		break;
+	default:
+		was_busy = false;
+		break;
+	}
+
+	return was_busy;
+}
+
+/**
+ * rtas_spin_if_busy() - Consume a busy status in atomic context.
+ * @status: Return value from rtas_call() or similar function.
+ *
+ * Prefer rtas_sched_if_busy() over this function. Prefer this
+ * function over rtas_force_spin_if_busy(). Use this function in
+ * atomic contexts with RTAS calls that are specified to return -2 but
+ * not 990x. This function will complain and execute a minimal delay
+ * if passed a 990x status.
+ *
+ * Return: True if @status is -2 or 990x, in which case
+ *         rtas_spin_if_busy() will have delayed an appropriate amount
+ *         of time, and the caller should call the RTAS function
+ *         again. False otherwise.
+ */
+bool rtas_spin_if_busy(int status)
+{
+	bool was_busy = true;
+
+	switch (status) {
+	case RTAS_BUSY:
+		/* OK to call again immediately; do nothing. */
+		break;
+	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
+		/*
+		 * Generally, RTAS functions which can return this
+		 * status should be considered too expensive to use in
+		 * atomic context. Change the calling code to use
+		 * rtas_sched_if_busy(), or if that's not possible,
+		 * use rtas_force_spin_if_busy().
+		 */
+		pr_warn_once("%pS may use RTAS call in atomic context which returns extended delay.\n",
+			     __builtin_return_address(0));
+		mdelay(1);
+		break;
+	default:
+		was_busy = false;
+		break;
+	}
+
+	return was_busy;
+}
+
+static unsigned long extended_delay_ms(unsigned int status)
+{
+	unsigned int extdelay;
+	unsigned int order;
+	unsigned int ms;
+
+	extdelay = clamp((int)status, RTAS_EXTENDED_DELAY_MIN, RTAS_EXTENDED_DELAY_MAX);
+	WARN_ONCE(extdelay != status, "%s passed invalid status %u\n", __func__, status);
+
+	order = status - RTAS_EXTENDED_DELAY_MIN;
+	for (ms = 1; order > 0; order--)
+		ms *= 10;
+
+	return ms;
+}
+
+static void handle_extended_delay(unsigned int status)
+{
+	unsigned long usecs;
+
+	usecs = 1000 * extended_delay_ms(status);
+
+	/*
+	 * If we have no other work pending, there's no reason to
+	 * sleep.
+	 */
+	if (!need_resched())
+		return;
+
+	/*
+	 * The extended delay hint can be as high as 100
+	 * seconds. Surely any function returning such a status is
+	 * either buggy or isn't going to be significantly slowed by
+	 * us polling at 1HZ. Clamp the sleep time to one second.
+	 */
+	usecs = clamp(usecs, 1000UL, 1000000UL);
+
+	/*
+	 * The delay hint is an order-of-magnitude suggestion, not a
+	 * minimum. It is fine, possibly even advantageous, for us to
+	 * pause for less time than suggested. For small values, use
+	 * usleep_range() to ensure we don't sleep much longer than
+	 * actually suggested.
+	 *
+	 * See Documentation/timers/timers-howto.rst for explanation
+	 * of the threshold used here.
+	 */
+	if (usecs <= 20000)
+		usleep_range(usecs / 2, 2 * usecs);
+	else
+		msleep(DIV_ROUND_UP(usecs, 1000));
+}
+
+/**
+ * rtas_sched_if_busy() - Consume a busy or extended delay status.
+ * @status: Return value from rtas_call() or similar function.
+ *
+ * Prefer this function over rtas_spin_if_busy().
+ *
+ * Context: This function may sleep.
+ *
+ * Return: True if @status is -2 or 990x, in which case
+ *         rtas_sched_if_busy() will have slept an appropriate amount
+ *         of time, and the caller should call the RTAS function
+ *         again. False otherwise.
+ */
+bool rtas_sched_if_busy(int status)
+{
+	bool was_busy = true;
+
+	might_sleep();
+
+	switch (status) {
+	case RTAS_BUSY:
+		/*
+		 * OK to call again immediately. Schedule if there's
+		 * other work to do, but no sleep is necessary.
+		 */
+		cond_resched();
+		break;
+	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
+		handle_extended_delay(status);
+		break;
+	default:
+		was_busy = false;
+		break;
+	}
+
+	return was_busy;
+}
+
 static int rtas_error_rc(int rtas_rc)
 {
 	int rc;
-- 
2.30.2

[RFC 02/10] powerpc/rtas: do not schedule in rtas_os_term()

[Nathan Lynch]

rtas_os_term() is called in the panic path and should immediately
re-call the RTAS ibm,os-term function as long as it returns a busy
status. It's not safe to use rtas_busy_delay() in this context, which
potentially can schedule away. Use rtas_spin_if_busy().

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

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 4a1dfbfa51ba..4177f7385ea2 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -960,7 +960,7 @@ 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));
+	} while (rtas_spin_if_busy(status));
 
 	if (status != 0)
 		printk(KERN_EMERG "ibm,os-term call failed %d\n", status);
-- 
2.30.2

[RFC 03/10] powerpc/rtas-rtc: convert get-time-of-day to rtas_force_spin_if_busy()

[Nathan Lynch]

The functions in rtas-rtc which call get-time-of-day can be invoked in
boot, suspend, and resume paths with interrupts off. Unfortunately
get-time-of-day can return an extended delay status, so we use
rtas_force_spin_if_busy().

In the specific case of rtas_get_rtc_time(), it is not clear why
returning an incorrect result is better than calling again even if we
are in interrupt context. Remove this logic.

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

diff --git a/arch/powerpc/kernel/rtas-rtc.c b/arch/powerpc/kernel/rtas-rtc.c
index a28239b8b0c0..82cb95f29a11 100644
--- a/arch/powerpc/kernel/rtas-rtc.c
+++ b/arch/powerpc/kernel/rtas-rtc.c
@@ -17,19 +17,12 @@ time64_t __init rtas_get_boot_time(void)
 {
 	int ret[8];
 	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);
-
-		wait_time = rtas_busy_delay_time(error);
-		if (wait_time) {
-			/* This is boot time so we spin. */
-			udelay(wait_time*1000);
-		}
-	} while (wait_time && (get_tb() < max_wait_tb));
+	} while (rtas_force_spin_if_busy(error) && (get_tb() < max_wait_tb));
 
 	if (error != 0) {
 		printk_ratelimited(KERN_WARNING
@@ -41,33 +34,16 @@ time64_t __init rtas_get_boot_time(void)
 	return mktime64(ret[0], ret[1], ret[2], ret[3], ret[4], ret[5]);
 }
 
-/* NOTE: get_rtc_time will get an error if executed in interrupt context
- * and if a delay is needed to read the clock.  In this case we just
- * silently return without updating rtc_tm.
- */
 void rtas_get_rtc_time(struct rtc_time *rtc_tm)
 {
         int ret[8];
 	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);
-
-		wait_time = rtas_busy_delay_time(error);
-		if (wait_time) {
-			if (in_interrupt()) {
-				memset(rtc_tm, 0, sizeof(struct rtc_time));
-				printk_ratelimited(KERN_WARNING
-						   "error: reading clock "
-						   "would delay interrupt\n");
-				return;	/* delay not allowed */
-			}
-			msleep(wait_time);
-		}
-	} while (wait_time && (get_tb() < max_wait_tb));
+	} while (rtas_sched_if_busy(error) && (get_tb() < max_wait_tb));
 
 	if (error != 0) {
 		printk_ratelimited(KERN_WARNING
-- 
2.30.2

[RFC 04/10] powerpc/rtas-rtc: convert set-time-of-day to rtas_sched_if_busy()

[Nathan Lynch]

rtas_set_rtc_time() is called only in process context; convert this to
rtas_sched_if_busy().

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

diff --git a/arch/powerpc/kernel/rtas-rtc.c b/arch/powerpc/kernel/rtas-rtc.c
index 82cb95f29a11..421b92f95669 100644
--- a/arch/powerpc/kernel/rtas-rtc.c
+++ b/arch/powerpc/kernel/rtas-rtc.c
@@ -62,7 +62,7 @@ void rtas_get_rtc_time(struct rtc_time *rtc_tm)
 
 int rtas_set_rtc_time(struct rtc_time *tm)
 {
-	int error, wait_time;
+	int error;
 	u64 max_wait_tb;
 
 	max_wait_tb = get_tb() + tb_ticks_per_usec * 1000 * MAX_RTC_WAIT;
@@ -72,13 +72,7 @@ int rtas_set_rtc_time(struct rtc_time *tm)
 				  tm->tm_mday, tm->tm_hour, tm->tm_min,
 				  tm->tm_sec, 0);
 
-		wait_time = rtas_busy_delay_time(error);
-		if (wait_time) {
-			if (in_interrupt())
-				return 1;	/* probably decrementer */
-			msleep(wait_time);
-		}
-	} while (wait_time && (get_tb() < max_wait_tb));
+	} while (rtas_sched_if_busy(error) && (get_tb() < max_wait_tb));
 
 	if (error != 0)
 		printk_ratelimited(KERN_WARNING
-- 
2.30.2

[RFC 05/10] powerpc/pseries/fadump: convert to rtas_sched_if_busy()

[Nathan Lynch]

None of these call sites need to use mdelay(); convert them to
rtas_sched_if_busy().

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/rtas-fadump.c | 22 +++-----------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.c b/arch/powerpc/platforms/pseries/rtas-fadump.c
index f8f73b47b107..9a200d3bf5e0 100644
--- a/arch/powerpc/platforms/pseries/rtas-fadump.c
+++ b/arch/powerpc/platforms/pseries/rtas-fadump.c
@@ -129,7 +129,6 @@ static u64 rtas_fadump_get_bootmem_min(void)
 
 static int rtas_fadump_register(struct fw_dump *fadump_conf)
 {
-	unsigned int wait_time;
 	int rc, err = -EIO;
 
 	/* TODO: Add upper time limit for the delay */
@@ -137,12 +136,7 @@ static int rtas_fadump_register(struct fw_dump *fadump_conf)
 		rc =  rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
 				NULL, FADUMP_REGISTER, &fdm,
 				sizeof(struct rtas_fadump_mem_struct));
-
-		wait_time = rtas_busy_delay_time(rc);
-		if (wait_time)
-			mdelay(wait_time);
-
-	} while (wait_time);
+	} while (rtas_sched_if_busy(rc));
 
 	switch (rc) {
 	case 0:
@@ -177,7 +171,6 @@ static int rtas_fadump_register(struct fw_dump *fadump_conf)
 
 static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
 {
-	unsigned int wait_time;
 	int rc;
 
 	/* TODO: Add upper time limit for the delay */
@@ -185,11 +178,7 @@ static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
 		rc =  rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
 				NULL, FADUMP_UNREGISTER, &fdm,
 				sizeof(struct rtas_fadump_mem_struct));
-
-		wait_time = rtas_busy_delay_time(rc);
-		if (wait_time)
-			mdelay(wait_time);
-	} while (wait_time);
+	} while (rtas_sched_if_busy(rc));
 
 	if (rc) {
 		pr_err("Failed to un-register - unexpected error(%d).\n", rc);
@@ -202,7 +191,6 @@ static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
 
 static int rtas_fadump_invalidate(struct fw_dump *fadump_conf)
 {
-	unsigned int wait_time;
 	int rc;
 
 	/* TODO: Add upper time limit for the delay */
@@ -210,11 +198,7 @@ static int rtas_fadump_invalidate(struct fw_dump *fadump_conf)
 		rc =  rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
 				NULL, FADUMP_INVALIDATE, fdm_active,
 				sizeof(struct rtas_fadump_mem_struct));
-
-		wait_time = rtas_busy_delay_time(rc);
-		if (wait_time)
-			mdelay(wait_time);
-	} while (wait_time);
+	} while (rtas_sched_if_busy(rc));
 
 	if (rc) {
 		pr_err("Failed to invalidate - unexpected error (%d).\n", rc);
-- 
2.30.2

[RFC 06/10] powerpc/pseries/msi: convert to rtas_sched_if_busy()

[Nathan Lynch]

rtas_sched_if_busy() has better behavior for RTAS_BUSY (-2) and small
extended delay values.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/msi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 637300330507..df434b8a3aa7 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -49,7 +49,7 @@ static int rtas_change_msi(struct pci_dn *pdn, u32 func, u32 num_irqs)
 					func, num_irqs, seq_num);
 
 		seq_num = rtas_ret[1];
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	/*
 	 * If the RTAS call succeeded, return the number of irqs allocated.
@@ -100,7 +100,7 @@ static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
 	do {
 		rc = rtas_call(query_token, 4, 3, rtas_ret, addr,
 			       BUID_HI(buid), BUID_LO(buid), offset);
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	if (rc) {
 		pr_debug("rtas_msi: error (%d) querying source number\n", rc);
-- 
2.30.2

[RFC 07/10] powerpc/pseries/iommu: convert to rtas_sched_if_busy()

[Nathan Lynch]

rtas_sched_if_busy() has better behavior for RTAS_BUSY (-2) and small
extended delay values.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 0c55b991f665..0f0e7a51b863 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1016,7 +1016,7 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail,
 		ret = rtas_call(ddw_avail[DDW_CREATE_PE_DMA_WIN], 5, 4,
 				(u32 *)create, cfg_addr, BUID_HI(buid),
 				BUID_LO(buid), page_shift, window_shift);
-	} while (rtas_busy_delay(ret));
+	} while (rtas_sched_if_busy(ret));
 	dev_info(&dev->dev,
 		"ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "
 		"(liobn = 0x%x starting addr = %x %x)\n",
-- 
2.30.2

[RFC 08/10] powerpc/pseries/dlpar: convert to rtas_sched_if_busy()

[Nathan Lynch]

rtas_sched_if_busy() has better behavior for RTAS_BUSY (-2) and small
extended delay values.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 3ac70790ec7a..3ba77bc09a6e 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -167,7 +167,7 @@ struct device_node *dlpar_configure_connector(__be32 drc_index,
 
 		spin_unlock(&rtas_data_buf_lock);
 
-		if (rtas_busy_delay(rc))
+		if (rtas_sched_if_busy(rc))
 			continue;
 
 		switch (rc) {
-- 
2.30.2

[RFC 09/10] powerpc/rtas: convert to rtas_sched_if_busy()

[Nathan Lynch]

rtas_sched_if_busy() has better behavior for RTAS_BUSY (-2) and small
extended delay values.

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

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 4177f7385ea2..c5cc4542856f 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -743,7 +743,7 @@ int rtas_set_power_level(int powerdomain, int level, int *setlevel)
 
 	do {
 		rc = rtas_call(token, 2, 2, setlevel, powerdomain, level);
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -761,7 +761,7 @@ int rtas_get_sensor(int sensor, int index, int *state)
 
 	do {
 		rc = rtas_call(token, 2, 2, state, sensor, index);
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -822,7 +822,7 @@ int rtas_set_indicator(int indicator, int index, int new_value)
 
 	do {
 		rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	if (rc < 0)
 		return rtas_error_rc(rc);
@@ -990,7 +990,7 @@ void rtas_activate_firmware(void)
 
 	do {
 		fwrc = rtas_call(token, 0, 1, NULL);
-	} while (rtas_busy_delay(fwrc));
+	} while (rtas_sched_if_busy(fwrc));
 
 	if (fwrc)
 		pr_err("ibm,activate-firmware failed (%i)\n", fwrc);
-- 
2.30.2

[RFC 10/10] powerpc/rtas_flash: convert to rtas_sched_if_busy()

[Nathan Lynch]

rtas_sched_if_busy() has better behavior for RTAS_BUSY (-2) and small
extended delay values.

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

diff --git a/arch/powerpc/kernel/rtas_flash.c b/arch/powerpc/kernel/rtas_flash.c
index a99179d83538..bedefb9178ec 100644
--- a/arch/powerpc/kernel/rtas_flash.c
+++ b/arch/powerpc/kernel/rtas_flash.c
@@ -378,7 +378,7 @@ static void manage_flash(struct rtas_manage_flash_t *args_buf, unsigned int op)
 	do {
 		rc = rtas_call(rtas_token("ibm,manage-flash-image"), 1, 1,
 			       NULL, op);
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	args_buf->status = rc;
 }
@@ -456,7 +456,7 @@ static void validate_flash(struct rtas_validate_flash_t *args_buf)
 			       (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);
-	} while (rtas_busy_delay(rc));
+	} while (rtas_sched_if_busy(rc));
 
 	args_buf->status = rc;
 	args_buf->update_results = update_results;
-- 
2.30.2

Re: [RFC 10/10] powerpc/rtas_flash: convert to rtas_sched_if_busy()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

Hi Nathan,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.12 next-20210504]
[cannot apply to scottwood/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nathan-Lynch/powerpc-rtas-improved-busy-and-extended-delay-status-handling/20210504-111153
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f8a8e0e560b20b03cccc4fe99f121d89a0975c04
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nathan-Lynch/powerpc-rtas-improved-busy-and-extended-delay-status-handling/20210504-111153
        git checkout f8a8e0e560b20b03cccc4fe99f121d89a0975c04
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: ".rtas_sched_if_busy" [arch/powerpc/kernel/rtas_flash.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 73285 bytes --]

Re: [RFC 01/10] powerpc/rtas: new APIs for busy and extended delay statuses

[Alexey Kardashevskiy]

On 04/05/2021 13:03, Nathan Lynch wrote:
> Add new APIs for handling busy (-2) and extended delay
> hint (9900...9905) statuses from RTAS. These are intended to be
> drop-in replacements for existing uses of rtas_busy_delay().
> 
> A problem with rtas_busy_delay() and rtas_busy_delay_time() is that
> they consider -2/busy to be equivalent to 9900 (wait 1ms). In fact,
> the OS should call again as soon as it wants on -2, which at least on
> PowerVM means RTAS is returning only to uphold the general requirement
> that RTAS must return control to the OS in a "timely fashion" (250us).
> 
> Combine this with the fact that msleep(1) actually sleeps for more
> like 20ms in practice: on busy VMs we schedule away for much longer
> than necessary on -2 and 9900.
> 
> This is fixed in rtas_sched_if_busy(), which uses usleep_range() for
> small delay hints, and only schedules away on -2 if there is other
> work available. It also refuses to sleep longer than one second
> regardless of the hinted value, on the assumption that even longer
> running operations can tolerate polling at 1HZ.
> 
> rtas_spin_if_busy() and rtas_force_spin_if_busy() are provided for
> atomic contexts which need to handle busy status and extended delay
> hints.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/rtas.h |   4 +
>   arch/powerpc/kernel/rtas.c      | 168 ++++++++++++++++++++++++++++++++
>   2 files changed, 172 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 9dc97d2f9d27..555ff3290f92 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -266,6 +266,10 @@ extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
>   extern unsigned int rtas_busy_delay_time(int status);
>   extern unsigned int rtas_busy_delay(int status);
>   
> +bool rtas_sched_if_busy(int status);
> +bool rtas_spin_if_busy(int status);
> +bool rtas_force_spin_if_busy(int status);
> +
>   extern int early_init_dt_scan_rtas(unsigned long node,
>   		const char *uname, int depth, void *data);
>   
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 6bada744402b..4a1dfbfa51ba 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -519,6 +519,174 @@ unsigned int rtas_busy_delay(int status)
>   }
>   EXPORT_SYMBOL(rtas_busy_delay);
>   
> +/**
> + * rtas_force_spin_if_busy() - Consume a busy or extended delay status
> + *                             in atomic context.
> + * @status: Return value from rtas_call() or similar function.
> + *
> + * Use this function when you cannot avoid using an RTAS function
> + * which may return an extended delay hint in atomic context. If
> + * possible, use rtas_spin_if_busy() or rtas_sched_if_busy() instead
> + * of this function.
> + *
> + * Return: True if @status is -2 or 990x, in which case
> + *         rtas_spin_if_busy() will have delayed an appropriate amount
> + *         of time, and the caller should call the RTAS function
> + *         again. False otherwise.
> + */
> +bool rtas_force_spin_if_busy(int status)

rtas_force_delay_if_busy()? neither this one nor rtas_spin_if_busy() 
actually spins.


> +{
> +	bool was_busy = true;
> +
> +	switch (status) {
> +	case RTAS_BUSY:
> +		/* OK to call again immediately; do nothing. */
> +		break;
> +	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
> +		mdelay(1);
> +		break;
> +	default:
> +		was_busy = false;
> +		break;
> +	}
> +
> +	return was_busy;
> +}
> +
> +/**
> + * rtas_spin_if_busy() - Consume a busy status in atomic context.
> + * @status: Return value from rtas_call() or similar function.
> + *
> + * Prefer rtas_sched_if_busy() over this function. Prefer this
> + * function over rtas_force_spin_if_busy(). Use this function in
> + * atomic contexts with RTAS calls that are specified to return -2 but
> + * not 990x. This function will complain and execute a minimal delay
> + * if passed a 990x status.
> + *
> + * Return: True if @status is -2 or 990x, in which case
> + *         rtas_spin_if_busy() will have delayed an appropriate amount
> + *         of time, and the caller should call the RTAS function
> + *         again. False otherwise.
> + */
> +bool rtas_spin_if_busy(int status)

rtas_delay_if_busy()?


> +{
> +	bool was_busy = true;
> +
> +	switch (status) {
> +	case RTAS_BUSY:
> +		/* OK to call again immediately; do nothing. */
> +		break;
> +	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
> +		/*
> +		 * Generally, RTAS functions which can return this
> +		 * status should be considered too expensive to use in
> +		 * atomic context. Change the calling code to use
> +		 * rtas_sched_if_busy(), or if that's not possible,
> +		 * use rtas_force_spin_if_busy().
> +		 */
> +		pr_warn_once("%pS may use RTAS call in atomic context which returns extended delay.\n",
> +			     __builtin_return_address(0));
> +		mdelay(1);
> +		break;
> +	default:
> +		was_busy = false;
> +		break;
> +	}
> +
> +	return was_busy;
> +}
> +
> +static unsigned long extended_delay_ms(unsigned int status)

extended_delay_to_ms()?

> +{
> +	unsigned int extdelay;
> +	unsigned int order;
> +	unsigned int ms;
> +
> +	extdelay = clamp((int)status, RTAS_EXTENDED_DELAY_MIN, RTAS_EXTENDED_DELAY_MAX);
> +	WARN_ONCE(extdelay != status, "%s passed invalid status %u\n", __func__, status);
> +
> +	order = status - RTAS_EXTENDED_DELAY_MIN;


Using extdelay instead of status seems safer.


> +	for (ms = 1; order > 0; order--)
> +		ms *= 10;
> +
> +	return ms;
> +}
> +
> +static void handle_extended_delay(unsigned int status)

rtas_sleep()? rtas_extended_delay()?


> +{
> +	unsigned long usecs;
> +
> +	usecs = 1000 * extended_delay_ms(status);


usecs could be msecs, you would need fewer zeroes and would not need 
DIV_ROUND_UP below...


> +
> +	/*
> +	 * If we have no other work pending, there's no reason to
> +	 * sleep.
> +	 */
> +	if (!need_resched())
> +		return;
> +
> +	/*
> +	 * The extended delay hint can be as high as 100
> +	 * seconds. Surely any function returning such a status is
> +	 * either buggy or isn't going to be significantly slowed by
> +	 * us polling at 1HZ. Clamp the sleep time to one second.
> +	 */
> +	usecs = clamp(usecs, 1000UL, 1000000UL);
> +
> +	/*
> +	 * The delay hint is an order-of-magnitude suggestion, not a
> +	 * minimum. It is fine, possibly even advantageous, for us to
> +	 * pause for less time than suggested. For small values, use
> +	 * usleep_range() to ensure we don't sleep much longer than
> +	 * actually suggested.
> +	 *
> +	 * See Documentation/timers/timers-howto.rst for explanation
> +	 * of the threshold used here.
> +	 */
> +	if (usecs <= 20000)
> +		usleep_range(usecs / 2, 2 * usecs);

... and this would be usleep_range(msecs*500, msecs*2000).


> +	else
> +		msleep(DIV_ROUND_UP(usecs, 1000));
> +}
> +
> +/**
> + * rtas_sched_if_busy() - Consume a busy or extended delay status.
> + * @status: Return value from rtas_call() or similar function.
> + *
> + * Prefer this function over rtas_spin_if_busy().
> + *
> + * Context: This function may sleep.
> + *
> + * Return: True if @status is -2 or 990x, in which case
> + *         rtas_sched_if_busy() will have slept an appropriate amount
> + *         of time, and the caller should call the RTAS function
> + *         again. False otherwise.
> + */
> +bool rtas_sched_if_busy(int status)
> +{
> +	bool was_busy = true;
> +
> +	might_sleep();
> +
> +	switch (status) {
> +	case RTAS_BUSY:
> +		/*
> +		 * OK to call again immediately. Schedule if there's
> +		 * other work to do, but no sleep is necessary.
> +		 */
> +		cond_resched();
> +		break;
> +	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
> +		handle_extended_delay(status);
> +		break;
> +	default:
> +		was_busy = false;
> +		break;
> +	}
> +
> +	return was_busy;
> +}

Throughout the series this one is called instead of simple spinning, 
03/10..05/10, and some of those have in_interrupt() checks for a reason 
(which I do not know but nevertheless) so they may be called with 
interrupts disabled which in turn means we should not be calling 
cond_resched() unconditionally. I am told this is should be done:


if (!preemptible())
	/* print warning, don't sleep or cond_resched */



> +
>   static int rtas_error_rc(int rtas_rc)
>   {
>   	int rc;
> 

-- 
Alexey