Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 22 May 2020 17:02:58 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, jason.wessel@windriver.com,
        dianders@chromium.org, pmladek@suse.com,
        sergey.senozhatsky@gmail.com, linux-kernel@vger.kernel.org
Subject: Re: [RFC] kdb: Switch kdb_printf to use safer console poll APIs
Message-ID: <20200522160258.yq63iigp74u3ngtn@holly.lan>
References: <1590158071-15325-1-git-send-email-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1590158071-15325-1-git-send-email-sumit.garg@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 22, 2020 at 08:04:31PM +0530, Sumit Garg wrote:
> In kgdb NMI context, polling driver APIs are more safer to use instead
> of console APIs since the polling drivers know they will execute from
> all sorts of crazy places. And for the most common use cases this would
> also result in no console handler ever being called. So switch to use
> polling driver APIs in case a particular console supports polling mode.

This comment seems rather half hearted, not least because it doesn't
explain what the current problem is nor why using the polling API is
safer.

Compare the above against the advice in
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
and I think it comes up short. Perhaps also consider Ingo Molnar's much
more concise suggestion on describing changes:

: Please use the customary changelog style we use in the kernel:
:   " Current code does (A), this has a problem when (B).
:   We can improve this doing (C), because (D)."
-- http://lkml.iu.edu/hypermail//linux/kernel/1311.1/01157.html


> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 3a5a068..8e0d581 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -24,6 +24,7 @@
>  #include <linux/kgdb.h>
>  #include <linux/kdb.h>
>  #include <linux/kallsyms.h>
> +#include <linux/tty_driver.h>
>  #include "kdb_private.h"
>  
>  #define CMD_BUFLEN 256
> @@ -699,11 +700,24 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  			}
>  		}
>  		for_each_console(c) {
> +			int line;
> +			struct tty_driver *p;
> +
>  			if (!(c->flags & CON_ENABLED))
>  				continue;
> -			++oops_in_progress;
> -			c->write(c, cp, retlen - (cp - kdb_buffer));
> -			--oops_in_progress;
> +			p = c->device ? c->device(c, &line) : NULL;
> +			if (p && p->ops && p->ops->poll_put_char) {

What prevents this logic from matching an active console that hasn't
been selected as the polling driver?


> +				len = retlen - (cp - kdb_buffer);
> +				cp2 = cp;
> +				while (len--) {
> +					p->ops->poll_put_char(p, line, *cp2);
> +					cp2++;
> +				}

Assuming it is possible to identify the console that matches the
currently selected polling driver can't we just drop the
is_console test and get rid of this branch entirely.

The only reason for the is_console test is to avoid issuing messages
twice so if we are able to suppress the c->write() for the same UART
then is_console check becomes pointless and can go.


> +			} else {
> +				++oops_in_progress;
> +				c->write(c, cp, retlen - (cp - kdb_buffer));
> +				--oops_in_progress;
> +			}
>  			touch_nmi_watchdog();
>  		}
>  	}
> @@ -765,11 +779,24 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  			}
>  		}
>  		for_each_console(c) {
> +			int line;
> +			struct tty_driver *p;
> +
>  			if (!(c->flags & CON_ENABLED))
>  				continue;
> -			++oops_in_progress;
> -			c->write(c, moreprompt, strlen(moreprompt));
> -			--oops_in_progress;
> +			p = c->device ? c->device(c, &line) : NULL;
> +			if (p && p->ops && p->ops->poll_put_char) {
> +				len = strlen(moreprompt);
> +				cp = moreprompt;
> +				while (len--) {
> +					p->ops->poll_put_char(p, line, *cp);
> +					cp++;
> +				}
> +			} else {
> +				++oops_in_progress;
> +				c->write(c, moreprompt, strlen(moreprompt));
> +				--oops_in_progress;
> +			}

Maybe also consider pulling the string emit to a separate function.


Daniel.