[PATCH 00/15] hvc and powerpc opal console latency reduction

[PATCH 00/15] hvc and powerpc opal console latency reduction

[Nicholas Piggin]

I'm seeing scattered reports of hard lockups triggering in the OPAL
console code. I haven't got a full latency trace -- they are difficult
to reproduce and sometimes just show up in dmesg of a bug report when
the system is having other issues. But it does seem like there are
some improvements that can be made to the hvc console and powerpc
opal driver.

I'm not sure how this series should be merged yet, but we can cross
that if/when it comes. Possibly all can go via the tty tree with
acks from powerpc maintainers.

Comments appreciated.

Thanks,
Nick

--
Nicholas Piggin (15):
  powerpc/powernv: opal_put_chars partial write fix
  powerpc/powernv: Fix OPAL console driver OPAL_BUSY loops
  powerpc/powernv: opal-kmsg standardise OPAL_BUSY handling
  powerpc/powernv: opal-kmsg use flush fallback from console code
  powerpc/powernv: Implement and use opal_flush_console
  powerpc/powernv: Remove OPALv1 support from opal console driver
  powerpc/powernv: move opal console flushing to udbg
  powerpc/powernv: implement opal_put_chars_atomic
  tty: hvc: remove unexplained "just in case" spin delay
  tty: hvc: use mutex instead of spinlock for hvc_structs lock
  tty: hvc: hvc_poll break hv read loop
  tty: hvc: hvc_poll may sleep
  tty: hvc: hvc_write may sleep
  tty: hvc: introduce the hv_ops.flush operation for hvc drivers
  powerpc/powernv: provide a console flush operation for opal hvc driver

 arch/powerpc/include/asm/opal.h            |   3 +
 arch/powerpc/platforms/powernv/opal-kmsg.c |  30 +---
 arch/powerpc/platforms/powernv/opal.c      | 176 +++++++++++++------
 drivers/tty/hvc/hvc_console.c              | 194 +++++++++++++--------
 drivers/tty/hvc/hvc_console.h              |   1 +
 drivers/tty/hvc/hvc_opal.c                 |  33 ++--
 6 files changed, 276 insertions(+), 161 deletions(-)

-- 
2.17.0

[PATCH 01/15] powerpc/powernv: opal_put_chars partial write fix

[Nicholas Piggin]

The intention here is to consume and discard the remaining buffer
upon error. This works if there has not been a previous partial write.
If there has been, then total_len is no longer total number of bytes
to copy. total_len is always "bytes left to copy", so it should be
added to written bytes.

