lkml.org 
[lkml]   [2016]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] printk/nmi: avoid direct printk()-s from __printk_nmi_flush()
On Mon 2016-08-29 21:32:20, Sergey Senozhatsky wrote:
> From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
>
> __printk_nmi_flush() can be called from nmi_panic(), therefore it has to
> test whether it's executed in NMI context and thus must route the messages
> through deferred printk() or via direct printk(). Except for two places
> where __printk_nmi_flush() does unconditional direct printk() calls:
> - pr_err("printk_nmi_flush: internal error ...")
> - pr_cont("\n")
>
> Factor out __print_nmi_seq_line(), which takes care of in_nmi(), and use
> it in __printk_nmi_flush() for printing and error-reporting.

Great catch! The check and eventually printk_deferred() need to be used
everywhere in __printk_nmi_flush().

Just some nitpicks below.

> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> kernel/printk/nmi.c | 29 ++++++++++++++++++++---------
> 1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> index b69eb8a..5f2c198 100644
> --- a/kernel/printk/nmi.c
> +++ b/kernel/printk/nmi.c
> @@ -103,23 +103,32 @@ again:
> * printk one line from the temporary buffer from @start index until
> * and including the @end index.
> */

The comment above is not longer valid.

> -static void print_nmi_seq_line(struct nmi_seq_buf *s, int start, int end)
> +static void __print_nmi_seq_line(const char *text, int len)

Also the name of the function might be confusing because it is not
longer used only for the seq buffer. I would rename it to
something like:

printk_nmi_flush_line()

> {
> - const char *buf = s->buffer + start;
> -
> /*
> * The buffers are flushed in NMI only on panic. The messages must
> * go only into the ring buffer at this stage. Consoles will get
> * explicitly called later when a crashdump is not generated.
> */
> if (in_nmi())
> - printk_deferred("%.*s", (end - start) + 1, buf);
> + printk_deferred("%.*s", len, text);
> else
> - printk("%.*s", (end - start) + 1, buf);
> + printk("%.*s", len, text);
>
> }
>
> /*
> + * printk one line from the temporary buffer from @start index until
> + * and including the @end index.
> + */
> +static void print_nmi_seq_line(struct nmi_seq_buf *s, int start, int end)
> +{

Then I would rename also this function to something like:

printk_nmi_flush_seq_line()

> + const char *buf = s->buffer + start;
> +
> + __print_nmi_seq_line(buf, (end - start) + 1);
> +}
> +

Othrewise, it looks fine. With the above suggested changes, feel
free to add:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.083 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site