From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757700AbcH2PQD (ORCPT ); Mon, 29 Aug 2016 11:16:03 -0400 Received: from mx2.suse.de ([195.135.220.15]:50726 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757563AbcH2PQC (ORCPT ); Mon, 29 Aug 2016 11:16:02 -0400 Date: Mon, 29 Aug 2016 17:16:00 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Andrew Morton , Jan Kara , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH] printk/nmi: avoid direct printk()-s from __printk_nmi_flush() Message-ID: <20160829151600.GM4866@pathway.suse.cz> References: <20160829123220.1295-1-sergey.senozhatsky@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160829123220.1295-1-sergey.senozhatsky@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 2016-08-29 21:32:20, Sergey Senozhatsky wrote: > From: Sergey Senozhatsky > > __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 > --- > 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 Best Regards, Petr