improve OPAL cosole flushing and locking
Nicholas Piggin
reviewercover improve OPAL cosole flushing and locking
This series of patches aims to improve the robustness of the OPAL console by simplifying and removing locks. If this code is interrupted due to a panic or xmon request, it may block the xmon IO or panic console flush because they use the opal console too.
So reducing locking and relaxing atomicity requirements is important for improving kernel debugging.
Nicholas Piggin (6):
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_nonatomic drivers/tty/hvc: remove unexplained "just in case" spin delay
arch/powerpc/include/asm/opal.h | 2 + arch/powerpc/platforms/powernv/opal-kmsg.c | 38 +----- arch/powerpc/platforms/powernv/opal.c | 141 ++++++++++++++------- drivers/tty/hvc/hvc_opal.c | 17 ++- 4 files changed, 106 insertions(+), 92 deletions(-)
Signature · 1 line
2.17.0
Nicholas Piggin
reviewerPATCH 1/6 powerpc/powernv: opal-kmsg use flush fallback from console code
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.
--- arch/powerpc/platforms/powernv/opal-kmsg.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
arch/powerpc/platforms/powernv/opal-kmsg.c +6 -9
@@ -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 ); } }
Signature · 1 line
2.17.0
Re: [PATCH 1/6] powerpc/powernv: opal-kmsg use flush fallback from console code
Quoted 7 lines · 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.
Quoted 5 lines
Use
WARN_ONCErather than a printk when the fallback is invoked to prepare for moving the console flush into a common function.Cc Russell Currey <[email protected]>Signed-off-by Nicholas Piggin <[email protected]>
Nicholas Piggin
reviewerPATCH 2/6 powerpc/powernv: Implement and use opal_flush_console
In reply to Nicholas PigginA 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.
--- 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(-)
arch/powerpc/include/asm/opal.h +1 -0
@@ -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 );
arch/powerpc/platforms/powernv/opal-kmsg.c +3 -32
@@ -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 )
arch/powerpc/platforms/powernv/opal.c +38 -4
64 lines not on this page · raw message
Signature · 1 line
2.17.0
Re: [PATCH 2/6] powerpc/powernv: Implement and use opal_flush_console
Quoted 9 lines · 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 akmsg_dumperthat 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.
Quoted 6 lines
So move the opal-kmsg flush code to
opal_flush_console, and use the new function in opal-kmsg andopal_put_chars.Cc Benjamin Herrenschmidt <[email protected]>Cc Russell Currey <[email protected]>Signed-off-by Nicholas Piggin <[email protected]>
Nicholas Piggin
reviewerPATCH 3/6 powerpc/powernv: Remove OPALv1 support from opal console driver
In reply to Nicholas Pigginopal_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 and turn 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.
--- arch/powerpc/platforms/powernv/opal.c | 86 +++++++++++++-------------- 1 file changed, 40 insertions(+), 46 deletions(-)
arch/powerpc/platforms/powernv/opal.c +40 -46
@@ -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; }
Signature · 1 line
2.17.0
Nicholas Piggin
reviewerPATCH 4/6 powerpc/powernv: move opal console flushing to udbg
In reply to Nicholas PigginOPAL 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.
--- arch/powerpc/platforms/powernv/opal.c | 12 +++++++----- drivers/tty/hvc/hvc_opal.c | 5 +++++ 2 files changed, 12 insertions(+), 5 deletions(-)
arch/powerpc/platforms/powernv/opal.c +7 -5
@@ -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; }
drivers/tty/hvc/hvc_opal.c +5 -0
@@ -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 ); }
Signature · 1 line
2.17.0
Nicholas Piggin
reviewerPATCH 5/6 powerpc/powernv: implement opal_put_chars_nonatomic
In reply to Nicholas PigginThe RAW console does not need writes to be atomic, so implement a _nonatomic variant which does not take a spinlock. This API is used in xmon, so the less locking thta's used, the better chance there is that a crash can be debugged.
--- arch/powerpc/include/asm/opal.h | 1 + arch/powerpc/platforms/powernv/opal.c | 35 +++++++++++++++++++-------- drivers/tty/hvc/hvc_opal.c | 4 +-- 3 files changed, 28 insertions(+), 12 deletions(-)
arch/powerpc/include/asm/opal.h +1 -0
@@ -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_nonatomic (uint32_t vtermno,const char *buf,int total_len);extern int opal_flush_console (uint32_t vtermno);extern void hvc_opal_init_early (void );
arch/powerpc/platforms/powernv/opal.c +25 -10
@@ -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,20 @@ 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) +{ +/* 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. +*/ +return __opal_put_chars (vtermno, data, total_len, true); +} + +int opal_put_chars_nonatomic (uint32_t vtermno,const char *data,int total_len) +{ +return __opal_put_chars (vtermno, data, total_len, false); +} +int opal_flush_console (uint32_t vtermno) {s64 rc;
drivers/tty/hvc/hvc_opal.c +2 -2
@@ -51,7 +51,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, + .put_chars = opal_put_chars_nonatomic, .notifier_add = notifier_add_irq, .notifier_del = notifier_del_irq, .notifier_hangup = notifier_hangup_irq,
@@ -269,7 +269,7 @@ static void udbg_opal_putc(char c) do { switch(hvc_opal_boot_priv.proto) { case HV_PROTOCOL_RAW: - count = opal_put_chars(termno, &c, 1); + count = opal_put_chars_nonatomic(termno, &c, 1); break; case HV_PROTOCOL_HVSI: count = hvc_opal_hvsi_put_chars(termno, &c, 1);
Signature · 1 line
2.17.0
Re: [PATCH 5/6] powerpc/powernv: implement opal_put_chars_nonatomic
Quoted 4 lines · Nicholas Piggin
The RAW console does not need writes to be atomic, so implement a
_nonatomicvariant which does not take a spinlock. This API is used in xmon, so the less locking thta's used, the better chance there is that a crash can be debugged.
I find the term "nonatomic" confusing... don't we have a problem if we start hitting OPAL without a lock where we can't trust opal_console_write_buffer_space anymore ? I think we need to handle partial writes in that case. Maybe we should return how much was written and leave the caller to deal with it.
I was hoping (but that isn't the case) that by nonatomic you actually meant calls that could be done in a non-atomic context, where we can do msleep instead of mdelay. That would be handy for the console coming from the hvc thread (the tty one).
Cheers,
Ben.
Quoted 116 lines
Cc Benjamin Herrenschmidt <[email protected]>Signed-off-by Nicholas Piggin <[email protected]>--- arch/powerpc/include/asm/opal.h | 1 + arch/powerpc/platforms/powernv/opal.c | 35 +++++++++++++++++++-------- drivers/tty/hvc/hvc_opal.c | 4 +-- 3 files changed, 28 insertions(+), 12 deletions(-)arch/powerpc/include/asm/opal.h +1
-0@@ -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_nonatomic (uint32_t vtermno,const char *buf,int total_len);extern int opal_flush_console (uint32_t vtermno);extern void hvc_opal_init_early (void );arch/powerpc/platforms/powernv/opal.c +25
-10@@ -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,20 @@ 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) +{ +/* 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. +*/ +return __opal_put_chars (vtermno, data, total_len, true); +} + +int opal_put_chars_nonatomic (uint32_t vtermno,const char *data,int total_len) +{ +return __opal_put_chars (vtermno, data, total_len, false); +} +int opal_flush_console (uint32_t vtermno) {s64 rc;drivers/tty/hvc/hvc_opal.c +2
-2@@ -51,7 +51,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, + .put_chars = opal_put_chars_nonatomic, .notifier_add = notifier_add_irq, .notifier_del = notifier_del_irq, .notifier_hangup = notifier_hangup_irq,@@ -269,7 +269,7 @@ static void udbg_opal_putc(char c) do { switch(hvc_opal_boot_priv.proto) { case HV_PROTOCOL_RAW: - count = opal_put_chars(termno, &c, 1); + count = opal_put_chars_nonatomic(termno, &c, 1); break; case HV_PROTOCOL_HVSI: count = hvc_opal_hvsi_put_chars(termno, &c, 1);
Nicholas Piggin
reviewerRe: [PATCH 5/6] powerpc/powernv: implement opal_put_chars_nonatomic
Quoted 7 lines · On Mon, 09 Apr 2018 15:57:55 +1000 Benjamin Herrenschmidt…
Quoted 4 lines · Nicholas Piggin
The RAW console does not need writes to be atomic, so implement a
_nonatomicvariant which does not take a spinlock. This API is used in xmon, so the less locking thta's used, the better chance there is that a crash can be debugged.I find the term "nonatomic" confusing...
I guess it is to go with the "atomic" comment for the hvsi console case -- all characters must get to the console together or not at all.
Quoted 5 lines
don't we have a problem if we
start hitting OPAL without a lock where we can't trustopal_console_write_buffer_spaceanymore ? I think we need to handle partial writes in that case. Maybe we should return how much was written and leave the caller to deal with it.
Yes, the _nonatomic variant doesn't use opal_console_write_buffer_space and it does handle partial writes by returning written bytes (although callers generally tend to loop at the moment, we might do something smarter with them later).
Quoted 4 lines
I was hoping (but that isn't the case) that by nonatomic you actually meant calls that could be done in a non-atomic context, where we can do msleep instead of mdelay. That would be handy for the console coming from the hvc thread (the tty one).
Ah right, no. However we no longer loop until everything is written, so the hvc console driver (or the console layer) should be able to deal with that with sleeping. I don't think we need to put it at this level of the driver, but I don't know much about the console code.
Thanks,
Nick
Re: [PATCH 5/6] powerpc/powernv: implement opal_put_chars_nonatomic
Quoted 9 lines · Nicholas Piggin
Quoted 6 lines · On Mon, 09 Apr 2018 15:57:55 +1000 Benjamin Herrenschmidt…
Quoted 4 lines · Nicholas Piggin
The RAW console does not need writes to be atomic, so implement a
_nonatomicvariant which does not take a spinlock. This API is used in xmon, so the less locking thta's used, the better chance there is that a crash can be debugged.
Quoted 5 lines
Quoted 1 line
I find the term "nonatomic" confusing...
I guess it is to go with the "atomic" comment for the hvsi console case -- all characters must get to the console together or not at all.
Yeah ok, it's just that in Linux "atomic" usually means something else :-) Why not just call it "unlocked" which is what it's about and matches existing practices thorough the kernel ?
Quoted 16 lines
Quoted 5 lines
don't we have a problem if we
start hitting OPAL without a lock where we can't trustopal_console_write_buffer_spaceanymore ? I think we need to handle partial writes in that case. Maybe we should return how much was written and leave the caller to deal with it.Yes, the
_nonatomicvariant doesn't useopal_console_write_buffer_spaceand it does handle partial writes by returning written bytes (although callers generally tend to loop at the moment, we might do something smarter with them later).Quoted 4 lines
I was hoping (but that isn't the case) that by nonatomic you actually meant calls that could be done in a non-atomic context, where we can do msleep instead of mdelay. That would be handy for the console coming from the hvc thread (the tty one).
Quoted 4 lines
Ah right, no. However we no longer loop until everything is written, so the hvc console driver (or the console layer) should be able to deal with that with sleeping. I don't think we need to put it at this level of the driver, but I don't know much about the console code.
Ok, so hopefully we shouldn't be hitting the delay..
Cheers,
Ben.
Nicholas Piggin
reviewerRe: [PATCH 5/6] powerpc/powernv: implement opal_put_chars_nonatomic
Quoted 12 lines · On Mon, 09 Apr 2018 18:24:46 +1000 Benjamin Herrenschmidt…
Quoted 11 lines · Nicholas Piggin
Quoted 7 lines · On Mon, 09 Apr 2018 15:57:55 +1000 Benjamin Herrenschmidt…
Quoted 4 lines · Nicholas Piggin
The RAW console does not need writes to be atomic, so implement a
_nonatomicvariant which does not take a spinlock. This API is used in xmon, so the less locking thta's used, the better chance there is that a crash can be debugged.I find the term "nonatomic" confusing...
Quoted 7 lines
Quoted 3 lines
I guess it is to go with the "atomic" comment for the hvsi console case -- all characters must get to the console together or not at all.
Yeah ok, it's just that in Linux "atomic" usually means something else :-) Why not just call it "unlocked" which is what it's about and matches existing practices thorough the kernel ?
Sure, I'll change it.
Quoted 16 lines
Quoted 16 lines
Quoted 5 lines
don't we have a problem if we
start hitting OPAL without a lock where we can't trustopal_console_write_buffer_spaceanymore ? I think we need to handle partial writes in that case. Maybe we should return how much was written and leave the caller to deal with it.Yes, the
_nonatomicvariant doesn't useopal_console_write_buffer_spaceand it does handle partial writes by returning written bytes (although callers generally tend to loop at the moment, we might do something smarter with them later).Quoted 4 lines
I was hoping (but that isn't the case) that by nonatomic you actually meant calls that could be done in a non-atomic context, where we can do msleep instead of mdelay. That would be handy for the console coming from the hvc thread (the tty one).
Quoted 6 lines
Quoted 4 lines
Ah right, no. However we no longer loop until everything is written, so the hvc console driver (or the console layer) should be able to deal with that with sleeping. I don't think we need to put it at this level of the driver, but I don't know much about the console code.
Ok, so hopefully we shouldn't be hitting the delay..
I *think* so. It may actually hit it once on the way out, but I don't know if it's worth adding a new API to avoid it. Probably warrants somebody to take a look and measure things though.
Actually I did just take a look. The bigger problem actually is because we do the console flush here and even the "good" hvc path holds an irq lock in this case:
dmesg-8334 12d... 0us : _raw_spin_lock_irqsave dmesg-8334 12d... 0us : hvc_push <-hvc_write dmesg-8334 12d... 1us : opal_put_chars_nonatomic <-hvc_push dmesg-8334 12d... 1us : __opal_put_chars <-hvc_push dmesg-8334 12d... 2us : opal_flush_console <-__opal_put_chars dmesg-8334 12d... 4us!: udelay <-opal_flush_console dmesg-8334 12d... 787us : soft_nmi_interrupt <-soft_nmi_common dmesg-8334 12d... 787us : printk_nmi_enter <-soft_nmi_interrupt dmesg-8334 12d.Z. 788us : rcu_nmi_enter <-soft_nmi_interrupt dmesg-8334 12d.Z. 788us : rcu_nmi_exit <-soft_nmi_interrupt dmesg-8334 12d... 788us#: printk_nmi_exit <-soft_nmi_interrupt dmesg-8334 12d... 10005us*: udelay <-opal_flush_console dmesg-8334 12d... 20007us*: udelay <-opal_flush_console dmesg-8334 12d... 30020us*: udelay <-opal_flush_console dmesg-8334 12d... 40022us*: udelay <-opal_flush_console dmesg-8334 12d... 50023us*: udelay <-opal_flush_console dmesg-8334 12d... 60024us : opal_error_code <-opal_flush_console dmesg-8334 12d... 60025us : _raw_spin_unlock_irqrestore <-hvc_write dmesg-8334 12d... 60025us : _raw_spin_unlock_irqrestore dmesg-8334 12d... 60025us : trace_hardirqs_on <-_raw_spin_unlock_irqrestore dmesg-8334 12d... 60027us : <stack trace>
60ms interrupt off latency waiting for the console to flush (just running dmesg from OpenBMC console).
That requires some reworking of the hvc code, we can't fix it in the OPAL driver alone.
Thanks,
Nick
Nicholas Piggin
reviewerPATCH 6/6 drivers/tty/hvc: remove unexplained "just in case" spin delay
In reply to Nicholas PigginThis delay was in the very first OPAL console commit 6.5 years ago. The firmware console has hardened sufficiently to remove it.
--- drivers/tty/hvc/hvc_opal.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/tty/hvc/hvc_opal.c +1 -7
@@ -307,14 +307,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; - } } }
Signature · 1 line
2.17.0
Re: [PATCH 6/6] drivers/tty/hvc: remove unexplained "just in case" spin delay
Quoted 3 lines · Nicholas Piggin
This delay was in the very first OPAL console commit 6.5 years ago. The firmware console has hardened sufficiently to remove it.
Quoted 25 lines
Signed-off-by Nicholas Piggin <[email protected]>--- drivers/tty/hvc/hvc_opal.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)drivers/tty/hvc/hvc_opal.c +1
-7@@ -307,14 +307,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; - } } }