[PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

Currently, ramoops is capable to collect dmesg buffer only during
panic and oops events. However, it is desirable for shutdown performance
analysis reasons to optionally allow collecting dmesg buffers during other
events as well: reboot, kexec, emergency reboot etc.

How to quickly test:

virtme-run --mods=auto --kdir --mods=auto --kdir . \
	-a memmap=1G$8G -a ramoops.mem_address=0x200000000 \
	-a ramoops.mem_size=0x100000 -a ramoops.record_size=32768 \
	-a ramoops.dump_all=1 -a quiet --qemu-opts -m 8G
..
# reboot -f

After VM is back:

# mount -t pstore pstore /mnt
# head /mnt/dmesg-ramoops-0 
Restart#1 Part1
...

Pavel Tatashin (3):
  printk: honor the max_reason field in kmsg_dumper
  pstore/ram: allow to dump kmesg during regular reboot
  ramoops: add dump_all optional field to ramoops DT node

 .../bindings/reserved-memory/ramoops.txt      |  3 ++
 fs/pstore/platform.c                          |  6 ++-
 fs/pstore/ram.c                               | 38 +++++++++++++------
 include/linux/kmsg_dump.h                     |  1 +
 include/linux/pstore.h                        |  1 +
 include/linux/pstore_ram.h                    |  1 +
 kernel/printk/printk.c                        | 16 ++++----
 7 files changed, 46 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Pavel Tatashin]

kmsg_dump() allows to dump kmesg buffer for various system events: oops,
panic, reboot, etc. It provides an interface to register a callback call
for clients, and in that callback interface there is a field "max_reason"
which gets ignored unless always_kmsg_dump is passed as kernel parameter.

Allow clients to decide max_reason, and keep the current behavior when
max_reason is not set.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 include/linux/kmsg_dump.h |  1 +
 kernel/printk/printk.c    | 16 +++++++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 2e7a1e032c71..c0d703b7ce38 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -28,6 +28,7 @@ enum kmsg_dump_reason {
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
+	KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
 };
 
 /**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a9b6156270b..04c1e9a9b139 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3085,6 +3085,8 @@ EXPORT_SYMBOL(printk_timed_ratelimit);
 
 static DEFINE_SPINLOCK(dump_list_lock);
 static LIST_HEAD(dump_list);
+static bool always_kmsg_dump;
+module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
 
 /**
  * kmsg_dump_register - register a kernel log dumper.
@@ -3106,6 +3108,12 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
 	spin_lock_irqsave(&dump_list_lock, flags);
 	/* Don't allow registering multiple times */
 	if (!dumper->registered) {
+		if (!dumper->max_reason) {
+			if (always_kmsg_dump)
+				dumper->max_reason = KMSG_DUMP_MAX;
+			else
+				dumper->max_reason = KMSG_DUMP_OOPS;
+		}
 		dumper->registered = 1;
 		list_add_tail_rcu(&dumper->list, &dump_list);
 		err = 0;
@@ -3141,9 +3149,6 @@ int kmsg_dump_unregister(struct kmsg_dumper *dumper)
 }
 EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
 
-static bool always_kmsg_dump;
-module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
-
 /**
  * kmsg_dump - dump kernel log to kernel message dumpers.
  * @reason: the reason (oops, panic etc) for dumping
@@ -3157,12 +3162,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	struct kmsg_dumper *dumper;
 	unsigned long flags;
 
-	if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
-		return;
-
 	rcu_read_lock();
 	list_for_each_entry_rcu(dumper, &dump_list, list) {
-		if (dumper->max_reason && reason > dumper->max_reason)
+		if (reason > dumper->max_reason)
 			continue;
 
 		/* initialize iterator with data about the stored records */
-- 
2.25.1

[PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular reboot

[Pavel Tatashin]

Currently, ramoops is capable to collect dmesg buffer only during
panic and oops events. However, it is desirable to optionally allow
collecting dmesg buffers during other events as well: reboot, kexec,
emergency reboot etc.

While, a similar functionality is provided by pstore console it is not the
same. Often, console message level is reduced in production due to baud
rate limitation of serial consoles.  Having a noisy console reduces the
boot performance.

Thus, if the shutdown dmesg buffer is needed to study the shutdown
performance, it is currently not possible to do so with console as by
increasing the console output the shutdown time substantially increases
as well.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 fs/pstore/platform.c       |  6 ++++--
 fs/pstore/ram.c            | 38 +++++++++++++++++++++++++++-----------
 include/linux/pstore.h     |  1 +
 include/linux/pstore_ram.h |  1 +
 4 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 408277ee3cdb..d0393799fe6c 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -476,8 +476,10 @@ static struct kmsg_dumper pstore_dumper = {
 /*
  * Register with kmsg_dump to save last part of console log on panic.
  */
-static void pstore_register_kmsg(void)
+static void pstore_register_kmsg(int dmesg_all)
 {
+	if (dmesg_all)
+		pstore_dumper.max_reason = KMSG_DUMP_MAX;
 	kmsg_dump_register(&pstore_dumper);
 }
 
@@ -603,7 +605,7 @@ int pstore_register(struct pstore_info *psi)
 		pstore_get_records(0);
 
 	if (psi->flags & PSTORE_FLAGS_DMESG)
-		pstore_register_kmsg();
+		pstore_register_kmsg(psi->flags & PSTORE_FLAGS_DMESG_ALL);
 	if (psi->flags & PSTORE_FLAGS_CONSOLE)
 		pstore_register_console();
 	if (psi->flags & PSTORE_FLAGS_FTRACE)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 795622190c01..9d2d1b299225 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -62,6 +62,12 @@ module_param(dump_oops, int, 0600);
 MODULE_PARM_DESC(dump_oops,
 		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
 
+static int dump_all;
+module_param(dump_all, int, 0600);
+MODULE_PARM_DESC(dump_all,
+		 "set to 1 to record all kernel kmesg dump events (default 0) "
+		 "otherwise only panics and oops are recorded");
+
 static int ramoops_ecc;
 module_param_named(ecc, ramoops_ecc, int, 0600);
 MODULE_PARM_DESC(ramoops_ecc,
@@ -82,6 +88,7 @@ struct ramoops_context {
 	size_t ftrace_size;
 	size_t pmsg_size;
 	int dump_oops;
+	int dump_all;
 	u32 flags;
 	struct persistent_ram_ecc_info ecc_info;
 	unsigned int max_dump_cnt;
@@ -381,17 +388,19 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
 	if (record->type != PSTORE_TYPE_DMESG)
 		return -EINVAL;
 
-	/*
-	 * Out of the various dmesg dump types, ramoops is currently designed
-	 * to only store crash logs, rather than storing general kernel logs.
-	 */
-	if (record->reason != KMSG_DUMP_OOPS &&
-	    record->reason != KMSG_DUMP_PANIC)
-		return -EINVAL;
+	if (!cxt->dump_all) {
+		/*
+		 * By default only store crash logs, rather than storing general
+		 * kernel logs.
+		 */
+		if (record->reason != KMSG_DUMP_OOPS &&
+		    record->reason != KMSG_DUMP_PANIC)
+			return -EINVAL;
 
-	/* Skip Oopes when configured to do so. */
-	if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
-		return -EINVAL;
+		/* Skip Oopes when configured to do so. */
+		if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
+			return -EINVAL;
+	}
 
 	/*
 	 * Explicitly only take the first part of any new crash.
@@ -688,6 +697,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	pdata->mem_address = res->start;
 	pdata->mem_type = of_property_read_bool(of_node, "unbuffered");
 	pdata->dump_oops = !of_property_read_bool(of_node, "no-dump-oops");
+	pdata->dump_all = of_property_read_bool(of_node, "dump-all");
 
 #define parse_size(name, field) {					\
 		ret = ramoops_parse_dt_size(pdev, name, &value);	\
@@ -786,6 +796,7 @@ static int ramoops_probe(struct platform_device *pdev)
 	cxt->ftrace_size = pdata->ftrace_size;
 	cxt->pmsg_size = pdata->pmsg_size;
 	cxt->dump_oops = pdata->dump_oops;
+	cxt->dump_all = pdata->dump_all;
 	cxt->flags = pdata->flags;
 	cxt->ecc_info = pdata->ecc_info;
 
@@ -828,8 +839,11 @@ static int ramoops_probe(struct platform_device *pdev)
 	 * the single region size is how to check.
 	 */
 	cxt->pstore.flags = 0;
-	if (cxt->max_dump_cnt)
+	if (cxt->max_dump_cnt) {
 		cxt->pstore.flags |= PSTORE_FLAGS_DMESG;
+		if (cxt->dump_all)
+			cxt->pstore.flags |= PSTORE_FLAGS_DMESG_ALL;
+	}
 	if (cxt->console_size)
 		cxt->pstore.flags |= PSTORE_FLAGS_CONSOLE;
 	if (cxt->max_ftrace_cnt)
@@ -866,6 +880,7 @@ static int ramoops_probe(struct platform_device *pdev)
 	mem_address = pdata->mem_address;
 	record_size = pdata->record_size;
 	dump_oops = pdata->dump_oops;
+	dump_all = pdata->dump_all;
 	ramoops_console_size = pdata->console_size;
 	ramoops_pmsg_size = pdata->pmsg_size;
 	ramoops_ftrace_size = pdata->ftrace_size;
@@ -949,6 +964,7 @@ static void __init ramoops_register_dummy(void)
 	pdata.ftrace_size = ramoops_ftrace_size;
 	pdata.pmsg_size = ramoops_pmsg_size;
 	pdata.dump_oops = dump_oops;
+	pdata.dump_all = dump_all;
 	pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
 
 	/*
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index e779441e6d26..32092c2d7224 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -195,6 +195,7 @@ struct pstore_info {
 #define PSTORE_FLAGS_CONSOLE	BIT(1)
 #define PSTORE_FLAGS_FTRACE	BIT(2)
 #define PSTORE_FLAGS_PMSG	BIT(3)
+#define PSTORE_FLAGS_DMESG_ALL	BIT(4)
 
 extern int pstore_register(struct pstore_info *);
 extern void pstore_unregister(struct pstore_info *);
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9cb9b9067298..f23c29cbd205 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -134,6 +134,7 @@ struct ramoops_platform_data {
 	unsigned long	ftrace_size;
 	unsigned long	pmsg_size;
 	int		dump_oops;
+	int		dump_all;
 	u32		flags;
 	struct persistent_ram_ecc_info ecc_info;
 };
-- 
2.25.1

[PATCH v1 3/3] ramoops: add dump_all optional field to ramoops DT node

[Pavel Tatashin]

Currently, it is possible to dump kmesges for panic, or oops.
With dump_all it is possible to dump messages for kmesg_dump events,
for example reboot, halt, shutdown, kexec.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 Documentation/devicetree/bindings/reserved-memory/ramoops.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
index 0eba562fe5c6..3ce424c9ad4c 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
@@ -47,5 +47,8 @@ Optional properties:
 
 - no-dump-oops: if present, only dump panics (defaults to panics and oops)
 
+- dump-all: if present, dump kernel messages during all kmesg dump events.
+  Reasons are specified in include/linux/kmsg_dump.h KMSG_DUMP_*
+
 - flags: if present, pass ramoops behavioral flags (defaults to 0,
   see include/linux/pstore_ram.h RAMOOPS_FLAG_* for flag values).
-- 
2.25.1

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Steven Rostedt]

On Sat,  2 May 2020 10:35:53 -0400
Pavel Tatashin <pasha.tatashin@soleen.com> wrote:

> kmsg_dump() allows to dump kmesg buffer for various system events: oops,
> panic, reboot, etc. It provides an interface to register a callback call
> for clients, and in that callback interface there is a field "max_reason"
> which gets ignored unless always_kmsg_dump is passed as kernel parameter.
> 
> Allow clients to decide max_reason, and keep the current behavior when
> max_reason is not set.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  include/linux/kmsg_dump.h |  1 +
>  kernel/printk/printk.c    | 16 +++++++++-------
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> index 2e7a1e032c71..c0d703b7ce38 100644
> --- a/include/linux/kmsg_dump.h
> +++ b/include/linux/kmsg_dump.h
> @@ -28,6 +28,7 @@ enum kmsg_dump_reason {
>  	KMSG_DUMP_RESTART,
>  	KMSG_DUMP_HALT,
>  	KMSG_DUMP_POWEROFF,
> +	KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF

Hmm, I didn't realize that enums were allowed to have duplicates. That can
usually screw up logic. I would recommend making that a define afterward.

#define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF

As is done in other locations of the kernel.


The rest looks fine to me.

-- Steve

>  };
>  
>  /**
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 9a9b6156270b..04c1e9a9b139 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3085,6 +3085,8 @@ EXPORT_SYMBOL(printk_timed_ratelimit);
>  
>  static DEFINE_SPINLOCK(dump_list_lock);
>  static LIST_HEAD(dump_list);
> +static bool always_kmsg_dump;
> +module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
>  
>  /**
>   * kmsg_dump_register - register a kernel log dumper.
> @@ -3106,6 +3108,12 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>  	spin_lock_irqsave(&dump_list_lock, flags);
>  	/* Don't allow registering multiple times */
>  	if (!dumper->registered) {
> +		if (!dumper->max_reason) {
> +			if (always_kmsg_dump)
> +				dumper->max_reason = KMSG_DUMP_MAX;
> +			else
> +				dumper->max_reason = KMSG_DUMP_OOPS;
> +		}
>  		dumper->registered = 1;
>  		list_add_tail_rcu(&dumper->list, &dump_list);
>  		err = 0;
> @@ -3141,9 +3149,6 @@ int kmsg_dump_unregister(struct kmsg_dumper *dumper)
>  }
>  EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
>  
> -static bool always_kmsg_dump;
> -module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
> -
>  /**
>   * kmsg_dump - dump kernel log to kernel message dumpers.
>   * @reason: the reason (oops, panic etc) for dumping
> @@ -3157,12 +3162,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
>  	struct kmsg_dumper *dumper;
>  	unsigned long flags;
>  
> -	if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
> -		return;
> -
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(dumper, &dump_list, list) {
> -		if (dumper->max_reason && reason > dumper->max_reason)
> +		if (reason > dumper->max_reason)
>  			continue;
>  
>  		/* initialize iterator with data about the stored records */

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Pavel Tatashin]

> Hmm, I didn't realize that enums were allowed to have duplicates. That can
> usually screw up logic. I would recommend making that a define afterward.
>
> #define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF
>
> As is done in other locations of the kernel.
>

Hi Steve,

Sure, I will change it to define.

Thank you,
Pasha

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Kees Cook]

On Mon, May 04, 2020 at 01:15:00PM -0400, Steven Rostedt wrote:
> On Sat,  2 May 2020 10:35:53 -0400
> Pavel Tatashin <pasha.tatashin@soleen.com> wrote:
> 
> > kmsg_dump() allows to dump kmesg buffer for various system events: oops,
> > panic, reboot, etc. It provides an interface to register a callback call
> > for clients, and in that callback interface there is a field "max_reason"
> > which gets ignored unless always_kmsg_dump is passed as kernel parameter.
> > 
> > Allow clients to decide max_reason, and keep the current behavior when
> > max_reason is not set.
> > 
> > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> > ---
> >  include/linux/kmsg_dump.h |  1 +
> >  kernel/printk/printk.c    | 16 +++++++++-------
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> > index 2e7a1e032c71..c0d703b7ce38 100644
> > --- a/include/linux/kmsg_dump.h
> > +++ b/include/linux/kmsg_dump.h
> > @@ -28,6 +28,7 @@ enum kmsg_dump_reason {
> >  	KMSG_DUMP_RESTART,
> >  	KMSG_DUMP_HALT,
> >  	KMSG_DUMP_POWEROFF,
> > +	KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
> 
> Hmm, I didn't realize that enums were allowed to have duplicates. That can
> usually screw up logic. I would recommend making that a define afterward.
> 
> #define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF
> 
> As is done in other locations of the kernel.

I've seen it also be the last item in an enum, then comparisons can just
do "< KMSG_DUMP_MAX" instead of "<= KMSG_DUMP_MAX".

-- 
Kees Cook

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Sat, May 02, 2020 at 10:35:52AM -0400, Pavel Tatashin wrote:
> Currently, ramoops is capable to collect dmesg buffer only during
> panic and oops events. However, it is desirable for shutdown performance
> analysis reasons to optionally allow collecting dmesg buffers during other
> events as well: reboot, kexec, emergency reboot etc.
> 
> How to quickly test:
> 
> virtme-run --mods=auto --kdir --mods=auto --kdir . \
> 	-a memmap=1G$8G -a ramoops.mem_address=0x200000000 \
> 	-a ramoops.mem_size=0x100000 -a ramoops.record_size=32768 \
> 	-a ramoops.dump_all=1 -a quiet --qemu-opts -m 8G
> ..
> # reboot -f
> 
> After VM is back:
> 
> # mount -t pstore pstore /mnt
> # head /mnt/dmesg-ramoops-0 
> Restart#1 Part1

Is there a reason that using ramoops.console_size isn't sufficient for
this?

I'm not strictly opposed to making these changes, but traditionally the
granularity of dmesg output has been pretty easily "all or crashes"
instead of a range within.

-- 
Kees Cook

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Pavel Tatashin]

Thank Kees, I think it is a little cleaner this way.

Thank you,
Pasha


On Mon, May 4, 2020 at 2:12 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 04, 2020 at 01:15:00PM -0400, Steven Rostedt wrote:
> > On Sat,  2 May 2020 10:35:53 -0400
> > Pavel Tatashin <pasha.tatashin@soleen.com> wrote:
> >
> > > kmsg_dump() allows to dump kmesg buffer for various system events: oops,
> > > panic, reboot, etc. It provides an interface to register a callback call
> > > for clients, and in that callback interface there is a field "max_reason"
> > > which gets ignored unless always_kmsg_dump is passed as kernel parameter.
> > >
> > > Allow clients to decide max_reason, and keep the current behavior when
> > > max_reason is not set.
> > >
> > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> > > ---
> > >  include/linux/kmsg_dump.h |  1 +
> > >  kernel/printk/printk.c    | 16 +++++++++-------
> > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> > > index 2e7a1e032c71..c0d703b7ce38 100644
> > > --- a/include/linux/kmsg_dump.h
> > > +++ b/include/linux/kmsg_dump.h
> > > @@ -28,6 +28,7 @@ enum kmsg_dump_reason {
> > >     KMSG_DUMP_RESTART,
> > >     KMSG_DUMP_HALT,
> > >     KMSG_DUMP_POWEROFF,
> > > +   KMSG_DUMP_MAX = KMSG_DUMP_POWEROFF
> >
> > Hmm, I didn't realize that enums were allowed to have duplicates. That can
> > usually screw up logic. I would recommend making that a define afterward.
> >
> > #define KMSG_DUMP_MAX KMSG_DUMP_POWEROFF
> >
> > As is done in other locations of the kernel.
>
> I've seen it also be the last item in an enum, then comparisons can just
> do "< KMSG_DUMP_MAX" instead of "<= KMSG_DUMP_MAX".
>
> --
> Kees Cook

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

> > # reboot -f
> >
> > After VM is back:
> >
> > # mount -t pstore pstore /mnt
> > # head /mnt/dmesg-ramoops-0
> > Restart#1 Part1
>
> Is there a reason that using ramoops.console_size isn't sufficient for
> this?

Unfortunately, the console option is not working for us (Microsoft),
we have an embedded device with a serial console, and the baud rate
reduces the reboot performance, so we must keep the console quiet. We
also want to be able collect full shutdown logs from the field that
are collected during kexec based updates.

>
> I'm not strictly opposed to making these changes, but traditionally the
> granularity of dmesg output has been pretty easily "all or crashes"
> instead of a range within.

As of now, ramoops  allows collecting dmesg only for oops and panic,
but not for all types of events.

Thank you,
Pasha

On Mon, May 4, 2020 at 2:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, May 02, 2020 at 10:35:52AM -0400, Pavel Tatashin wrote:
> > Currently, ramoops is capable to collect dmesg buffer only during
> > panic and oops events. However, it is desirable for shutdown performance
> > analysis reasons to optionally allow collecting dmesg buffers during other
> > events as well: reboot, kexec, emergency reboot etc.
> >
> > How to quickly test:
> >
> > virtme-run --mods=auto --kdir --mods=auto --kdir . \
> >       -a memmap=1G$8G -a ramoops.mem_address=0x200000000 \
> >       -a ramoops.mem_size=0x100000 -a ramoops.record_size=32768 \
> >       -a ramoops.dump_all=1 -a quiet --qemu-opts -m 8G
> > ..
> > # reboot -f
> >
> > After VM is back:
> >
> > # mount -t pstore pstore /mnt
> > # head /mnt/dmesg-ramoops-0
> > Restart#1 Part1
>
> Is there a reason that using ramoops.console_size isn't sufficient for
> this?
>
> I'm not strictly opposed to making these changes, but traditionally the
> granularity of dmesg output has been pretty easily "all or crashes"
> instead of a range within.
>
> --
> Kees Cook

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Mon, May 04, 2020 at 02:47:45PM -0400, Pavel Tatashin wrote:
> > > # reboot -f
> > >
> > > After VM is back:
> > >
> > > # mount -t pstore pstore /mnt
> > > # head /mnt/dmesg-ramoops-0
> > > Restart#1 Part1
> >
> > Is there a reason that using ramoops.console_size isn't sufficient for
> > this?
> 
> Unfortunately, the console option is not working for us (Microsoft),
> we have an embedded device with a serial console, and the baud rate
> reduces the reboot performance, so we must keep the console quiet. We
> also want to be able collect full shutdown logs from the field that
> are collected during kexec based updates.

I meant collecting console via pstore (i.e. /mnt/console-ramoops-0). Are
you saying that's still too large for your situation?

-- 
Kees Cook

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

On Mon, May 4, 2020 at 3:12 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 04, 2020 at 02:47:45PM -0400, Pavel Tatashin wrote:
> > > > # reboot -f
> > > >
> > > > After VM is back:
> > > >
> > > > # mount -t pstore pstore /mnt
> > > > # head /mnt/dmesg-ramoops-0
> > > > Restart#1 Part1
> > >
> > > Is there a reason that using ramoops.console_size isn't sufficient for
> > > this?
> >
> > Unfortunately, the console option is not working for us (Microsoft),
> > we have an embedded device with a serial console, and the baud rate
> > reduces the reboot performance, so we must keep the console quiet. We
> > also want to be able collect full shutdown logs from the field that
> > are collected during kexec based updates.
>
> I meant collecting console via pstore (i.e. /mnt/console-ramoops-0). Are
> you saying that's still too large for your situation?

pstore /mnt/console-ramoops-0 outputs only messages below the console
loglevel, and our console loglevel is set to 3 due to slowness of
serial console. Which means only errors and worse types of messages
are recorded. AFAIK, there is no way to have different log levels for
different consoles.

Thank you,
Pasha

>
> --
> Kees Cook

Re: [PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular reboot

[Kees Cook]

On Sat, May 02, 2020 at 10:35:54AM -0400, Pavel Tatashin wrote:
> Currently, ramoops is capable to collect dmesg buffer only during
> panic and oops events. However, it is desirable to optionally allow
> collecting dmesg buffers during other events as well: reboot, kexec,
> emergency reboot etc.
> 
> While, a similar functionality is provided by pstore console it is not the
> same. Often, console message level is reduced in production due to baud
> rate limitation of serial consoles.  Having a noisy console reduces the
> boot performance.
> 
> Thus, if the shutdown dmesg buffer is needed to study the shutdown
> performance, it is currently not possible to do so with console as by
> increasing the console output the shutdown time substantially increases
> as well.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  fs/pstore/platform.c       |  6 ++++--
>  fs/pstore/ram.c            | 38 +++++++++++++++++++++++++++-----------
>  include/linux/pstore.h     |  1 +
>  include/linux/pstore_ram.h |  1 +
>  4 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 408277ee3cdb..d0393799fe6c 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -476,8 +476,10 @@ static struct kmsg_dumper pstore_dumper = {
>  /*
>   * Register with kmsg_dump to save last part of console log on panic.
>   */
> -static void pstore_register_kmsg(void)
> +static void pstore_register_kmsg(int dmesg_all)
>  {
> +	if (dmesg_all)
> +		pstore_dumper.max_reason = KMSG_DUMP_MAX;

So, I'd like to avoid any new arguments in the API and instead add a new
field to struct pstore_info, which will be valid when PSTORE_FLAGS_DMESG
is set, and the max kdump reason can be set there by the pstore backends.

>  	kmsg_dump_register(&pstore_dumper);
>  }
>  
> @@ -603,7 +605,7 @@ int pstore_register(struct pstore_info *psi)
>  		pstore_get_records(0);
>  
>  	if (psi->flags & PSTORE_FLAGS_DMESG)
> -		pstore_register_kmsg();
> +		pstore_register_kmsg(psi->flags & PSTORE_FLAGS_DMESG_ALL);
>  	if (psi->flags & PSTORE_FLAGS_CONSOLE)
>  		pstore_register_console();
>  	if (psi->flags & PSTORE_FLAGS_FTRACE)
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 795622190c01..9d2d1b299225 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -62,6 +62,12 @@ module_param(dump_oops, int, 0600);
>  MODULE_PARM_DESC(dump_oops,
>  		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
>  
> +static int dump_all;
> +module_param(dump_all, int, 0600);
> +MODULE_PARM_DESC(dump_all,
> +		 "set to 1 to record all kernel kmesg dump events (default 0) "
> +		 "otherwise only panics and oops are recorded");

And instead of using a "dump_all" here, let's add a new field that is
the max reason. When both "max_reason" and "dump_oops" are defined,
"max_reason" should win.

> +
>  static int ramoops_ecc;
>  module_param_named(ecc, ramoops_ecc, int, 0600);
>  MODULE_PARM_DESC(ramoops_ecc,
> @@ -82,6 +88,7 @@ struct ramoops_context {
>  	size_t ftrace_size;
>  	size_t pmsg_size;
>  	int dump_oops;
> +	int dump_all;
>  	u32 flags;
>  	struct persistent_ram_ecc_info ecc_info;
>  	unsigned int max_dump_cnt;
> @@ -381,17 +388,19 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
>  	if (record->type != PSTORE_TYPE_DMESG)
>  		return -EINVAL;
>  
> -	/*
> -	 * Out of the various dmesg dump types, ramoops is currently designed
> -	 * to only store crash logs, rather than storing general kernel logs.
> -	 */
> -	if (record->reason != KMSG_DUMP_OOPS &&
> -	    record->reason != KMSG_DUMP_PANIC)
> -		return -EINVAL;
> +	if (!cxt->dump_all) {
> +		/*
> +		 * By default only store crash logs, rather than storing general
> +		 * kernel logs.
> +		 */
> +		if (record->reason != KMSG_DUMP_OOPS &&
> +		    record->reason != KMSG_DUMP_PANIC)
> +			return -EINVAL;

Then all these tests can be collapsed much more cleanly, etc.

>  
> -	/* Skip Oopes when configured to do so. */
> -	if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
> -		return -EINVAL;
> +		/* Skip Oopes when configured to do so. */
> +		if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
> +			return -EINVAL;
> +	}
>  
>  	/*
>  	 * Explicitly only take the first part of any new crash.
> @@ -688,6 +697,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  	pdata->mem_address = res->start;
>  	pdata->mem_type = of_property_read_bool(of_node, "unbuffered");
>  	pdata->dump_oops = !of_property_read_bool(of_node, "no-dump-oops");
> +	pdata->dump_all = of_property_read_bool(of_node, "dump-all");
>  
>  #define parse_size(name, field) {					\
>  		ret = ramoops_parse_dt_size(pdev, name, &value);	\
> @@ -786,6 +796,7 @@ static int ramoops_probe(struct platform_device *pdev)
>  	cxt->ftrace_size = pdata->ftrace_size;
>  	cxt->pmsg_size = pdata->pmsg_size;
>  	cxt->dump_oops = pdata->dump_oops;
> +	cxt->dump_all = pdata->dump_all;
>  	cxt->flags = pdata->flags;
>  	cxt->ecc_info = pdata->ecc_info;
>  
> @@ -828,8 +839,11 @@ static int ramoops_probe(struct platform_device *pdev)
>  	 * the single region size is how to check.
>  	 */
>  	cxt->pstore.flags = 0;
> -	if (cxt->max_dump_cnt)
> +	if (cxt->max_dump_cnt) {
>  		cxt->pstore.flags |= PSTORE_FLAGS_DMESG;
> +		if (cxt->dump_all)
> +			cxt->pstore.flags |= PSTORE_FLAGS_DMESG_ALL;
> +	}
>  	if (cxt->console_size)
>  		cxt->pstore.flags |= PSTORE_FLAGS_CONSOLE;
>  	if (cxt->max_ftrace_cnt)
> @@ -866,6 +880,7 @@ static int ramoops_probe(struct platform_device *pdev)
>  	mem_address = pdata->mem_address;
>  	record_size = pdata->record_size;
>  	dump_oops = pdata->dump_oops;
> +	dump_all = pdata->dump_all;
>  	ramoops_console_size = pdata->console_size;
>  	ramoops_pmsg_size = pdata->pmsg_size;
>  	ramoops_ftrace_size = pdata->ftrace_size;
> @@ -949,6 +964,7 @@ static void __init ramoops_register_dummy(void)
>  	pdata.ftrace_size = ramoops_ftrace_size;
>  	pdata.pmsg_size = ramoops_pmsg_size;
>  	pdata.dump_oops = dump_oops;
> +	pdata.dump_all = dump_all;
>  	pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
>  
>  	/*
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index e779441e6d26..32092c2d7224 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -195,6 +195,7 @@ struct pstore_info {
>  #define PSTORE_FLAGS_CONSOLE	BIT(1)
>  #define PSTORE_FLAGS_FTRACE	BIT(2)
>  #define PSTORE_FLAGS_PMSG	BIT(3)
> +#define PSTORE_FLAGS_DMESG_ALL	BIT(4)
>  
>  extern int pstore_register(struct pstore_info *);
>  extern void pstore_unregister(struct pstore_info *);
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 9cb9b9067298..f23c29cbd205 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -134,6 +134,7 @@ struct ramoops_platform_data {
>  	unsigned long	ftrace_size;
>  	unsigned long	pmsg_size;
>  	int		dump_oops;
> +	int		dump_all;
>  	u32		flags;
>  	struct persistent_ram_ecc_info ecc_info;
>  };
> -- 
> 2.25.1
> 

Thanks!

-- 
Kees Cook

Re: [PATCH v1 3/3] ramoops: add dump_all optional field to ramoops DT node

[Kees Cook]

On Sat, May 02, 2020 at 10:35:55AM -0400, Pavel Tatashin wrote:
> Currently, it is possible to dump kmesges for panic, or oops.
> With dump_all it is possible to dump messages for kmesg_dump events,
> for example reboot, halt, shutdown, kexec.

Please just collapse this into patch #2.

Thanks!

-Kees

> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  Documentation/devicetree/bindings/reserved-memory/ramoops.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
> index 0eba562fe5c6..3ce424c9ad4c 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
> @@ -47,5 +47,8 @@ Optional properties:
>  
>  - no-dump-oops: if present, only dump panics (defaults to panics and oops)
>  
> +- dump-all: if present, dump kernel messages during all kmesg dump events.
> +  Reasons are specified in include/linux/kmsg_dump.h KMSG_DUMP_*
> +
>  - flags: if present, pass ramoops behavioral flags (defaults to 0,
>    see include/linux/pstore_ram.h RAMOOPS_FLAG_* for flag values).
> -- 
> 2.25.1
> 

-- 
Kees Cook

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Mon, May 04, 2020 at 03:17:40PM -0400, Pavel Tatashin wrote:
> On Mon, May 4, 2020 at 3:12 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, May 04, 2020 at 02:47:45PM -0400, Pavel Tatashin wrote:
> > > > > # reboot -f
> > > > >
> > > > > After VM is back:
> > > > >
> > > > > # mount -t pstore pstore /mnt
> > > > > # head /mnt/dmesg-ramoops-0
> > > > > Restart#1 Part1
> > > >
> > > > Is there a reason that using ramoops.console_size isn't sufficient for
> > > > this?
> > >
> > > Unfortunately, the console option is not working for us (Microsoft),
> > > we have an embedded device with a serial console, and the baud rate
> > > reduces the reboot performance, so we must keep the console quiet. We
> > > also want to be able collect full shutdown logs from the field that
> > > are collected during kexec based updates.
> >
> > I meant collecting console via pstore (i.e. /mnt/console-ramoops-0). Are
> > you saying that's still too large for your situation?
> 
> pstore /mnt/console-ramoops-0 outputs only messages below the console
> loglevel, and our console loglevel is set to 3 due to slowness of
> serial console. Which means only errors and worse types of messages
> are recorded. AFAIK, there is no way to have different log levels for
> different consoles.

Ah-ha! Okay, thanks. Please include this rationale in the v2 changelog.
That makes perfect sense; I just didn't see it and maybe others will
need the same clarity too. Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v1 3/3] ramoops: add dump_all optional field to ramoops DT node

[Pavel Tatashin]

On Mon, May 4, 2020 at 3:29 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, May 02, 2020 at 10:35:55AM -0400, Pavel Tatashin wrote:
> > Currently, it is possible to dump kmesges for panic, or oops.
> > With dump_all it is possible to dump messages for kmesg_dump events,
> > for example reboot, halt, shutdown, kexec.
>
> Please just collapse this into patch #2.

Will do it.

Thank you,
Pasha

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

> > pstore /mnt/console-ramoops-0 outputs only messages below the console
> > loglevel, and our console loglevel is set to 3 due to slowness of
> > serial console. Which means only errors and worse types of messages
> > are recorded. AFAIK, there is no way to have different log levels for
> > different consoles.
>
> Ah-ha! Okay, thanks. Please include this rationale in the v2 changelog.
> That makes perfect sense; I just didn't see it and maybe others will
> need the same clarity too. Thanks!

I will add this explanation to v2.

Thank you,
Pasha

>
> -Kees
>
> --
> Kees Cook

Re: [PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular reboot

[Pavel Tatashin]

> > -static void pstore_register_kmsg(void)
> > +static void pstore_register_kmsg(int dmesg_all)
> >  {
> > +     if (dmesg_all)
> > +             pstore_dumper.max_reason = KMSG_DUMP_MAX;
>
> So, I'd like to avoid any new arguments in the API and instead add a new
> field to struct pstore_info, which will be valid when PSTORE_FLAGS_DMESG
> is set, and the max kdump reason can be set there by the pstore backends.

Hi Kees,

I am trying to verify that I understand the request correctly:

1. pstore_register_kmsg() -> remove argument.
2. pstore_info -> add a new field  max_kmsg_reason: contains the
actual reason value
3. Modify: pstore_register() to set this field in pstore_dumper prior
to calling pstore_register_kmsg().
4. remove ramoops.dump_all boolean parameter
5. add a new parameter ramoops.max_reason integer variable, which will
be set in pstore_register_kmsg
6. Modify other users of pstore_register() to provide the correct
max_kmsg_reason.

Is this correct?

Thank you,
Pasha

Re: [PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular reboot

[Kees Cook]

On Mon, May 04, 2020 at 04:30:52PM -0400, Pavel Tatashin wrote:
> > > -static void pstore_register_kmsg(void)
> > > +static void pstore_register_kmsg(int dmesg_all)
> > >  {
> > > +     if (dmesg_all)
> > > +             pstore_dumper.max_reason = KMSG_DUMP_MAX;
> >
> > So, I'd like to avoid any new arguments in the API and instead add a new
> > field to struct pstore_info, which will be valid when PSTORE_FLAGS_DMESG
> > is set, and the max kdump reason can be set there by the pstore backends.
> 
> Hi Kees,
> 
> I am trying to verify that I understand the request correctly:
> 
> 1. pstore_register_kmsg() -> remove argument.

Yes (or, from the perspective of what v2 will look like, "do not add
an argument to pstore_register_kmsg()").

> 2. pstore_info -> add a new field  max_kmsg_reason: contains the
> actual reason value

Let's just call it max_reason, but yes. And perhaps instead of adding
KMSG_DUMP_MAX, maybe just use INT_MAX or something for "all reasons".

> 3. Modify: pstore_register() to set this field in pstore_dumper prior
> to calling pstore_register_kmsg().

Correct.

> 4. remove ramoops.dump_all boolean parameter

Yes, or more specifically, "don't add ramoops.dump_all".

> 5. add a new parameter ramoops.max_reason integer variable, which will
> be set in pstore_register_kmsg

Right, though this will likely require some refactoring of the existing
handling of the dump_oops parameter, likely as a separate patch, since
we should not remove the parameter, as some systems may be expecting to
use it still. But it should be reworked in terms of the new max_reason.

> 6. Modify other users of pstore_register() to provide the correct
> max_kmsg_reason.

Yes, which should be a trivial adjustment. You can look for all the
initializers using PSTORE_FLAGS_DMESG:

arch/powerpc/kernel/nvram_64.c: .flags = PSTORE_FLAGS_DMESG,
drivers/acpi/apei/erst.c:       .flags          = PSTORE_FLAGS_DMESG,
drivers/firmware/efi/efi-pstore.c:      .flags          = PSTORE_FLAGS_DMESG,

It looks like all the other backends actually already dump all reasons,
so they should likely all be set to the INT_MAX, or whatever is chosen
for "all reasons".

> 
> Is this correct?

But, yes, your list essentially matches what I've got in my head too. :)

-- 
Kees Cook

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Sergey Senozhatsky]

On (20/05/02 10:35), Pavel Tatashin wrote:
[..]
> +static bool always_kmsg_dump;
> +module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
>  
>  /**
>   * kmsg_dump_register - register a kernel log dumper.
> @@ -3106,6 +3108,12 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>  	spin_lock_irqsave(&dump_list_lock, flags);
>  	/* Don't allow registering multiple times */
>  	if (!dumper->registered) {
> +		if (!dumper->max_reason) {
> +			if (always_kmsg_dump)
> +				dumper->max_reason = KMSG_DUMP_MAX;
> +			else
> +				dumper->max_reason = KMSG_DUMP_OOPS;
> +		}
>  		dumper->registered = 1;
>  		list_add_tail_rcu(&dumper->list, &dump_list);
>  		err = 0;

[..]

> @@ -3157,12 +3162,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
>  	struct kmsg_dumper *dumper;
>  	unsigned long flags;
>  
> -	if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
> -		return;
> -
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(dumper, &dump_list, list) {
> -		if (dumper->max_reason && reason > dumper->max_reason)
> +		if (reason > dumper->max_reason)
>  			continue;

Why always_kmsg_dump check moved from the dumper loop entry point to the
dumper registration code? What if the user change always_ksmsg_dump
dynamically via sysfs?

	-ss

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Pavel Tatashin]

> > @@ -3157,12 +3162,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
> >       struct kmsg_dumper *dumper;
> >       unsigned long flags;
> >
> > -     if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
> > -             return;
> > -
> >       rcu_read_lock();
> >       list_for_each_entry_rcu(dumper, &dump_list, list) {
> > -             if (dumper->max_reason && reason > dumper->max_reason)
> > +             if (reason > dumper->max_reason)
> >                       continue;
>
> Why always_kmsg_dump check moved from the dumper loop entry point to the
> dumper registration code? What if the user change always_ksmsg_dump
> dynamically via sysfs?

Hi Sergey,

I changed it to make code cleaner:  for such basic operation there are
too many conditions if we will keep it inside the kmsg_dump().
However, if being able to set always_kmsg_dump dynamically during
runtime is deemed important, I can change it back to be checked in
kmsg_dump.

Thank you,
Pasha

>
>         -ss

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Pavel Tatashin]

On Mon, May 4, 2020 at 10:52 PM Pavel Tatashin
<pasha.tatashin@soleen.com> wrote:
>
> > > @@ -3157,12 +3162,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
> > >       struct kmsg_dumper *dumper;
> > >       unsigned long flags;
> > >
> > > -     if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
> > > -             return;
> > > -
> > >       rcu_read_lock();
> > >       list_for_each_entry_rcu(dumper, &dump_list, list) {
> > > -             if (dumper->max_reason && reason > dumper->max_reason)
> > > +             if (reason > dumper->max_reason)
> > >                       continue;
> >
> > Why always_kmsg_dump check moved from the dumper loop entry point to the
> > dumper registration code? What if the user change always_ksmsg_dump
> > dynamically via sysfs?
>
> Hi Sergey,
>
> I changed it to make code cleaner:  for such basic operation there are
> too many conditions if we will keep it inside the kmsg_dump().
> However, if being able to set always_kmsg_dump dynamically during
> runtime is deemed important, I can change it back to be checked in
> kmsg_dump.

If you agree that we do not have to modify this variable dynamically,
I will also change the permission here:
module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);

>
> Thank you,
> Pasha
>
> >
> >         -ss

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Pavel Tatashin]

> > I changed it to make code cleaner:  for such basic operation there are
> > too many conditions if we will keep it inside the kmsg_dump().
> > However, if being able to set always_kmsg_dump dynamically during
> > runtime is deemed important, I can change it back to be checked in
> > kmsg_dump.
>
> If you agree that we do not have to modify this variable dynamically,
> I will also change the permission here:
> module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);

Hi Sergey,

After thinking about this. I will move this logic back to kmsg_dump(),
to keep the current behavior where kmsg_dump can be modified during
runtime.

Thank you,
Pasha

Re: [PATCH v1 1/3] printk: honor the max_reason field in kmsg_dumper

[Sergey Senozhatsky]

On (20/05/05 00:21), Pavel Tatashin wrote:
> > > I changed it to make code cleaner:  for such basic operation there are
> > > too many conditions if we will keep it inside the kmsg_dump().
> > > However, if being able to set always_kmsg_dump dynamically during
> > > runtime is deemed important, I can change it back to be checked in
> > > kmsg_dump.
> >
> > If you agree that we do not have to modify this variable dynamically,
> > I will also change the permission here:
> > module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
> 
> Hi Sergey,

Hi Pavel,

> After thinking about this. I will move this logic back to kmsg_dump(),
> to keep the current behavior where kmsg_dump can be modified during
> runtime.

Agreed. I think that sysfs knobs and user-visible API need to preserve
their behaviour. There is a deprecation protocol, but usually it takes
many years of WARN_ON() and pr_err("this knob will be removed") before
we can change anything. E.g. sysctl has been deprecated for about a
decade IIRC before it was actually removed.

	-ss

Re: [PATCH v1 0/3] allow ramoops to collect all kmesg_dump events

[Sergey Senozhatsky]

On (20/05/04 15:17), Pavel Tatashin wrote:
> AFAIK, there is no way to have different log levels for
> different consoles.

There was a patch set from facebook several years ago to make
loglevels per-console, but it didn't land. Perhaps we need to
refresh it.

	-ss

Re: [PATCH v1 3/3] ramoops: add dump_all optional field to ramoops DT node

[Pavel Tatashin]

Hi Kees,

According to Documentation/devicetree/bindings/submitting-patches.txt,
DT binding docs should be separate patches; checkpath.pl also
complaints about it. I will keep it as a separate patch in v2.

Pasha

On Mon, May 4, 2020 at 4:00 PM Pavel Tatashin <pasha.tatashin@soleen.com> wrote:
>
> On Mon, May 4, 2020 at 3:29 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Sat, May 02, 2020 at 10:35:55AM -0400, Pavel Tatashin wrote:
> > > Currently, it is possible to dump kmesges for panic, or oops.
> > > With dump_all it is possible to dump messages for kmesg_dump events,
> > > for example reboot, halt, shutdown, kexec.
> >
> > Please just collapse this into patch #2.
>
> Will do it.
>
> Thank you,
> Pasha