This code may not be exercised any more if partial writes will not be
hit, but this is a small bugfix before a larger change.

Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 48fbb41af5d1..e695b836fd49 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -388,7 +388,7 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 		/* Closed or other error drop */
 		if (rc != OPAL_SUCCESS && rc != OPAL_BUSY &&
 		    rc != OPAL_BUSY_EVENT) {
-			written = total_len;
+			written += total_len;
 			break;
 		}
 		if (rc == OPAL_SUCCESS) {
-- 
2.17.0

Re: [01/15] powerpc/powernv: opal_put_chars partial write fix

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:44 UTC, Nicholas Piggin wrote:
> The intention here is to consume and discard the remaining buffer
> upon error. This works if there has not been a previous partial write.
> If there has been, then total_len is no longer total number of bytes
> to copy. total_len is always "bytes left to copy", so it should be
> added to written bytes.
> 
> This code may not be exercised any more if partial writes will not be
> hit, but this is a small bugfix before a larger change.
> 
> Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Patches 1-9 applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/bd90284cc6c1c9e8e48c8eadd0c795

cheers

[PATCH 02/15] powerpc/powernv: Fix OPAL console driver OPAL_BUSY loops

[Nicholas Piggin]

The OPAL console driver does not delay in case it gets OPAL_BUSY or
OPAL_BUSY_EVENT from firmware.

It can't yet be made to sleep because it is called under spinlock,
but it can be changed to the standard OPAL_BUSY loop form, and a
delay added to keep it from hitting the firmware too frequently.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 38 ++++++++++++++++-----------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index e695b836fd49..6b621d47ac29 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -378,33 +378,41 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 	/* We still try to handle partial completions, though they
 	 * should no longer happen.
 	 */
-	rc = OPAL_BUSY;
-	while(total_len > 0 && (rc == OPAL_BUSY ||
-				rc == OPAL_BUSY_EVENT || rc == OPAL_SUCCESS)) {
+
+	while (total_len > 0) {
 		olen = cpu_to_be64(total_len);
-		rc = opal_console_write(vtermno, &olen, data);
+
+		rc = OPAL_BUSY;
+		while (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
+			rc = opal_console_write(vtermno, &olen, data);
+			if (rc == OPAL_BUSY_EVENT) {
+				mdelay(OPAL_BUSY_DELAY_MS);
+				opal_poll_events(NULL);
+			} else if (rc == OPAL_BUSY) {
+				mdelay(OPAL_BUSY_DELAY_MS);
+			}
+		}
+
 		len = be64_to_cpu(olen);
 
 		/* Closed or other error drop */
-		if (rc != OPAL_SUCCESS && rc != OPAL_BUSY &&
-		    rc != OPAL_BUSY_EVENT) {
-			written += total_len;
+		if (rc != OPAL_SUCCESS) {
+			written += total_len; /* drop remaining chars */
 			break;
 		}
-		if (rc == OPAL_SUCCESS) {
-			total_len -= len;
-			data += len;
-			written += len;
-		}
+
+		total_len -= len;
+		data += len;
+		written += len;
+
 		/* This is a bit nasty but we need that for the console to
 		 * flush when there aren't any interrupts. We will clean
 		 * things a bit later to limit that to synchronous path
 		 * such as the kernel console and xmon/udbg
 		 */
-		do
+		do {
 			opal_poll_events(&evt);
-		while(rc == OPAL_SUCCESS &&
-			(be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT));
+		} while (be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT);
 	}
 	spin_unlock_irqrestore(&opal_write_lock, flags);
 	return written;
-- 
2.17.0

[PATCH 03/15] powerpc/powernv: opal-kmsg standardise OPAL_BUSY handling

[Nicholas Piggin]

OPAL_CONSOLE_FLUSH is documented as being able to return OPAL_BUSY,
so implement the standard OPAL_BUSY handling for it.

Reviewed-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal-kmsg.c | 24 ++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c
index 6f1214d4de92..f8f41ccce75f 100644
--- a/arch/powerpc/platforms/powernv/opal-kmsg.c
+++ b/arch/powerpc/platforms/powernv/opal-kmsg.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/kmsg_dump.h>
+#include <linux/delay.h>
 
 #include <asm/opal.h>
 #include <asm/opal-api.h>
@@ -26,8 +27,7 @@
 static void force_opal_console_flush(struct kmsg_dumper *dumper,
 				     enum kmsg_dump_reason reason)
 {
-	int i;
-	int64_t ret;
+	s64 rc;
 
 	/*
 	 * Outside of a panic context the pollers will continue to run,
@@ -37,14 +37,22 @@ static void force_opal_console_flush(struct kmsg_dumper *dumper,
 		return;
 
 	if (opal_check_token(OPAL_CONSOLE_FLUSH)) {
-		ret = opal_console_flush(0);
+		do  {
+			rc = OPAL_BUSY;
+			while (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
+				rc = opal_console_flush(0);
+				if (rc == OPAL_BUSY_EVENT) {
+					mdelay(OPAL_BUSY_DELAY_MS);
+					opal_poll_events(NULL);
+				} else if (rc == OPAL_BUSY) {
+					mdelay(OPAL_BUSY_DELAY_MS);
+				}
+			}
+		} while (rc == OPAL_PARTIAL); /* More to flush */
 
-		if (ret == OPAL_UNSUPPORTED || ret == OPAL_PARAMETER)
-			return;
-
-		/* Incrementally flush until there's nothing left */
-		while (opal_console_flush(0) != OPAL_SUCCESS);
 	} else {
+		int i;
+
 		/*
 		 * If OPAL_CONSOLE_FLUSH is not implemented in the firmware,
 		 * the console can still be flushed by calling the polling
-- 
2.17.0

[PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code

[Nicholas Piggin]

Use the more refined and tested event polling loop from opal_put_chars
as the fallback console flush in the opal-kmsg path. This loop is used
by the console driver today, whereas the opal-kmsg fallback is not
likely to have been used for years.

Use WARN_ONCE rather than a printk when the fallback is invoked to
prepare for moving the console flush into a common function.

Reviewed-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal-kmsg.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c
index f8f41ccce75f..fd2bbf4fd6dc 100644
--- a/arch/powerpc/platforms/powernv/opal-kmsg.c
+++ b/arch/powerpc/platforms/powernv/opal-kmsg.c
@@ -51,20 +51,17 @@ static void force_opal_console_flush(struct kmsg_dumper *dumper,
 		} while (rc == OPAL_PARTIAL); /* More to flush */
 
 	} else {
-		int i;
+		__be64 evt;
 
+		WARN_ONCE(1, "opal: OPAL_CONSOLE_FLUSH missing.\n");
 		/*
 		 * If OPAL_CONSOLE_FLUSH is not implemented in the firmware,
 		 * the console can still be flushed by calling the polling
-		 * function enough times to flush the buffer.  We don't know
-		 * how much output still needs to be flushed, but we can be
-		 * generous since the kernel is in panic and doesn't need
-		 * to do much else.
+		 * function while it has OPAL_EVENT_CONSOLE_OUTPUT events.
 		 */
-		printk(KERN_NOTICE "opal: OPAL_CONSOLE_FLUSH missing.\n");
-		for (i = 0; i < 1024; i++) {
-			opal_poll_events(NULL);
-		}
+		do {
+			opal_poll_events(&evt);
+		} while (be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT);
 	}
 }
 
-- 
2.17.0

Re: [PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> Use the more refined and tested event polling loop from opal_put_chars
> as the fallback console flush in the opal-kmsg path. This loop is used
> by the console driver today, whereas the opal-kmsg fallback is not
> likely to have been used for years.
>
> Use WARN_ONCE rather than a printk when the fallback is invoked to
> prepare for moving the console flush into a common function.

Do we want to add a WARN in that path? If we're panicking things might
get worse if we WARN (which takes a trap).

cheers

Re: [PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code

[Nicholas Piggin]

On Fri, 04 May 2018 15:16:37 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > Use the more refined and tested event polling loop from opal_put_chars
> > as the fallback console flush in the opal-kmsg path. This loop is used
> > by the console driver today, whereas the opal-kmsg fallback is not
> > likely to have been used for years.
> >
> > Use WARN_ONCE rather than a printk when the fallback is invoked to
> > prepare for moving the console flush into a common function.  
> 
> Do we want to add a WARN in that path? If we're panicking things might
> get worse if we WARN (which takes a trap).

True, probably a good idea not to... oh there's a printk_once so
that'll work nicely.

Thanks,
Nick

Re: [PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> On Fri, 04 May 2018 15:16:37 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> > Use the more refined and tested event polling loop from opal_put_chars
>> > as the fallback console flush in the opal-kmsg path. This loop is used
>> > by the console driver today, whereas the opal-kmsg fallback is not
>> > likely to have been used for years.
>> >
>> > Use WARN_ONCE rather than a printk when the fallback is invoked to
>> > prepare for moving the console flush into a common function.  
>> 
>> Do we want to add a WARN in that path? If we're panicking things might
>> get worse if we WARN (which takes a trap).
>
> True, probably a good idea not to... oh there's a printk_once so
> that'll work nicely.

Cool.

I have this series in a tree so you can send me an incremental diff if
it's reasonably small.

cheers

Re: [PATCH 04/15] powerpc/powernv: opal-kmsg use flush fallback from console code

[Nicholas Piggin]

On Mon, 07 May 2018 20:36:39 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> > On Fri, 04 May 2018 15:16:37 +1000
> > Michael Ellerman <mpe@ellerman.id.au> wrote:  
> >> Nicholas Piggin <npiggin@gmail.com> writes:  
> >> > Use the more refined and tested event polling loop from opal_put_chars
> >> > as the fallback console flush in the opal-kmsg path. This loop is used
> >> > by the console driver today, whereas the opal-kmsg fallback is not
> >> > likely to have been used for years.
> >> >
> >> > Use WARN_ONCE rather than a printk when the fallback is invoked to
> >> > prepare for moving the console flush into a common function.    
> >> 
> >> Do we want to add a WARN in that path? If we're panicking things might
> >> get worse if we WARN (which takes a trap).  
> >
> > True, probably a good idea not to... oh there's a printk_once so
> > that'll work nicely.  
> 
> Cool.
> 
> I have this series in a tree so you can send me an incremental diff if
> it's reasonably small.

It's a one liner (also moved location of message back to where it was
originally).

The next patch will clash because it moves this over into opal.c, so
you'd have to fix that by hand.

Thanks,
Nick

---
 arch/powerpc/platforms/powernv/opal-kmsg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c
index fd2bbf4fd6dc..c610ef3541aa 100644
--- a/arch/powerpc/platforms/powernv/opal-kmsg.c
+++ b/arch/powerpc/platforms/powernv/opal-kmsg.c
@@ -53,12 +53,12 @@ static void force_opal_console_flush(struct kmsg_dumper *dumper,
 	} else {
 		__be64 evt;
 
-		WARN_ONCE(1, "opal: OPAL_CONSOLE_FLUSH missing.\n");
 		/*
 		 * If OPAL_CONSOLE_FLUSH is not implemented in the firmware,
 		 * the console can still be flushed by calling the polling
 		 * function while it has OPAL_EVENT_CONSOLE_OUTPUT events.
 		 */
+		printk_once(KERN_NOTICE "opal: OPAL_CONSOLE_FLUSH missing.\n");
 		do {
 			opal_poll_events(&evt);
 		} while (be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT);
-- 
2.17.0

[PATCH 05/15] powerpc/powernv: Implement and use opal_flush_console

[Nicholas Piggin]

A new console flushing firmware API was introduced to replace event
polling loops, and implemented in opal-kmsg with affddff69c55e
("powerpc/powernv: Add a kmsg_dumper that flushes console output on
panic"), to flush the console in the panic path.

The OPAL console driver has other situations where interrupts are off
and it needs to flush the console synchronously. These still use a
polling loop.

So move the opal-kmsg flush code to opal_flush_console, and use the
new function in opal-kmsg and opal_put_chars.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/opal.h            |  1 +
 arch/powerpc/platforms/powernv/opal-kmsg.c | 35 ++----------------
 arch/powerpc/platforms/powernv/opal.c      | 42 +++++++++++++++++++---
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 03e1a920491e..bbff49fab0e5 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -303,6 +303,7 @@ extern void opal_configure_cores(void);
 
 extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
 extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
+extern int opal_flush_console(uint32_t vtermno);
 
 extern void hvc_opal_init_early(void);
 
diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c
index fd2bbf4fd6dc..55691950d981 100644
--- a/arch/powerpc/platforms/powernv/opal-kmsg.c
+++ b/arch/powerpc/platforms/powernv/opal-kmsg.c
@@ -12,7 +12,6 @@
  */
 
 #include <linux/kmsg_dump.h>
-#include <linux/delay.h>
 
 #include <asm/opal.h>
 #include <asm/opal-api.h>
@@ -24,11 +23,9 @@
  * may not be completely printed.  This function does not actually dump the
  * message, it just ensures that OPAL completely flushes the console buffer.
  */
-static void force_opal_console_flush(struct kmsg_dumper *dumper,
+static void kmsg_dump_opal_console_flush(struct kmsg_dumper *dumper,
 				     enum kmsg_dump_reason reason)
 {
-	s64 rc;
-
 	/*
 	 * Outside of a panic context the pollers will continue to run,
 	 * so we don't need to do any special flushing.
@@ -36,37 +33,11 @@ static void force_opal_console_flush(struct kmsg_dumper *dumper,
 	if (reason != KMSG_DUMP_PANIC)
 		return;
 
-	if (opal_check_token(OPAL_CONSOLE_FLUSH)) {
-		do  {
-			rc = OPAL_BUSY;
-			while (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
-				rc = opal_console_flush(0);
-				if (rc == OPAL_BUSY_EVENT) {
-					mdelay(OPAL_BUSY_DELAY_MS);
-					opal_poll_events(NULL);
-				} else if (rc == OPAL_BUSY) {
-					mdelay(OPAL_BUSY_DELAY_MS);
-				}
-			}
-		} while (rc == OPAL_PARTIAL); /* More to flush */
-
-	} else {
-		__be64 evt;
-
-		WARN_ONCE(1, "opal: OPAL_CONSOLE_FLUSH missing.\n");
-		/*
-		 * If OPAL_CONSOLE_FLUSH is not implemented in the firmware,
-		 * the console can still be flushed by calling the polling
-		 * function while it has OPAL_EVENT_CONSOLE_OUTPUT events.
-		 */
-		do {
-			opal_poll_events(&evt);
-		} while (be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT);
-	}
+	opal_flush_console(0);
 }
 
 static struct kmsg_dumper opal_kmsg_dumper = {
-	.dump = force_opal_console_flush
+	.dump = kmsg_dump_opal_console_flush
 };
 
 void __init opal_kmsg_init(void)
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 6b621d47ac29..6640fccbf30c 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -350,7 +350,6 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 	__be64 olen;
 	s64 len, rc;
 	unsigned long flags;
-	__be64 evt;
 
 	if (!opal.entry)
 		return -ENODEV;
@@ -371,7 +370,7 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 		/* Closed -> drop characters */
 		if (rc)
 			return total_len;
-		opal_poll_events(NULL);
+		opal_flush_console(vtermno);
 		return -EAGAIN;
 	}
 
@@ -410,12 +409,47 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 		 * things a bit later to limit that to synchronous path
 		 * such as the kernel console and xmon/udbg
 		 */
+		opal_flush_console(vtermno);
+	}
+	spin_unlock_irqrestore(&opal_write_lock, flags);
+
+	return written;
+}
+
+int opal_flush_console(uint32_t vtermno)
+{
+	s64 rc;
+
+	if (!opal_check_token(OPAL_CONSOLE_FLUSH)) {
+		__be64 evt;
+
+		WARN_ONCE(1, "opal: OPAL_CONSOLE_FLUSH missing.\n");
+		/*
+		 * If OPAL_CONSOLE_FLUSH is not implemented in the firmware,
+		 * the console can still be flushed by calling the polling
+		 * function while it has OPAL_EVENT_CONSOLE_OUTPUT events.
+		 */
 		do {
 			opal_poll_events(&evt);
 		} while (be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT);
+
+		return OPAL_SUCCESS;
 	}
-	spin_unlock_irqrestore(&opal_write_lock, flags);
-	return written;
+
+	do  {
+		rc = OPAL_BUSY;
+		while (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
+			rc = opal_console_flush(vtermno);
+			if (rc == OPAL_BUSY_EVENT) {
+				mdelay(OPAL_BUSY_DELAY_MS);
+				opal_poll_events(NULL);
+			} else if (rc == OPAL_BUSY) {
+				mdelay(OPAL_BUSY_DELAY_MS);
+			}
+		}
+	} while (rc == OPAL_PARTIAL); /* More to flush */
+
+	return opal_error_code(rc);
 }
 
 static int opal_recover_mce(struct pt_regs *regs,
-- 
2.17.0

[PATCH 06/15] powerpc/powernv: Remove OPALv1 support from opal console driver

[Nicholas Piggin]

opal_put_chars deals with partial writes because in OPALv1,
opal_console_write_buffer_space did not work correctly. That firmware
is not supported.

This reworks the opal_put_chars code to no longer deal with partial
writes by turning them into full writes. Partial write handling is still
supported in terms of what gets returned to the caller, but it may not
go to the console atomically. A warning message is printed in this
case.

This allows console flushing to be moved out of the opal_write_lock
spinlock. That could cause the lock to be held for long periods if the
console is busy (especially if it was being spammed by firmware),
which is dangerous because the lock is taken by xmon to debug the
system. Flushing outside the lock improves the situation a bit.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 86 +++++++++++++--------------
 1 file changed, 40 insertions(+), 46 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 6640fccbf30c..0ddb63226695 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -346,10 +346,10 @@ int opal_get_chars(uint32_t vtermno, char *buf, int count)
 
 int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 {
-	int written = 0;
-	__be64 olen;
-	s64 len, rc;
 	unsigned long flags;
+	int written;
+	__be64 olen;
+	s64 rc;
 
 	if (!opal.entry)
 		return -ENODEV;
@@ -357,62 +357,56 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 	/* We want put_chars to be atomic to avoid mangling of hvsi
 	 * packets. To do that, we first test for room and return
 	 * -EAGAIN if there isn't enough.
-	 *
-	 * Unfortunately, opal_console_write_buffer_space() doesn't
-	 * appear to work on opal v1, so we just assume there is
-	 * enough room and be done with it
 	 */
 	spin_lock_irqsave(&opal_write_lock, flags);
 	rc = opal_console_write_buffer_space(vtermno, &olen);
-	len = be64_to_cpu(olen);
-	if (rc || len < total_len) {
-		spin_unlock_irqrestore(&opal_write_lock, flags);
+	if (rc || be64_to_cpu(olen) < total_len) {
 		/* Closed -> drop characters */
 		if (rc)
-			return total_len;
-		opal_flush_console(vtermno);
-		return -EAGAIN;
+			written = total_len;
+		else
+			written = -EAGAIN;
+		goto out;
 	}
 
-	/* We still try to handle partial completions, though they
-	 * should no longer happen.
-	 */
-
-	while (total_len > 0) {
-		olen = cpu_to_be64(total_len);
-
-		rc = OPAL_BUSY;
-		while (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
-			rc = opal_console_write(vtermno, &olen, data);
-			if (rc == OPAL_BUSY_EVENT) {
-				mdelay(OPAL_BUSY_DELAY_MS);
-				opal_poll_events(NULL);
-			} else if (rc == OPAL_BUSY) {
-				mdelay(OPAL_BUSY_DELAY_MS);
-			}
-		}
-
-		len = be64_to_cpu(olen);
-
-		/* Closed or other error drop */
-		if (rc != OPAL_SUCCESS) {
-			written += total_len; /* drop remaining chars */
-			break;
+	/* Should not get a partial write here because space is available. */
+	olen = cpu_to_be64(total_len);
+	rc = opal_console_write(vtermno, &olen, data);
+	if (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
+		if (rc == OPAL_BUSY_EVENT) {
+			mdelay(OPAL_BUSY_DELAY_MS);
+			opal_poll_events(NULL);
+		} else if (rc == OPAL_BUSY_EVENT) {
+			mdelay(OPAL_BUSY_DELAY_MS);
 		}
+		written = -EAGAIN;
+		goto out;
+	}
 
-		total_len -= len;
-		data += len;
-		written += len;
+	/* Closed or other error drop */
+	if (rc != OPAL_SUCCESS) {
+		written = opal_error_code(rc);
+		goto out;
+	}
 
-		/* This is a bit nasty but we need that for the console to
-		 * flush when there aren't any interrupts. We will clean
-		 * things a bit later to limit that to synchronous path
-		 * such as the kernel console and xmon/udbg
-		 */
-		opal_flush_console(vtermno);
+	written = be64_to_cpu(olen);
+	if (written < total_len) {
+		/* Should not happen */
+		pr_warn("atomic console write returned partial len=%d written=%d\n", total_len, written);
+		if (!written)
+			written = -EAGAIN;
 	}
+
+out:
 	spin_unlock_irqrestore(&opal_write_lock, flags);
 
+	/* This is a bit nasty but we need that for the console to
+	 * flush when there aren't any interrupts. We will clean
+	 * things a bit later to limit that to synchronous path
+	 * such as the kernel console and xmon/udbg
+	 */
+	opal_flush_console(vtermno);
+
 	return written;
 }
 
-- 
2.17.0

[PATCH 07/15] powerpc/powernv: move opal console flushing to udbg

[Nicholas Piggin]

OPAL console writes do not have to synchronously flush firmware /
hardware buffers unless they are going through the udbg path.

Remove the unconditional flushing from opal_put_chars. Flush if
there was no space in the buffer as an optimisation (callers loop
waiting for success in that case). udbg flushing is moved to
udbg_opal_putc.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 12 +++++++-----
 drivers/tty/hvc/hvc_opal.c            |  5 +++++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 0ddb63226695..55d4b1983110 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -400,12 +400,14 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 out:
 	spin_unlock_irqrestore(&opal_write_lock, flags);
 
-	/* This is a bit nasty but we need that for the console to
-	 * flush when there aren't any interrupts. We will clean
-	 * things a bit later to limit that to synchronous path
-	 * such as the kernel console and xmon/udbg
+	/* In the -EAGAIN case, callers loop, so we have to flush the console
+	 * here in case they have interrupts off (and we don't want to wait
+	 * for async flushing if we can make immediate progress here). If
+	 * necessary the API could be made entirely non-flushing if the
+	 * callers had a ->flush API to use.
 	 */
-	opal_flush_console(vtermno);
+	if (written == -EAGAIN)
+		opal_flush_console(vtermno);
 
 	return written;
 }
diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 2ed07ca6389e..af122ad7f06d 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -275,6 +275,11 @@ static void udbg_opal_putc(char c)
 			count = hvc_opal_hvsi_put_chars(termno, &c, 1);
 			break;
 		}
+
+		/* This is needed for the cosole to flush
+		 * when there aren't any interrupts.
+		 */
+		opal_flush_console(termno);
 	} while(count == 0 || count == -EAGAIN);
 }
 
-- 
2.17.0

[PATCH 08/15] powerpc/powernv: implement opal_put_chars_atomic

[Nicholas Piggin]

The RAW console does not need writes to be atomic, so relax
opal_put_chars to be able to do partial writes, and implement an
_atomic variant which does not take a spinlock. This API is used
in xmon, so the less locking that is used, the better chance there
is that a crash can be debugged.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/opal.h       |  1 +
 arch/powerpc/platforms/powernv/opal.c | 37 +++++++++++++++++++--------
 drivers/tty/hvc/hvc_opal.c            | 18 +++++++++----
 3 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index bbff49fab0e5..5d7072411561 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -303,6 +303,7 @@ extern void opal_configure_cores(void);
 
 extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
 extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
+extern int opal_put_chars_atomic(uint32_t vtermno, const char *buf, int total_len);
 extern int opal_flush_console(uint32_t vtermno);
 
 extern void hvc_opal_init_early(void);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 55d4b1983110..bcdb90ada938 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -344,9 +344,9 @@ int opal_get_chars(uint32_t vtermno, char *buf, int count)
 	return 0;
 }
 
-int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
+static int __opal_put_chars(uint32_t vtermno, const char *data, int total_len, bool atomic)
 {
-	unsigned long flags;
+	unsigned long flags = 0 /* shut up gcc */;
 	int written;
 	__be64 olen;
 	s64 rc;
@@ -354,11 +354,8 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 	if (!opal.entry)
 		return -ENODEV;
 
-	/* We want put_chars to be atomic to avoid mangling of hvsi
-	 * packets. To do that, we first test for room and return
-	 * -EAGAIN if there isn't enough.
-	 */
-	spin_lock_irqsave(&opal_write_lock, flags);
+	if (atomic)
+		spin_lock_irqsave(&opal_write_lock, flags);
 	rc = opal_console_write_buffer_space(vtermno, &olen);
 	if (rc || be64_to_cpu(olen) < total_len) {
 		/* Closed -> drop characters */
@@ -391,14 +388,18 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 
 	written = be64_to_cpu(olen);
 	if (written < total_len) {
-		/* Should not happen */
-		pr_warn("atomic console write returned partial len=%d written=%d\n", total_len, written);
+		if (atomic) {
+			/* Should not happen */
+			pr_warn("atomic console write returned partial "
+				"len=%d written=%d\n", total_len, written);
+		}
 		if (!written)
 			written = -EAGAIN;
 	}
 
 out:
-	spin_unlock_irqrestore(&opal_write_lock, flags);
+	if (atomic)
+		spin_unlock_irqrestore(&opal_write_lock, flags);
 
 	/* In the -EAGAIN case, callers loop, so we have to flush the console
 	 * here in case they have interrupts off (and we don't want to wait
@@ -412,6 +413,22 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
 	return written;
 }
 
+int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
+{
+	return __opal_put_chars(vtermno, data, total_len, false);
+}
+
+/*
+ * opal_put_chars_atomic will not perform partial-writes. Data will be
+ * atomically written to the terminal or not at all. This is not strictly
+ * true at the moment because console space can race with OPAL's console
+ * writes.
+ */
+int opal_put_chars_atomic(uint32_t vtermno, const char *data, int total_len)
+{
+	return __opal_put_chars(vtermno, data, total_len, true);
+}
+
 int opal_flush_console(uint32_t vtermno)
 {
 	s64 rc;
diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index af122ad7f06d..0a72f98ee082 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -183,9 +183,15 @@ static int hvc_opal_probe(struct platform_device *dev)
 			return -ENOMEM;
 		pv->proto = proto;
 		hvc_opal_privs[termno] = pv;
-		if (proto == HV_PROTOCOL_HVSI)
-			hvsilib_init(&pv->hvsi, opal_get_chars, opal_put_chars,
+		if (proto == HV_PROTOCOL_HVSI) {
+			/*
+			 * We want put_chars to be atomic to avoid mangling of
+			 * hvsi packets.
+			 */
+			hvsilib_init(&pv->hvsi,
+				     opal_get_chars, opal_put_chars_atomic,
 				     termno, 0);
+		}
 
 		/* Instanciate now to establish a mapping index==vtermno */
 		hvc_instantiate(termno, termno, ops);
@@ -376,8 +382,9 @@ void __init hvc_opal_init_early(void)
 	else if (of_device_is_compatible(stdout_node,"ibm,opal-console-hvsi")) {
 		hvc_opal_boot_priv.proto = HV_PROTOCOL_HVSI;
 		ops = &hvc_opal_hvsi_ops;
-		hvsilib_init(&hvc_opal_boot_priv.hvsi, opal_get_chars,
-			     opal_put_chars, index, 1);
+		hvsilib_init(&hvc_opal_boot_priv.hvsi,
+			     opal_get_chars, opal_put_chars_atomic,
+			     index, 1);
 		/* HVSI, perform the handshake now */
 		hvsilib_establish(&hvc_opal_boot_priv.hvsi);
 		pr_devel("hvc_opal: Found HVSI console\n");
@@ -409,7 +416,8 @@ void __init udbg_init_debug_opal_hvsi(void)
 	hvc_opal_privs[index] = &hvc_opal_boot_priv;
 	hvc_opal_boot_termno = index;
 	udbg_init_opal_common();
-	hvsilib_init(&hvc_opal_boot_priv.hvsi, opal_get_chars, opal_put_chars,
+	hvsilib_init(&hvc_opal_boot_priv.hvsi,
+		     opal_get_chars, opal_put_chars_atomic,
 		     index, 1);
 	hvsilib_establish(&hvc_opal_boot_priv.hvsi);
 }
-- 
2.17.0

Re: [PATCH 08/15] powerpc/powernv: implement opal_put_chars_atomic

[Benjamin Herrenschmidt]

On Tue, 2018-05-01 at 00:55 +1000, Nicholas Piggin wrote:
> The RAW console does not need writes to be atomic, so relax
> opal_put_chars to be able to do partial writes, and implement an
> _atomic variant which does not take a spinlock. This API is used
> in xmon, so the less locking that is used, the better chance there
> is that a crash can be debugged.

Same comment I already had :-) "atomic" in Linux tends to mean
something else (ie, atomic context), so I'd rather have something
like opal_put_chars_sync() or such...

> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/opal.h       |  1 +
>  arch/powerpc/platforms/powernv/opal.c | 37 +++++++++++++++++++--------
>  drivers/tty/hvc/hvc_opal.c            | 18 +++++++++----
>  3 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index bbff49fab0e5..5d7072411561 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -303,6 +303,7 @@ extern void opal_configure_cores(void);
>  
>  extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
>  extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
> +extern int opal_put_chars_atomic(uint32_t vtermno, const char *buf, int total_len);
>  extern int opal_flush_console(uint32_t vtermno);
>  
>  extern void hvc_opal_init_early(void);
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 55d4b1983110..bcdb90ada938 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -344,9 +344,9 @@ int opal_get_chars(uint32_t vtermno, char *buf, int count)
>  	return 0;
>  }
>  
> -int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> +static int __opal_put_chars(uint32_t vtermno, const char *data, int total_len, bool atomic)
>  {
> -	unsigned long flags;
> +	unsigned long flags = 0 /* shut up gcc */;
>  	int written;
>  	__be64 olen;
>  	s64 rc;
> @@ -354,11 +354,8 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
>  	if (!opal.entry)
>  		return -ENODEV;
>  
> -	/* We want put_chars to be atomic to avoid mangling of hvsi
> -	 * packets. To do that, we first test for room and return
> -	 * -EAGAIN if there isn't enough.
> -	 */
> -	spin_lock_irqsave(&opal_write_lock, flags);
> +	if (atomic)
> +		spin_lock_irqsave(&opal_write_lock, flags);
>  	rc = opal_console_write_buffer_space(vtermno, &olen);
>  	if (rc || be64_to_cpu(olen) < total_len) {
>  		/* Closed -> drop characters */
> @@ -391,14 +388,18 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
>  
>  	written = be64_to_cpu(olen);
>  	if (written < total_len) {
> -		/* Should not happen */
> -		pr_warn("atomic console write returned partial len=%d written=%d\n", total_len, written);
> +		if (atomic) {
> +			/* Should not happen */
> +			pr_warn("atomic console write returned partial "
> +				"len=%d written=%d\n", total_len, written);
> +		}
>  		if (!written)
>  			written = -EAGAIN;
>  	}
>  
>  out:
> -	spin_unlock_irqrestore(&opal_write_lock, flags);
> +	if (atomic)
> +		spin_unlock_irqrestore(&opal_write_lock, flags);
>  
>  	/* In the -EAGAIN case, callers loop, so we have to flush the console
>  	 * here in case they have interrupts off (and we don't want to wait
> @@ -412,6 +413,22 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
>  	return written;
>  }
>  
> +int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> +{
> +	return __opal_put_chars(vtermno, data, total_len, false);
> +}
> +
> +/*
> + * opal_put_chars_atomic will not perform partial-writes. Data will be
> + * atomically written to the terminal or not at all. This is not strictly
> + * true at the moment because console space can race with OPAL's console
> + * writes.
> + */
> +int opal_put_chars_atomic(uint32_t vtermno, const char *data, int total_len)
> +{
> +	return __opal_put_chars(vtermno, data, total_len, true);
> +}
> +
>  int opal_flush_console(uint32_t vtermno)
>  {
>  	s64 rc;
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index af122ad7f06d..0a72f98ee082 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -183,9 +183,15 @@ static int hvc_opal_probe(struct platform_device *dev)
>  			return -ENOMEM;
>  		pv->proto = proto;
>  		hvc_opal_privs[termno] = pv;
> -		if (proto == HV_PROTOCOL_HVSI)
> -			hvsilib_init(&pv->hvsi, opal_get_chars, opal_put_chars,
> +		if (proto == HV_PROTOCOL_HVSI) {
> +			/*
> +			 * We want put_chars to be atomic to avoid mangling of
> +			 * hvsi packets.
> +			 */
> +			hvsilib_init(&pv->hvsi,
> +				     opal_get_chars, opal_put_chars_atomic,
>  				     termno, 0);
> +		}
>  
>  		/* Instanciate now to establish a mapping index==vtermno */
>  		hvc_instantiate(termno, termno, ops);
> @@ -376,8 +382,9 @@ void __init hvc_opal_init_early(void)
>  	else if (of_device_is_compatible(stdout_node,"ibm,opal-console-hvsi")) {
>  		hvc_opal_boot_priv.proto = HV_PROTOCOL_HVSI;
>  		ops = &hvc_opal_hvsi_ops;
> -		hvsilib_init(&hvc_opal_boot_priv.hvsi, opal_get_chars,
> -			     opal_put_chars, index, 1);
> +		hvsilib_init(&hvc_opal_boot_priv.hvsi,
> +			     opal_get_chars, opal_put_chars_atomic,
> +			     index, 1);
>  		/* HVSI, perform the handshake now */
>  		hvsilib_establish(&hvc_opal_boot_priv.hvsi);
>  		pr_devel("hvc_opal: Found HVSI console\n");
> @@ -409,7 +416,8 @@ void __init udbg_init_debug_opal_hvsi(void)
>  	hvc_opal_privs[index] = &hvc_opal_boot_priv;
>  	hvc_opal_boot_termno = index;
>  	udbg_init_opal_common();
> -	hvsilib_init(&hvc_opal_boot_priv.hvsi, opal_get_chars, opal_put_chars,
> +	hvsilib_init(&hvc_opal_boot_priv.hvsi,
> +		     opal_get_chars, opal_put_chars_atomic,
>  		     index, 1);
>  	hvsilib_establish(&hvc_opal_boot_priv.hvsi);
>  }

Re: [PATCH 08/15] powerpc/powernv: implement opal_put_chars_atomic

[Nicholas Piggin]

On Tue, 01 May 2018 19:48:58 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Tue, 2018-05-01 at 00:55 +1000, Nicholas Piggin wrote:
> > The RAW console does not need writes to be atomic, so relax
> > opal_put_chars to be able to do partial writes, and implement an
> > _atomic variant which does not take a spinlock. This API is used
> > in xmon, so the less locking that is used, the better chance there
> > is that a crash can be debugged.  
> 
> Same comment I already had :-) "atomic" in Linux tends to mean
> something else (ie, atomic context), so I'd rather have something
> like opal_put_chars_sync() or such...

Oh yeah, I didn't ignore you, just... I thought atomic was okay.
atomic *also* tends to mean happens atomically. I think the in
atomic context meaning actually tends to be inatomic.

Sync I actually thought could be more easily confused with
synchronous vs asynchronous here.

> 
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/include/asm/opal.h       |  1 +
> >  arch/powerpc/platforms/powernv/opal.c | 37 +++++++++++++++++++--------
> >  drivers/tty/hvc/hvc_opal.c            | 18 +++++++++----
> >  3 files changed, 41 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> > index bbff49fab0e5..5d7072411561 100644
> > --- a/arch/powerpc/include/asm/opal.h
> > +++ b/arch/powerpc/include/asm/opal.h
> > @@ -303,6 +303,7 @@ extern void opal_configure_cores(void);
> >  
> >  extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
> >  extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
> > +extern int opal_put_chars_atomic(uint32_t vtermno, const char *buf, int total_len);
> >  extern int opal_flush_console(uint32_t vtermno);
> >  
> >  extern void hvc_opal_init_early(void);
> > diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> > index 55d4b1983110..bcdb90ada938 100644
> > --- a/arch/powerpc/platforms/powernv/opal.c
> > +++ b/arch/powerpc/platforms/powernv/opal.c
> > @@ -344,9 +344,9 @@ int opal_get_chars(uint32_t vtermno, char *buf, int count)
> >  	return 0;
> >  }
> >  
> > -int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> > +static int __opal_put_chars(uint32_t vtermno, const char *data, int total_len, bool atomic)
> >  {
> > -	unsigned long flags;
> > +	unsigned long flags = 0 /* shut up gcc */;
> >  	int written;
> >  	__be64 olen;
> >  	s64 rc;
> > @@ -354,11 +354,8 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> >  	if (!opal.entry)
> >  		return -ENODEV;
> >  
> > -	/* We want put_chars to be atomic to avoid mangling of hvsi
> > -	 * packets. To do that, we first test for room and return
> > -	 * -EAGAIN if there isn't enough.
> > -	 */
> > -	spin_lock_irqsave(&opal_write_lock, flags);
> > +	if (atomic)
> > +		spin_lock_irqsave(&opal_write_lock, flags);
> >  	rc = opal_console_write_buffer_space(vtermno, &olen);
> >  	if (rc || be64_to_cpu(olen) < total_len) {
> >  		/* Closed -> drop characters */
> > @@ -391,14 +388,18 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> >  
> >  	written = be64_to_cpu(olen);
> >  	if (written < total_len) {
> > -		/* Should not happen */
> > -		pr_warn("atomic console write returned partial len=%d written=%d\n", total_len, written);
> > +		if (atomic) {
> > +			/* Should not happen */
> > +			pr_warn("atomic console write returned partial "
> > +				"len=%d written=%d\n", total_len, written);
> > +		}
> >  		if (!written)
> >  			written = -EAGAIN;
> >  	}
> >  
> >  out:
> > -	spin_unlock_irqrestore(&opal_write_lock, flags);
> > +	if (atomic)
> > +		spin_unlock_irqrestore(&opal_write_lock, flags);
> >  
> >  	/* In the -EAGAIN case, callers loop, so we have to flush the console
> >  	 * here in case they have interrupts off (and we don't want to wait
> > @@ -412,6 +413,22 @@ int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> >  	return written;
> >  }
> >  
> > +int opal_put_chars(uint32_t vtermno, const char *data, int total_len)
> > +{
> > +	return __opal_put_chars(vtermno, data, total_len, false);
> > +}
> > +
> > +/*
> > + * opal_put_chars_atomic will not perform partial-writes. Data will be
> > + * atomically written to the terminal or not at all. This is not strictly
> > + * true at the moment because console space can race with OPAL's console
> > + * writes.
> > + */
> > +int opal_put_chars_atomic(uint32_t vtermno, const char *data, int total_len)
> > +{
> > +	return __opal_put_chars(vtermno, data, total_len, true);
> > +}
> > +
> >  int opal_flush_console(uint32_t vtermno)
> >  {
> >  	s64 rc;
> > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> > index af122ad7f06d..0a72f98ee082 100644
> > --- a/drivers/tty/hvc/hvc_opal.c
> > +++ b/drivers/tty/hvc/hvc_opal.c
> > @@ -183,9 +183,15 @@ static int hvc_opal_probe(struct platform_device *dev)
> >  			return -ENOMEM;
> >  		pv->proto = proto;
> >  		hvc_opal_privs[termno] = pv;
> > -		if (proto == HV_PROTOCOL_HVSI)
> > -			hvsilib_init(&pv->hvsi, opal_get_chars, opal_put_chars,
> > +		if (proto == HV_PROTOCOL_HVSI) {
> > +			/*
> > +			 * We want put_chars to be atomic to avoid mangling of
> > +			 * hvsi packets.
> > +			 */
> > +			hvsilib_init(&pv->hvsi,
> > +				     opal_get_chars, opal_put_chars_atomic,
> >  				     termno, 0);
> > +		}
> >  
> >  		/* Instanciate now to establish a mapping index==vtermno */
> >  		hvc_instantiate(termno, termno, ops);
> > @@ -376,8 +382,9 @@ void __init hvc_opal_init_early(void)
> >  	else if (of_device_is_compatible(stdout_node,"ibm,opal-console-hvsi")) {
> >  		hvc_opal_boot_priv.proto = HV_PROTOCOL_HVSI;
> >  		ops = &hvc_opal_hvsi_ops;
> > -		hvsilib_init(&hvc_opal_boot_priv.hvsi, opal_get_chars,
> > -			     opal_put_chars, index, 1);
> > +		hvsilib_init(&hvc_opal_boot_priv.hvsi,
> > +			     opal_get_chars, opal_put_chars_atomic,
> > +			     index, 1);
> >  		/* HVSI, perform the handshake now */
> >  		hvsilib_establish(&hvc_opal_boot_priv.hvsi);
> >  		pr_devel("hvc_opal: Found HVSI console\n");
> > @@ -409,7 +416,8 @@ void __init udbg_init_debug_opal_hvsi(void)
> >  	hvc_opal_privs[index] = &hvc_opal_boot_priv;
> >  	hvc_opal_boot_termno = index;
> >  	udbg_init_opal_common();
> > -	hvsilib_init(&hvc_opal_boot_priv.hvsi, opal_get_chars, opal_put_chars,
> > +	hvsilib_init(&hvc_opal_boot_priv.hvsi,
> > +		     opal_get_chars, opal_put_chars_atomic,
> >  		     index, 1);
> >  	hvsilib_establish(&hvc_opal_boot_priv.hvsi);
> >  }  

Re: [PATCH 08/15] powerpc/powernv: implement opal_put_chars_atomic

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> On Tue, 01 May 2018 19:48:58 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> On Tue, 2018-05-01 at 00:55 +1000, Nicholas Piggin wrote:
>> > The RAW console does not need writes to be atomic, so relax
>> > opal_put_chars to be able to do partial writes, and implement an
>> > _atomic variant which does not take a spinlock. This API is used
>> > in xmon, so the less locking that is used, the better chance there
>> > is that a crash can be debugged.  
>> 
>> Same comment I already had :-) "atomic" in Linux tends to mean
>> something else (ie, atomic context), so I'd rather have something
>> like opal_put_chars_sync() or such...
>
> Oh yeah, I didn't ignore you, just... I thought atomic was okay.
> atomic *also* tends to mean happens atomically. I think the in
> atomic context meaning actually tends to be inatomic.
>
> Sync I actually thought could be more easily confused with
> synchronous vs asynchronous here.

I think we probably want opal_put_chars() to stay as it is.

And then add a variant for the call (just xmon?) that want lock free
behaviour.

opal_put_chars_unlocked() or something?

cheers

Re: [PATCH 08/15] powerpc/powernv: implement opal_put_chars_atomic

[Nicholas Piggin]

On Mon, 07 May 2018 20:35:42 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > On Tue, 01 May 2018 19:48:58 +1000
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >  
> >> On Tue, 2018-05-01 at 00:55 +1000, Nicholas Piggin wrote:  
> >> > The RAW console does not need writes to be atomic, so relax
> >> > opal_put_chars to be able to do partial writes, and implement an
> >> > _atomic variant which does not take a spinlock. This API is used
> >> > in xmon, so the less locking that is used, the better chance there
> >> > is that a crash can be debugged.    
> >> 
> >> Same comment I already had :-) "atomic" in Linux tends to mean
> >> something else (ie, atomic context), so I'd rather have something
> >> like opal_put_chars_sync() or such...  
> >
> > Oh yeah, I didn't ignore you, just... I thought atomic was okay.
> > atomic *also* tends to mean happens atomically. I think the in
> > atomic context meaning actually tends to be inatomic.
> >
> > Sync I actually thought could be more easily confused with
> > synchronous vs asynchronous here.  
> 
> I think we probably want opal_put_chars() to stay as it is.
> 
> And then add a variant for the call (just xmon?) that want lock free
> behaviour.

No it's not the lock which is important here, it is whether the
message goes to the console atomically versus other writes. The
raw console does not require this, only one which sends some
control characters, which is the hvterm-protocol compatible variant
of the vio console, and I think FSP console.

BMC consoles for example always use raw.

> opal_put_chars_unlocked() or something?

I prefer the _atomic as the special case. Ordinarily we don't have
a special requirement, but with the control characters then we do.

Thanks,
Nick

[PATCH 09/15] tty: hvc: remove unexplained "just in case" spin delay

[Nicholas Piggin]

This delay was in the very first OPAL console commit 6.5 years ago,
and came from the vio hvc driver. The firmware console has hardened
sufficiently to remove it.

Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/tty/hvc/hvc_opal.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 0a72f98ee082..786e76a1e06d 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -313,14 +313,8 @@ static int udbg_opal_getc(void)
 	int ch;
 	for (;;) {
 		ch = udbg_opal_getc_poll();
-		if (ch == -1) {
-			/* This shouldn't be needed...but... */
-			volatile unsigned long delay;
-			for (delay=0; delay < 2000000; delay++)
-				;
-		} else {
+		if (ch != -1)
 			return ch;
-		}
 	}
 }
 
-- 
2.17.0

[PATCH 10/15] tty: hvc: use mutex instead of spinlock for hvc_structs lock

[Nicholas Piggin]

This allows hvc operations to sleep under the lock.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/tty/hvc/hvc_console.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 7709fcc707f4..fddb63322c67 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -73,7 +73,7 @@ static LIST_HEAD(hvc_structs);
  * Protect the list of hvc_struct instances from inserts and removals during
  * list traversal.
  */
-static DEFINE_SPINLOCK(hvc_structs_lock);
+static DEFINE_MUTEX(hvc_structs_mutex);
 
 /*
  * This value is used to assign a tty->index value to a hvc_struct based
@@ -83,7 +83,7 @@ static DEFINE_SPINLOCK(hvc_structs_lock);
 static int last_hvc = -1;
 
 /*
- * Do not call this function with either the hvc_structs_lock or the hvc_struct
+ * Do not call this function with either the hvc_structs_mutex or the hvc_struct
  * lock held.  If successful, this function increments the kref reference
  * count against the target hvc_struct so it should be released when finished.
  */
@@ -92,25 +92,24 @@ static struct hvc_struct *hvc_get_by_index(int index)
 	struct hvc_struct *hp;
 	unsigned long flags;
 
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_mutex);
 
 	list_for_each_entry(hp, &hvc_structs, next) {
 		spin_lock_irqsave(&hp->lock, flags);
 		if (hp->index == index) {
 			tty_port_get(&hp->port);
 			spin_unlock_irqrestore(&hp->lock, flags);
-			spin_unlock(&hvc_structs_lock);
+			mutex_unlock(&hvc_structs_mutex);
 			return hp;
 		}
 		spin_unlock_irqrestore(&hp->lock, flags);
 	}
 	hp = NULL;
+	mutex_unlock(&hvc_structs_mutex);
 
-	spin_unlock(&hvc_structs_lock);
 	return hp;
 }
 
-
 /*
  * Initial console vtermnos for console API usage prior to full console
  * initialization.  Any vty adapter outside this range will not have usable
@@ -224,13 +223,13 @@ static void hvc_port_destruct(struct tty_port *port)
 	struct hvc_struct *hp = container_of(port, struct hvc_struct, port);
 	unsigned long flags;
 
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_mutex);
 
 	spin_lock_irqsave(&hp->lock, flags);
 	list_del(&(hp->next));
 	spin_unlock_irqrestore(&hp->lock, flags);
 
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_mutex);
 
 	kfree(hp);
 }
@@ -733,11 +732,11 @@ static int khvcd(void *unused)
 		try_to_freeze();
 		wmb();
 		if (!cpus_are_in_xmon()) {
-			spin_lock(&hvc_structs_lock);
+			mutex_lock(&hvc_structs_mutex);
 			list_for_each_entry(hp, &hvc_structs, next) {
 				poll_mask |= hvc_poll(hp);
 			}
-			spin_unlock(&hvc_structs_lock);
+			mutex_unlock(&hvc_structs_mutex);
 		} else
 			poll_mask |= HVC_POLL_READ;
 		if (hvc_kicked)
@@ -871,7 +870,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 
 	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
 	spin_lock_init(&hp->lock);
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_mutex);
 
 	/*
 	 * find index to use:
@@ -891,7 +890,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 	vtermnos[i] = vtermno;
 
 	list_add_tail(&(hp->next), &hvc_structs);
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_mutex);
 
 	/* check if we need to re-register the kernel console */
 	hvc_check_console(i);
-- 
2.17.0

Re: [10/15] tty: hvc: use mutex instead of spinlock for hvc_structs lock

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:53 UTC, Nicholas Piggin wrote:
> This allows hvc operations to sleep under the lock.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/a9bf5c8a271b9a954709b7ada1bd25

cheers

[PATCH 11/15] tty: hvc: hvc_poll break hv read loop

[Nicholas Piggin]

Avoid looping with the spinlock held while there is read data
being returned from the hv driver. Instead note if the entire
size returned by tty_buffer_request_room was read, and request
another read poll.

This limits the critical section lengths, and provides more
even service to other consoles in case there is a pathological
condition.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/tty/hvc/hvc_console.c | 88 ++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index fddb63322c67..745ac220fce8 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -592,7 +592,7 @@ static u32 timeout = MIN_TIMEOUT;
 int hvc_poll(struct hvc_struct *hp)
 {
 	struct tty_struct *tty;
-	int i, n, poll_mask = 0;
+	int i, n, count, poll_mask = 0;
 	char buf[N_INBUF] __ALIGNED__;
 	unsigned long flags;
 	int read_total = 0;
@@ -618,7 +618,7 @@ int hvc_poll(struct hvc_struct *hp)
 
 	/* Now check if we can get data (are we throttled ?) */
 	if (tty_throttled(tty))
-		goto throttled;
+		goto out;
 
 	/* If we aren't notifier driven and aren't throttled, we always
 	 * request a reschedule
@@ -627,56 +627,58 @@ int hvc_poll(struct hvc_struct *hp)
 		poll_mask |= HVC_POLL_READ;
 
 	/* Read data if any */
-	for (;;) {
-		int count = tty_buffer_request_room(&hp->port, N_INBUF);
 
-		/* If flip is full, just reschedule a later read */
-		if (count == 0) {
+	count = tty_buffer_request_room(&hp->port, N_INBUF);
+
+	/* If flip is full, just reschedule a later read */
+	if (count == 0) {
+		poll_mask |= HVC_POLL_READ;
+		goto out;
+	}
+
+	n = hp->ops->get_chars(hp->vtermno, buf, count);
+	if (n <= 0) {
+		/* Hangup the tty when disconnected from host */
+		if (n == -EPIPE) {
+			spin_unlock_irqrestore(&hp->lock, flags);
+			tty_hangup(tty);
+			spin_lock_irqsave(&hp->lock, flags);
+		} else if ( n == -EAGAIN ) {
+			/*
+			 * Some back-ends can only ensure a certain min
+			 * num of bytes read, which may be > 'count'.
+			 * Let the tty clear the flip buff to make room.
+			 */
 			poll_mask |= HVC_POLL_READ;
-			break;
 		}
+		goto out;
+	}
 
-		n = hp->ops->get_chars(hp->vtermno, buf, count);
-		if (n <= 0) {
-			/* Hangup the tty when disconnected from host */
-			if (n == -EPIPE) {
-				spin_unlock_irqrestore(&hp->lock, flags);
-				tty_hangup(tty);
-				spin_lock_irqsave(&hp->lock, flags);
-			} else if ( n == -EAGAIN ) {
-				/*
-				 * Some back-ends can only ensure a certain min
-				 * num of bytes read, which may be > 'count'.
-				 * Let the tty clear the flip buff to make room.
-				 */
-				poll_mask |= HVC_POLL_READ;
-			}
-			break;
-		}
-		for (i = 0; i < n; ++i) {
+	for (i = 0; i < n; ++i) {
 #ifdef CONFIG_MAGIC_SYSRQ
-			if (hp->index == hvc_console.index) {
-				/* Handle the SysRq Hack */
-				/* XXX should support a sequence */
-				if (buf[i] == '\x0f') {	/* ^O */
-					/* if ^O is pressed again, reset
-					 * sysrq_pressed and flip ^O char */
-					sysrq_pressed = !sysrq_pressed;
-					if (sysrq_pressed)
-						continue;
-				} else if (sysrq_pressed) {
-					handle_sysrq(buf[i]);
-					sysrq_pressed = 0;
+		if (hp->index == hvc_console.index) {
+			/* Handle the SysRq Hack */
+			/* XXX should support a sequence */
+			if (buf[i] == '\x0f') {	/* ^O */
+				/* if ^O is pressed again, reset
+				 * sysrq_pressed and flip ^O char */
+				sysrq_pressed = !sysrq_pressed;
+				if (sysrq_pressed)
 					continue;
-				}
+			} else if (sysrq_pressed) {
+				handle_sysrq(buf[i]);
+				sysrq_pressed = 0;
+				continue;
 			}
-#endif /* CONFIG_MAGIC_SYSRQ */
-			tty_insert_flip_char(&hp->port, buf[i], 0);
 		}
-
-		read_total += n;
+#endif /* CONFIG_MAGIC_SYSRQ */
+		tty_insert_flip_char(&hp->port, buf[i], 0);
 	}
- throttled:
+	if (n == count)
+		poll_mask |= HVC_POLL_READ;
+	read_total = n;
+
+ out:
 	/* Wakeup write queue if necessary */
 	if (hp->do_wakeup) {
 		hp->do_wakeup = 0;
-- 
2.17.0

Re: [11/15] tty: hvc: hvc_poll break hv read loop

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:54 UTC, Nicholas Piggin wrote:
> Avoid looping with the spinlock held while there is read data
> being returned from the hv driver. Instead note if the entire
> size returned by tty_buffer_request_room was read, and request
> another read poll.
> 
> This limits the critical section lengths, and provides more
> even service to other consoles in case there is a pathological
> condition.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/ec97eaad1383ab2500fcf9a07ade60

cheers

[PATCH 12/15] tty: hvc: hvc_poll may sleep

[Nicholas Piggin]

Introduce points where hvc_poll drops the lock, enables interrupts,
and reschedules.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/tty/hvc/hvc_console.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 745ac220fce8..2abfc0b15fbb 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -589,7 +589,7 @@ static u32 timeout = MIN_TIMEOUT;
 #define HVC_POLL_READ	0x00000001
 #define HVC_POLL_WRITE	0x00000002
 
-int hvc_poll(struct hvc_struct *hp)
+static int __hvc_poll(struct hvc_struct *hp, bool may_sleep)
 {
 	struct tty_struct *tty;
 	int i, n, count, poll_mask = 0;
@@ -611,6 +611,12 @@ int hvc_poll(struct hvc_struct *hp)
 		timeout = (written_total) ? 0 : MIN_TIMEOUT;
 	}
 
+	if (may_sleep) {
+		spin_unlock_irqrestore(&hp->lock, flags);
+		cond_resched();
+		spin_lock_irqsave(&hp->lock, flags);
+	}
+
 	/* No tty attached, just skip */
 	tty = tty_port_tty_get(&hp->port);
 	if (tty == NULL)
@@ -698,6 +704,11 @@ int hvc_poll(struct hvc_struct *hp)
 
 	return poll_mask;
 }
+
+int hvc_poll(struct hvc_struct *hp)
+{
+	return __hvc_poll(hp, false);
+}
 EXPORT_SYMBOL_GPL(hvc_poll);
 
 /**
@@ -736,7 +747,8 @@ static int khvcd(void *unused)
 		if (!cpus_are_in_xmon()) {
 			mutex_lock(&hvc_structs_mutex);
 			list_for_each_entry(hp, &hvc_structs, next) {
-				poll_mask |= hvc_poll(hp);
+				poll_mask |= __hvc_poll(hp, true);
+				cond_resched();
 			}
 			mutex_unlock(&hvc_structs_mutex);
 		} else
-- 
2.17.0

Re: [12/15] tty: hvc: hvc_poll may sleep

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:55 UTC, Nicholas Piggin wrote:
> Introduce points where hvc_poll drops the lock, enables interrupts,
> and reschedules.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/cfb5946b55f1dfd19e042feae1fbff

cheers

[PATCH 13/15] tty: hvc: hvc_write may sleep

[Nicholas Piggin]

Rework the hvc_write loop to drop and re-take the spinlock on each
iteration, add a cond_resched. Don't bother with an initial hvc_push
initially, which makes the logic simpler -- just do a hvc_push on
each time around the loop.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/tty/hvc/hvc_console.c | 36 ++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 2abfc0b15fbb..6131d5084c42 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -493,23 +493,29 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 	if (hp->port.count <= 0)
 		return -EIO;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	while (count > 0) {
+		spin_lock_irqsave(&hp->lock, flags);
 
-	/* Push pending writes */
-	if (hp->n_outbuf > 0)
-		hvc_push(hp);
-
-	while (count > 0 && (rsize = hp->outbuf_size - hp->n_outbuf) > 0) {
-		if (rsize > count)
-			rsize = count;
-		memcpy(hp->outbuf + hp->n_outbuf, buf, rsize);
-		count -= rsize;
-		buf += rsize;
-		hp->n_outbuf += rsize;
-		written += rsize;
-		hvc_push(hp);
+		rsize = hp->outbuf_size - hp->n_outbuf;
+
+		if (rsize) {
+			if (rsize > count)
+				rsize = count;
+			memcpy(hp->outbuf + hp->n_outbuf, buf, rsize);
+			count -= rsize;
+			buf += rsize;
+			hp->n_outbuf += rsize;
+			written += rsize;
+		}
+
+		if (hp->n_outbuf > 0)
+			hvc_push(hp);
+
+		spin_unlock_irqrestore(&hp->lock, flags);
+
+		if (count)
+			cond_resched();
 	}
-	spin_unlock_irqrestore(&hp->lock, flags);
 
 	/*
 	 * Racy, but harmless, kick thread if there is still pending data.
-- 
2.17.0

Re: [13/15] tty: hvc: hvc_write may sleep

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:56 UTC, Nicholas Piggin wrote:
> Rework the hvc_write loop to drop and re-take the spinlock on each
> iteration, add a cond_resched. Don't bother with an initial hvc_push
> initially, which makes the logic simpler -- just do a hvc_push on
> each time around the loop.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/550ddadcc7580ec2a6c22d4ed04291

cheers

[PATCH 14/15] tty: hvc: introduce the hv_ops.flush operation for hvc drivers

[Nicholas Piggin]

Use .flush to wait for drivers to flush their console outside of
the spinlock, to reduce lock/irq latencies.

Flush the hvc console driver after each write, which can help
messages make it out to the console after a crash.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/tty/hvc/hvc_console.c | 35 +++++++++++++++++++++++++++++++++--
 drivers/tty/hvc/hvc_console.h |  1 +
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 6131d5084c42..5414c4a87bea 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -110,6 +110,29 @@ static struct hvc_struct *hvc_get_by_index(int index)
 	return hp;
 }
 
+static int __hvc_flush(const struct hv_ops *ops, uint32_t vtermno, bool wait)
+{
+	if (wait)
+		might_sleep();
+
+	if (ops->flush)
+		return ops->flush(vtermno, wait);
+	return 0;
+}
+
+static int hvc_console_flush(const struct hv_ops *ops, uint32_t vtermno)
+{
+	return __hvc_flush(ops, vtermno, false);
+}
+
+/*
+ * Wait for the console to flush before writing more to it. This sleeps.
+ */
+static int hvc_flush(struct hvc_struct *hp)
+{
+	return __hvc_flush(hp->ops, hp->vtermno, true);
+}
+
 /*
  * Initial console vtermnos for console API usage prior to full console
  * initialization.  Any vty adapter outside this range will not have usable
@@ -155,8 +178,12 @@ static void hvc_console_print(struct console *co, const char *b,
 			if (r <= 0) {
 				/* throw away characters on error
 				 * but spin in case of -EAGAIN */
-				if (r != -EAGAIN)
+				if (r != -EAGAIN) {
 					i = 0;
+				} else {
+					hvc_console_flush(cons_ops[index],
+						      vtermnos[index]);
+				}
 			} else if (r > 0) {
 				i -= r;
 				if (i > 0)
@@ -164,6 +191,7 @@ static void hvc_console_print(struct console *co, const char *b,
 			}
 		}
 	}
+	hvc_console_flush(cons_ops[index], vtermnos[index]);
 }
 
 static struct tty_driver *hvc_console_device(struct console *c, int *index)
@@ -513,8 +541,11 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 
 		spin_unlock_irqrestore(&hp->lock, flags);
 
-		if (count)
+		if (count) {
+			if (hp->n_outbuf > 0)
+				hvc_flush(hp);
 			cond_resched();
+		}
 	}
 
 	/*
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index ea63090e013f..e9319954c832 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -54,6 +54,7 @@ struct hvc_struct {
 struct hv_ops {
 	int (*get_chars)(uint32_t vtermno, char *buf, int count);
 	int (*put_chars)(uint32_t vtermno, const char *buf, int count);
+	int (*flush)(uint32_t vtermno, bool wait);
 
 	/* Callbacks for notification. Called in open, close and hangup */
 	int (*notifier_add)(struct hvc_struct *hp, int irq);
-- 
2.17.0

Re: [14/15] tty: hvc: introduce the hv_ops.flush operation for hvc drivers

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:57 UTC, Nicholas Piggin wrote:
> Use .flush to wait for drivers to flush their console outside of
> the spinlock, to reduce lock/irq latencies.
> 
> Flush the hvc console driver after each write, which can help
> messages make it out to the console after a crash.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/9f65b81f36e31563c5a5e4df3b3b8b

cheers

[PATCH 15/15] powerpc/powernv: provide a console flush operation for opal hvc driver

[Nicholas Piggin]

Provide the flush hv_op for the opal hvc driver. This will flush the
firmware console buffers without spinning with interrupts disabled.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/opal.h       |  1 +
 arch/powerpc/platforms/powernv/opal.c | 83 +++++++++++++++++----------
 drivers/tty/hvc/hvc_opal.c            |  2 +
 3 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 5d7072411561..a426d2caaf9f 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -304,6 +304,7 @@ extern void opal_configure_cores(void);
 extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
 extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
 extern int opal_put_chars_atomic(uint32_t vtermno, const char *buf, int total_len);
+extern int opal_flush_chars(uint32_t vtermno, bool wait);
 extern int opal_flush_console(uint32_t vtermno);
 
 extern void hvc_opal_init_early(void);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index bcdb90ada938..048bc09afc83 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -370,12 +370,8 @@ static int __opal_put_chars(uint32_t vtermno, const char *data, int total_len, b
 	olen = cpu_to_be64(total_len);
 	rc = opal_console_write(vtermno, &olen, data);
 	if (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
-		if (rc == OPAL_BUSY_EVENT) {
-			mdelay(OPAL_BUSY_DELAY_MS);
+		if (rc == OPAL_BUSY_EVENT)
 			opal_poll_events(NULL);
-		} else if (rc == OPAL_BUSY_EVENT) {
-			mdelay(OPAL_BUSY_DELAY_MS);
-		}
 		written = -EAGAIN;
 		goto out;
 	}
@@ -401,15 +397,6 @@ static int __opal_put_chars(uint32_t vtermno, const char *data, int total_len, b
 	if (atomic)
 		spin_unlock_irqrestore(&opal_write_lock, flags);
 
-	/* In the -EAGAIN case, callers loop, so we have to flush the console
-	 * here in case they have interrupts off (and we don't want to wait
-	 * for async flushing if we can make immediate progress here). If
-	 * necessary the API could be made entirely non-flushing if the
-	 * callers had a ->flush API to use.
-	 */
-	if (written == -EAGAIN)
-		opal_flush_console(vtermno);
-
 	return written;
 }
 
@@ -429,40 +416,74 @@ int opal_put_chars_atomic(uint32_t vtermno, const char *data, int total_len)
 	return __opal_put_chars(vtermno, data, total_len, true);
 }
 
-int opal_flush_console(uint32_t vtermno)
+static s64 __opal_flush_console(uint32_t vtermno)
 {
 	s64 rc;
 
 	if (!opal_check_token(OPAL_CONSOLE_FLUSH)) {
 		__be64 evt;
 
-		WARN_ONCE(1, "opal: OPAL_CONSOLE_FLUSH missing.\n");
 		/*
 		 * If OPAL_CONSOLE_FLUSH is not implemented in the firmware,
 		 * the console can still be flushed by calling the polling
 		 * function while it has OPAL_EVENT_CONSOLE_OUTPUT events.
 		 */
-		do {
-			opal_poll_events(&evt);
-		} while (be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT);
+		WARN_ONCE(1, "opal: OPAL_CONSOLE_FLUSH missing.\n");
+
+		opal_poll_events(&evt);
+		if (!(be64_to_cpu(evt) & OPAL_EVENT_CONSOLE_OUTPUT))
+			return OPAL_SUCCESS;
+		return OPAL_BUSY;
 
-		return OPAL_SUCCESS;
+	} else {
+		rc = opal_console_flush(vtermno);
+		if (rc == OPAL_BUSY_EVENT) {
+			opal_poll_events(NULL);
+			rc = OPAL_BUSY;
+		}
+		return rc;
 	}
 
-	do  {
-		rc = OPAL_BUSY;
-		while (rc == OPAL_BUSY || rc == OPAL_BUSY_EVENT) {
-			rc = opal_console_flush(vtermno);
-			if (rc == OPAL_BUSY_EVENT) {
-				mdelay(OPAL_BUSY_DELAY_MS);
-				opal_poll_events(NULL);
-			} else if (rc == OPAL_BUSY) {
-				mdelay(OPAL_BUSY_DELAY_MS);
+}
+
+/*
+ * opal_flush_console spins until the console is flushed
+ */
+int opal_flush_console(uint32_t vtermno)
+{
+	for (;;) {
+		s64 rc = __opal_flush_console(vtermno);
+
+		if (rc == OPAL_BUSY || rc == OPAL_PARTIAL) {
+			mdelay(1);
+			continue;
+		}
+
+		return opal_error_code(rc);
+	}
+}
+
+/*
+ * opal_flush_chars is an hvc interface that sleeps until the console is
+ * flushed if wait, otherwise it will return -EBUSY if the console has data,
+ * -EAGAIN if it has data and some of it was flushed.
+ */
+int opal_flush_chars(uint32_t vtermno, bool wait)
+{
+	for (;;) {
+		s64 rc = __opal_flush_console(vtermno);
+
+		if (rc == OPAL_BUSY || rc == OPAL_PARTIAL) {
+			if (wait) {
+				msleep(OPAL_BUSY_DELAY_MS);
+				continue;
 			}
+			if (rc == OPAL_PARTIAL)
+				return -EAGAIN;
 		}
-	} while (rc == OPAL_PARTIAL); /* More to flush */
 
-	return opal_error_code(rc);
+		return opal_error_code(rc);
+	}
 }
 
 static int opal_recover_mce(struct pt_regs *regs,
diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 786e76a1e06d..fd306033f8d4 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -52,6 +52,7 @@ static u32 hvc_opal_boot_termno;
 static const struct hv_ops hvc_opal_raw_ops = {
 	.get_chars = opal_get_chars,
 	.put_chars = opal_put_chars,
+	.flush = opal_flush_chars,
 	.notifier_add = notifier_add_irq,
 	.notifier_del = notifier_del_irq,
 	.notifier_hangup = notifier_hangup_irq,
@@ -141,6 +142,7 @@ static int hvc_opal_hvsi_tiocmset(struct hvc_struct *hp, unsigned int set,
 static const struct hv_ops hvc_opal_hvsi_ops = {
 	.get_chars = hvc_opal_hvsi_get_chars,
 	.put_chars = hvc_opal_hvsi_put_chars,
+	.flush = opal_flush_chars,
 	.notifier_add = hvc_opal_hvsi_open,
 	.notifier_del = hvc_opal_hvsi_close,
 	.notifier_hangup = hvc_opal_hvsi_hangup,
-- 
2.17.0

Re: [15/15] powerpc/powernv: provide a console flush operation for opal hvc driver

[Michael Ellerman]

On Mon, 2018-04-30 at 14:55:58 UTC, Nicholas Piggin wrote:
> Provide the flush hv_op for the opal hvc driver. This will flush the
> firmware console buffers without spinning with interrupts disabled.
> 
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/95b861a76c1ded3e89d33a3d9f4552

cheers