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.thomp...@linaro.org> > Signed-off-by: Sumit Garg <sumit.g...@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. _______________________________________________ Kgdb-bugreport mailing list Kgdb-bugreport@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
