Date:   Fri, 19 May 2023 16:40:01 +0800
From:   Hongyu Xie <xiehongyu1@...inos.cn>
To:     rdunlap@...radead.org, linux@...linux.org.uk,
        gregkh@...uxfoundation.org, jirislaby@...nel.org
Cc:     linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        xy521521@...il.com, Hongyu Xie <xiehongyu1@...inos.cn>
Subject: Re: [RFC PATCH v2 -next] tty: serial: add panic serial helper

Hi,


在 2023/5/19 13:33, Randy Dunlap 写道:
> Hi--
> 
> On 5/18/23 03:29, Hongyu Xie wrote:
>> It was inspired by kgdboc.
>>
>> This is a debug module that allows you to get all kernel logs
>> after panic.
>>
>> Normally you need to attach a USB-to-UART tool or enable kdump
>> before panic happens to get log from kernel after panic. If you
>> didn't do that and kdump is not working, you can't get any log to
>> know what happened before panic. If you have a USB-to-UART tool
>> and the uart port on your computer is working. This module helps
>> you to get all kernel log after panic() is called.
>>
>> To use this, see Documentation/dev-tools/panic_serial_helper.rst.
>>
>> Tested on arm64, x86 device.
>>
>> Signed-off-by: Hongyu Xie <xiehongyu1@...inos.cn>
>> ---
>>
>> v2:
>>   1. add a doc file
>>   2. remove the password thing
>>
>>   .../dev-tools/panic_serial_helper.rst         | 143 +++++
> 
> This file name (panic_serial_helper) also needs to be added to
> Documentation/dev-tools/index.rst.
> 
>>   MAINTAINERS                                   |   5 +
>>   drivers/tty/serial/Kconfig                    |  46 ++
>>   drivers/tty/serial/Makefile                   |   1 +
>>   drivers/tty/serial/panic_serial_helper.c      | 571 ++++++++++++++++++
>>   include/linux/panic.h                         |   1 +
>>   kernel/panic.c                                |  12 +
>>   7 files changed, 779 insertions(+)
>>   create mode 100644 Documentation/dev-tools/panic_serial_helper.rst
>>   create mode 100644 drivers/tty/serial/panic_serial_helper.c
>>
>> diff --git a/Documentation/dev-tools/panic_serial_helper.rst b/Documentation/dev-tools/panic_serial_helper.rst
>> new file mode 100644
>> index 000000000000..adbc4026fbe4
>> --- /dev/null
>> +++ b/Documentation/dev-tools/panic_serial_helper.rst
>> @@ -0,0 +1,143 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
> 
> The "===...===" lines here must be at least as long as the heading.
> 
>> +=================================================
>> +Using panic serial helper to get kernel logs after panic
>> +=================================================
> 
>> +========================================================
>> +Using panic serial helper to get kernel logs after panic
>> +========================================================
> 
>> +
>> +:Author: Hongyu Xie <xiehongyu1@...inos.cn>
>> +
>> +What is this?
>> +============
> 
>     =============
> 
>> +
>> +A debug module inspired by kgdboc that allows you to get all kernel logs
>> +after panic.
>> +
>> +When do you need it and why?
>> +============
> 
>     =============================
> 
>> +
>> +When
>> +--------------
>> +
>> +Didn't enable debugging tool like Kdump and didn't connect a USB-to-UART
>> +tool to the debug uart port on your PC before panic.
>> +
>> +Why
>> +--------------
>> +
>> +There are many debugging methods to know what was going on before panic.
>> +
>> +Kdump, for example. If Kdump is enabled, you can get a core image after
>> +panic. Then use GDB or Crash to debug that core image to know what happened
>> +before panic(see ``Documentation/admin-guide/kdump/kdump.rst`` for more
> 
>     before panice (see
> 
>> +information about Kdump).
>> +
>> +Another way is to connect the UART side of a USB-to-UART tool to the
> 
> Please use "UART" consistently (not "uart").
> 
>> +debugging uart port(normally a 3 pin slot on the motherborad or a RS232
> 
>     debugging UART port (normally a 3-pin            motherboard or an RS232
> 
>> +port on the back panel of your PC) before panic happens. Then connect the
> 
> I wish I had a PC with a serial port so that I could test/use this.
> 
>> +USB side of a USB-to-UART tool to another PC. You can read all the kernel
>> +logs coming from that uart port through apps like minicom on another PC.
>> +So when panic happens you'll know what was going on.
>> +
>> +What if Kdump hasn't been enabled? And in production environment you don't
>> +always connect a USB-to-UART tool before panic happens.
>> +
>> +So if Kdump is not enabled, you can use this module to get all the kernel
>> +logs after panic.
> 
> if this module is loaded prior to the panic.
> 
>> +
>> +How to use it?
>> +============
> 
>     ===============
>> +
>> +Prerequisites
>> +--------------
>> +
>> +1. Same as kgdboc, the UART driver must implement two callbacks in the
>> +struct uart_ops. See ``Documentation/dev-tools/kgdb.rst`` section
>> +``kgdboc and uarts``
>> +
>> +2. Your PC has an uart port and it's working.
>> +
>> +How
>> +--------------
>> +
>> +First you need to enable ``CONFIG_PANIC_SERIAL_HELPER`` in your
>> +config. To enable ``CONFIG_PANIC_SERIAL_HELPER`` you should look under
>> +:menuselection:
>> +`Device Drivers
>> +  --> Character devices
>> +    --> Enable TTY (TTY [=y])
>> +      --> Serial drivers`
>> +and select
>> +:menuselection:`debug through uart after panic`.
>> +
>> +Second, build and update the kernel image. Then wait for panic.
>> +
>> +After panic, you need to do the following,
> 
>                                     following:
> 
>> +1. connect the uart side of an USB-to-UART tool to any uart
>> +  port on your device(PC, server, Laptop, etc...) after panic.
> 
>                    device (PC,                etc.).
> 
> You have already said "after panic".
> 
>> +  Connect the USB side of that tool to another PC. Open
>> +  minicom(or other app) on that PC, and set "/dev/ttyUSB0"(or
> 
>       minicom (or                                     ttyUSB0" (or
> 
>> +  "/dev/ttyUSB1 if there is already another USB-to-UART tool
>> +  connected to your device) with "115200 8N1".
>> +
>> +  It automatically selects the port where you first pressing the
> 
>                                                   first press the
> 
>> +  "Enter"(some keyboard labeled with "Return")
> 
>       "Enter" key (some keyboards label this with "Return").
> 
>> +
>> +2. press "Enter"(some keyboard labeled with "Return") in that
> 
>              "Enter" (or "Return") in that
> 
>> +  minicom window, you'll get a help menu,
> 
>       minicom window; you'll get a help meu:
> 
>> +  "
>> +  help:
>> +      -a      show all kernel msg
>> +      -3      show S3 msg
>> +      -4      show S4 msg
>> +      -filter-[string]        show msg contains [string]
> 
>                                            containing
> 
>> +      -q-     quit
>> +  "
>> +
>> +see ``Help menu options`` for details.
>> +
>> +3. finally, type 'a', '3', '4', 'q' or "filter-xxx" then press
>> + "Enter" to get what you want.
>> +
>> +Help menu options
>> +--------------
>> +Available options:
>> +
>> + - a
>> +
>> +   Show all the messages starting from ``Booting Linux on ...``
>> +
>> + - 3
>> +
>> +   If STR happened before panic, this will show messages starting from
> 
> What "STR" is this?
Suspend to ram, also known as S3.
> 
>> +   ``PM: suspend entry...``
>> +
>> + - 4
>> +
>> +   If STD happened before panic, this will show messages starting from
> 
> Is this "STD" or "STR"?
STD, suspend to disk, also known as S4.
> 
>> +   ``PM: hibernation entry...``
>> +
>> + - filter-[string]
>> +
>> +   Only show messages that contain ``string``. For example, if you're only
>> +   interesting in message lines that contain ``CPU``, you just input
>> +   ``filter-CPU``.
>> +   Here is an output example for fitering ``CPU``::
> 
>                                      filtering
> 
> It might be nice to provide case-ignored filter matching, e.g.,
> find "CPU" or "cpu".
Ok, it'll be in v3.
> 
>> +
>> +   <6>[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x701f6633
>> +   <6>[    0.000000] Detected PIPT I-cache on CPU0
>> +   <6>[    0.000000] CPU features: detected: Kernel page table isolation (K
>> +   ...
>> +   <6>[    0.000000] GICv3: CPU0: using allocated LPI pending table @0x0000
>> +   <6>[    0.002411] smp: Bringing up secondary CPUs ...
>> +   <6>[    0.039105] Detected PIPT I-cache on CPU1
>> +   ...
>> +   <4>[    6.432129] CPU: 3 PID: 392 Comm: (crub_all) Tainted: G        W
>> +   <4>[    6.560279] CPU: 2 PID: 478 Comm: (ostnamed) Tainted: G        W
>> +   ...
>> +   <4>[  225.297828] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G        W
>> +   <2>[  225.297909] SMP: stopping secondary CPUs
>> +   <0>[  225.297919] CPU features: 0x000000,02000800,0400421b
>> +
>> + - q-
>> +
>> +   return to help menu.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5bd0f510f744..951c6804b3cb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11552,6 +11552,11 @@ F:	include/linux/kgdb.h
>>   F:	kernel/debug/
>>   F:	kernel/module/kdb.c
>>   
>> +PANIC SERIAL CONSOLE
>> +M:	Hongyu Xie <xiehongyu1@...inos.cn>
>> +F:	drivers/tty/serial/panic_serial_helper.c
>> +F:	drivers/tty/serial/panic_serial_helper.h
>> +
> 
> This is the wrong place for "PANIC SERIAL CONSOLE".
> The MAINTAINERS file should remain in alphabetical order.
> Please move this to after
> PANASONIC LAPTOP ACPI EXTRAS DRIVER
> Thanks.
> 
>>   KHADAS MCU MFD DRIVER
>>   M:	Neil Armstrong <neil.armstrong@...aro.org>
>>   L:	linux-amlogic@...ts.infradead.org
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index 398e5aac2e77..66cc7bddf561 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -198,6 +198,52 @@ config SERIAL_KGDB_NMI
>>   
>>   	  If unsure, say N.
>>   
> 
> I have comments here and similar ones in the documentation file.
> It would be quite OK to make the Kconfig help text shorter and refer
> to the Documentation file for more detailed help.
> 
>> +config PANIC_SERIAL_HELPER
>> +	tristate "debug through uart after panic"
>> +	depends on PANIC_TIMEOUT=0
>> +	select CONSOLE_POLL
>> +	help
>> +	  This is a debug module that allows you to get all kernel logs
>> +	  after panic.
>> +
>> +	  Normally you need to attach a USB-to-UART tool or enable kdump
>> +	  before panic happens to get log from kernel after panic. If you
>> +	  didn't do that and kdump is not working, you can't get any log to
>> +	  know what happened before panic. If you have a USB-to-UART tool
>> +	  and the uart port on your computer is working. This module helps
> 
> 	                                     is working, this module helps
> 
>> +	  you to get all kernel log after panic() is called.
>> +
>> +	  This module use serial port in poll mode, so it's more stable
> 
> 	              uses
> 
>> +	  than other debugging methods.
>> +
>> +	  To use this, you need to do the following after panic,
>> +	    1. connect the uart side of an USB-to-UART tool to any uart
>> +	    port on your device(PC, server, Laptop, etc...) after panic.
> 
> 	                 device (PC,                etc.).
> 
>> +	    Connect the USB side of that tool to another PC. Open
>> +	    minicom(or other app) on that PC, and set "/dev/ttyUSB0"(or
> 
> 	    minicom (or                                     ttyUSB0" (or
> 
>> +	    "/dev/ttyUSB1 if there is already another USB-to-UART tool
>> +	    connected to your device) with "115200 8N1".
>> +
>> +	    It automatically selects the port where you first pressing the
> 
> 	                                                first press the
> 
>> +	    "Enter"(some keyboard labeled with "Return")
> 
> 	    "Enter" key (some keyboards label this as "Return").
> 
>> +
>> +	    2.press "Enter"(some keyboard labeled with "Return") in that
> 
> 	    2. press "Enter" (or "Return") in that
> 
>> +	    minicom window, you'll get a help menu,
> 
> 	            window; you'll get a help menu:
> 
>> +	    "
>> +	    help:
>> +	        -a      show all kernel msg
>> +	        -3      show S3 msg
>> +	        -4      show S4 msg
>> +	        -filter-[string]        show msg contains [string]
> 
> 	                                         containing
> 
>> +	        -q-     quit
>> +	    "
>> +
>> +	   3.Finally, type 'a', '3', '4', 'q' or "filter-xxx" then press
> 
> 	   3. Finally,
> 
>> +	   "Enter" to get what you want.
>> +
>> +	  Say Y if you have an UART port that is working.  If unsure, say N
> 
> 	                                                              say N.
> 
>> +	  Say M if you want add this as a module driver.
> 
> 	  The moudul will be called panic_serial_helper.
> 
>> +
>>   config SERIAL_MESON
>>   	tristate "Meson serial port support"
>>   	depends on ARCH_MESON || COMPILE_TEST
>> diff --git a/drivers/tty/serial/panic_serial_helper.c b/drivers/tty/serial/panic_serial_helper.c
>> new file mode 100644
>> index 000000000000..59863f777331
>> --- /dev/null
>> +++ b/drivers/tty/serial/panic_serial_helper.c
>> @@ -0,0 +1,571 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * panic_serial_helper.c Debug through uart when panic.
>> + *
>> + * Copyright (C) 2023 Xie Hongyu <xiehongyu1@...inos.cn>
>> + *
>> + * Inspired by kgdboc.
>> + *
>> + */
>> +
>> +#define MODULE_NAME "panic_seial_helper"
> 
>                                serial
> 
>> +#define pr_fmt(fmt) MODULE_NAME ": " fmt
>> +
>> +#include <linux/kmsg_dump.h>
>> +#include <linux/bsearch.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/tty_driver.h>
>> +#include <linux/serial_core.h>
>> +
>> +#define S3_ENTRY "PM: suspend entry"
>> +#define S3_EXIT "PM: suspend exit"
>> +#define S4_ENTRY "PM: hibernation entry"
>> +#define S4_EXIT "PM: hibernation exit"
>> +
>> +/* list to store msg lines */
>> +static LIST_HEAD(psh_list);
>> +
>> +/* msg line prototype */
>> +struct dmesg_lines {
>> +	struct list_head entry;
>> +	char *buf;
>> +	int size;
>> +};
>> +
>> +/* panic serial helper status*/
>> +enum PSHS {
>> +	PSHS_INIT,
>> +	PSHS_WAIT_HELP_INPUT,
>> +};
>> +
>> +/* panic serial helper msg type */
>> +enum PSHM_TYPE {
>> +	PSHM_TYPE_ALL,
>> +	PSHM_TYPE_S3,
>> +	PSHM_TYPE_S4,
>> +	PSHM_TYPE_STRINGS,
>> +	PSHM_TYPE_QUIT,
>> +};
>> +
>> +/* whether uart is dumping msg */
>> +static bool dumping_msg;
>> +
>> +/* to filter msg */
>> +static char filter[256] = {0};
>> +
>> +struct psh_buf {
>> +#define PSH_BUF_SIZE 256
>> +	char buf[PSH_BUF_SIZE];
>> +	int cur;
>> +};
>> +
>> +static const char psh_tty_types[][32] = {
>> +	{"ttyAMA"},
>> +	{"ttyS"},
>> +	{"ttyPS"},
>> +	{"ttyLP"},
>> +	{"ttyARC"},
>> +	{"ttyAL"},
>> +	{"ttyUL"},
>> +};
>> +
>> +#define TTY_OPS "115200n8n"
> 
> Other places here say "115200 8N1". Is there a typo above?
Not a typo, the order that uart_parse_options parse is parity, bits, 
flow. "115200 8N1" shows in minicom setting window.
> 
>> +
>> +struct psh_serial_dev {
>> +	struct list_head entry;
>> +	struct tty_driver *drv;
>> +	struct psh_buf buf;
>> +	enum PSHS psh_status;
>> +	enum PSHM_TYPE psh_msg_type;
>> +	int line;
>> +};
>> +
>> +struct psh_serial_dev *psh_dev;
>> +
>> +/* char handle prototype */
> 
> s/handle/handler/ IMO
> and in other places also.
> 
>> +struct c_handle {
>> +	char c;
>> +	int (*handler)(struct psh_serial_dev *dev, void *d);
>> +};
>> +
> 
> [snip]
What does "[snip]" mean? Can't find it on Bing or Google.
> 
>> +
>> +static void psh_help(struct psh_serial_dev *dev)
>> +{
>> +	static const char help[] = "\nhelp:\n";
>> +	static const char show_all[] = "\t-a\tshow all kernel msg\n";
>> +	static const char show_s3[] = "\t-3\tshow S3 msg\n";
>> +	static const char show_s4[] = "\t-4\tshow S4 msg\n";
>> +	static const char _filter[] =
>> +		"\t-filter-[string]\tshow msg contains [string]\n";
> 
> 		                              containing
> 
>> +	static const char _quit[] = "\t-q-\tquit\n";
>> +
>> +	psh_put_strings(dev, help, strlen(help));
>> +	psh_put_strings(dev, show_all, strlen(show_all));
>> +	psh_put_strings(dev, show_s3, strlen(show_s3));
>> +	psh_put_strings(dev, show_s4, strlen(show_s4));
>> +	psh_put_strings(dev, _filter, strlen(_filter));
>> +	psh_put_strings(dev, _quit, strlen(_quit));
>> +}
>> +
> 
> [snip]
> 
>> +
>> +/* try all uart port under the same driver */
>> +static int psh_try_all_uart_port(struct uart_driver *drv)
>> +{
>> +	struct psh_serial_dev *dev = NULL;
>> +	struct uart_driver *driver = NULL;
>> +	char drv_name[64] = {0};
>> +	int count = 10000;
>> +	int i = 0, nr = drv->nr;
>> +
>> +	for (i = 0; i < nr; i++) {
>> +		memset(drv_name, 0, sizeof(drv_name));
>> +		snprintf(drv_name, sizeof(drv_name), "%s%d,%s",
>> +			drv->driver_name,
>> +			i, TTY_OPS);
>> +
>> +		driver = psh_find_uart_driver(drv_name);
>> +		if (!driver)
>> +			continue;
>> +
>> +		dev = create_psh_serial_dev(driver, i);
>> +		if (!dev) {
>> +			tty_driver_kref_put(driver->tty_driver);
>> +			driver = NULL;
>> +			continue;
>> +		}
>> +
>> +		count = 10000;
> 
> Already init-ed to 10000 above...
Need it here. Out side of while loop is a for loop.
> 
>> +		while (count-- > 0) {
>> +			psh_wait_for_input(dev);
>> +
>> +			if (psh_dev)
>> +				return 0;
>> +		}
>> +
>> +		tty_driver_kref_put(driver->tty_driver);
>> +		driver = NULL;
>> +		kfree(dev);
>> +		dev = NULL;
>> +	}
>> +
>> +	return -1;
>> +}
> 
> 
> Thanks.
Thanks for your kind reply. I'll send v3 asap.

Hongyu Xie

Powered by blists - more mailing lists