Skip to contentSkip to conversation

ramoops: provide panic data even in suspend

PATCH In review [email protected] 2 participants 3 messages Aug 5, 2015 – Aug 11, 2015 1 message not in the archive

Subsystems (2)
Printk Pstore filesystem
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 ramoops: provide panic data even in suspend

From: yangdongdong <[email protected]>

Good comments. It is valuable for code readable. Here is not necessary wrap atomic_notifier_chain_register into ramoops_prepare. The updated patch as below:

Quoted 1 line

From 5ec3976e2b55dfaf8bd1bc88c5a9c05762148fef Mon Sep 17 00:00:00 2001

From: yangdongdong <[email protected]>
Date: Wed, 5 Aug 2015 18:51:17 +0800
Subject: [PATCH] ramoops: provide panic data even in suspend

This also enables panic and oops messages which in suspend context to be logged into ramoops console buffer where it can be read back at some later point.

Signed-off-by yangdongdong <[email protected]>
Signed-off-by gulinghua <[email protected]>
---
 fs/pstore/ram.c            | 16 ++++++++++++++++
 include/linux/pstore_ram.h |  1 +
 kernel/printk/printk.c     |  6 ++++++
 3 files changed, 23 insertions(+)
fs/pstore/ram.c +16 -0
index 6c26c4d..3d981a1 100644
@@ -642,8 +642,23 @@ static void ramoops_register_dummy(void)
 	}
 }
 
+static int ramoops_console_notify(struct notifier_block *this,
+		unsigned long event, void *ptr)
+{
+	pr_emerg("ramoops unlock console ...\n");
+	emergency_unlock_console();
+
+	return 0;
+}
+
+static struct notifier_block ramoop_nb = {
+	.notifier_call = ramoops_console_notify,
+	.priority = INT_MAX,
+};
+
 static int __init ramoops_init(void)
 {
+	atomic_notifier_chain_register(&panic_notifier_list, &ramoop_nb);
 	ramoops_register_dummy();
 	return platform_driver_register(&ramoops_driver);
 }

@@ -654,6 +669,7 @@ static void __exit ramoops_exit(void)
 	platform_driver_unregister(&ramoops_driver);
 	platform_device_unregister(dummy);
 	kfree(dummy_data);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &ramoop_nb);
 }
 module_exit(ramoops_exit);
include/linux/pstore_ram.h +1 -0
index 9c9d6c1..826a35b 100644
@@ -52,6 +52,7 @@ struct persistent_ram_zone {
 	size_t old_log_size;
 };
 
+extern void emergency_unlock_console(void);
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 			u32 sig, struct persistent_ram_ecc_info *ecc_info,
 			unsigned int memtype);
kernel/printk/printk.c +6 -0
index cf8c242..ece645c 100644
@@ -2107,6 +2107,12 @@ void resume_console(void)
 	console_unlock();
 }
 
+void emergency_unlock_console(void)
+{
+	resume_console();
+}
+EXPORT_SYMBOL(emergency_unlock_console);
+
 /**
  * console_cpu_notify - print deferred console messages after CPU hotplug
  * @self: notifier struct
Signature · 11 lines
2.5.0

Thanks,
Andy

-----邮件原件-----
发件人: Alex Elder [mailto:[email protected]]
发送时间: 2015年8月5日 21:23
收件人: [email protected]; Anton Vorontsov; Colin Cross; Kees Cook; Tony Luck; Andrew Morton; Petr Mladek; Luis R. Rodriguez; Peter Hurley; Joe Perches; Tejun Heo; 杜慧; 顾凌华
抄送: [email protected]; 杨冬东
主题: Re: [PATCH] fs/pstore: provide panic data even in suspend
Quoted 8 lines · On 08/05/2015 08:11 AM, [email protected] wrote:

From: yangdongdong <[email protected]>

This also enables panic and oops messages which in suspend context to be logged into ramoops console buffer where it can be read back at some later point.

Signed-off-by yangdongdong <[email protected]>
Signed-off-by gulinghua <[email protected]>

I have a small comment on the code below.

-Alex

Quoted 39 lines
---
 fs/pstore/ram.c            | 21 +++++++++++++++++++++
 include/linux/pstore_ram.h |  1 +
 kernel/printk/printk.c     |  6 ++++++
 3 files changed, 28 insertions(+)
fs/pstore/ram.c index 6c26c4d..f84c5ab renamed from fs/pstore/ram.c +0 -0

100644

fs/pstore/ram.c +19 -0
@@ -642,8 +642,28 @@ static void ramoops_register_dummy(void)
       }
 }
 
+static int ramoops_console_notify(struct notifier_block *this,
+             unsigned long event, void *ptr)
+{
+     pr_emerg("ramoops unlock console ...\n");
+     emergency_unlock_console();
+
+     return 0;
+}
+
+static struct notifier_block ramoop_nb = {
+     .notifier_call = ramoops_console_notify,
+     .priority = INT_MAX,
+};
+
+static void ramoops_prepare(void)
+{
+     atomic_notifier_chain_register(&panic_notifier_list, &ramoop_nb); }
+
 static int __init ramoops_init(void)
 {
+     ramoops_prepare();
       ramoops_register_dummy();
       return platform_driver_register(&ramoops_driver);
 }

fs/pstore/ram.c static void __exit ramoops_exit(void) · line 677

Quoted 4 lines
fs/pstore/ram.c +1 -0
@@ -654,6 +674,7 @@ static void __exit ramoops_exit(void)
       platform_driver_unregister(&ramoops_driver);
       platform_device_unregister(dummy);
       kfree(dummy_data);
+     atomic_notifier_chain_unregister(&panic_notifier_list, &ramoop_nb);

I would say either wrap this atomic_notificer_chain_unregister()
call in a trivial function ramoops_unprepare(), or (preferably) get rid of ramoops_prepare().

Quoted 33 lines
fs/pstore/ram.c +0 -0
 }
 module_exit(ramoops_exit);
include/linux/pstore_ram.h +1 -0
index 9c9d6c1..826a35b 100644
@@ -52,6 +52,7 @@ struct persistent_ram_zone {
       size_t old_log_size;
 };
 
+extern void emergency_unlock_console(void);
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
                       u32 sig, struct persistent_ram_ecc_info *ecc_info,
                       unsigned int memtype);
kernel/printk/printk.c index renamed from kernel/printk/printk.c +0 -0

cf8c242..ece645c 100644

kernel/printk/printk.c +6 -0
@@ -2107,6 +2107,12 @@ void resume_console(void)
       console_unlock();
 }
 
+void emergency_unlock_console(void)
+{
+     resume_console();
+}
+EXPORT_SYMBOL(emergency_unlock_console);
+
 /**
  * console_cpu_notify - print deferred console messages after CPU hotplug
  * @self: notifier struct
maintainer
#1 lore raw
Quoted 26 lines · On Wed 2015-08-05 21:56:07, [email protected] wrote:

From: yangdongdong <[email protected]>

Good comments. It is valuable for code readable. Here is not necessary wrap atomic_notifier_chain_register into ramoops_prepare. The updated patch as below:

Quoted 1 line

From 5ec3976e2b55dfaf8bd1bc88c5a9c05762148fef Mon Sep 17 00:00:00 2001

From: yangdongdong <[email protected]>
Date: Wed, 5 Aug 2015 18:51:17 +0800
Subject: [PATCH] ramoops: provide panic data even in suspend

This also enables panic and oops messages which in suspend context to be logged into ramoops console buffer where it can be read back at some later point.

Signed-off-by yangdongdong <[email protected]>
Signed-off-by gulinghua <[email protected]>
---
 fs/pstore/ram.c            | 16 ++++++++++++++++
 include/linux/pstore_ram.h |  1 +
 kernel/printk/printk.c     |  6 ++++++
 3 files changed, 23 insertions(+)
fs/pstore/ram.c +0 -0
index 6c26c4d..3d981a1 100644

fs/pstore/ram.c static void ramoops_register_dummy(void) · line 649

Quoted 8 lines
fs/pstore/ram.c +5 -0
@@ -642,8 +642,23 @@ static void ramoops_register_dummy(void)
 	}
 }
 
+static int ramoops_console_notify(struct notifier_block *this,
+		unsigned long event, void *ptr)
+{
+	pr_emerg("ramoops unlock console ...\n");
+	emergency_unlock_console();

IMHO, you need to make sure that this is called only when the console is suspended.

Well, the panic might come at any time, for example when the console is being suspended. In this case, you might not be able to take the console semaphore and you might break panic().

A solution might be to call zap_locks(). But Peter Zijlstra is against using zap_locks() because it might cause losing messages. He suggests to print to safe consoles directly, something like the x86 early_serial_console.

Best Regards,
Petr

Quoted 57 lines
fs/pstore/ram.c +11 -0
+
+	return 0;
+}
+
+static struct notifier_block ramoop_nb = {
+	.notifier_call = ramoops_console_notify,
+	.priority = INT_MAX,
+};
+
 static int __init ramoops_init(void)
 {
+	atomic_notifier_chain_register(&panic_notifier_list, &ramoop_nb);
 	ramoops_register_dummy();
 	return platform_driver_register(&ramoops_driver);
 }

@@ -654,6 +669,7 @@ static void __exit ramoops_exit(void)
 	platform_driver_unregister(&ramoops_driver);
 	platform_device_unregister(dummy);
 	kfree(dummy_data);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &ramoop_nb);
 }
 module_exit(ramoops_exit);
include/linux/pstore_ram.h +1 -0
index 9c9d6c1..826a35b 100644
@@ -52,6 +52,7 @@ struct persistent_ram_zone {
 	size_t old_log_size;
 };
 
+extern void emergency_unlock_console(void);
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 			u32 sig, struct persistent_ram_ecc_info *ecc_info,
 			unsigned int memtype);
kernel/printk/printk.c +6 -0
index cf8c242..ece645c 100644
@@ -2107,6 +2107,12 @@ void resume_console(void)
 	console_unlock();
 }
 
+void emergency_unlock_console(void)
+{
+	resume_console();
+}
+EXPORT_SYMBOL(emergency_unlock_console);
+
 /**
  * console_cpu_notify - print deferred console messages after CPU hotplug
  * @self: notifier struct
Signature · 4 lines
2.5.0

Thanks,
Andy

A message that is not in the archive (sent off-list or not yet received): CAEG8FrHTbsJ8vwLAi_dcQ8q3axGtko=hkZ5p7X=TK7=HTro5Ag@mail.gmail.com

maintainer
#3 lore raw
Quoted 73 lines · Dongdong Yang
Quoted 1 line

From 6b28004d7b3144d4d26d60fe0bf6a750bbab39f6 Mon Sep 17 00:00:00 2001

From: yangdongdong <[email protected]>
Date: Wed, 5 Aug 2015 18:51:17 +0800
Subject: [PATCH] fs/pstore: provide panic data even in suspend

If system restart after panic, this patch also enables panic and oops messages which in suspend context to be logged into ramoops console buffer where it can be read back at some later point.

Signed-off-by yangdongdong <[email protected]>
Signed-off-by gulinghua <[email protected]>
Signed-off-by duhui <[email protected]>
---
 fs/pstore/ram.c            | 16 ++++++++++++++++
 include/linux/pstore_ram.h |  1 +
 kernel/printk/printk.c     |  7 +++++++
 3 files changed, 24 insertions(+)
fs/pstore/ram.c +16 -0
index 6c26c4d..3d981a1 100644
@@ -642,8 +642,23 @@ static void ramoops_register_dummy(void)
        }
 }
 
+static int ramoops_console_notify(struct notifier_block *this,
+               unsigned long event, void *ptr)
+{
+       pr_emerg("ramoops unlock console ...\n");
+       emergency_unlock_console();
+
+       return 0;
+}
+
+static struct notifier_block ramoop_nb = {
+       .notifier_call = ramoops_console_notify,
+       .priority = INT_MAX,
+};
+
 static int __init ramoops_init(void)
 {
+       atomic_notifier_chain_register(&panic_notifier_list, &ramoop_nb);
        ramoops_register_dummy();
        return platform_driver_register(&ramoops_driver);
 }

@@ -654,6 +669,7 @@ static void __exit ramoops_exit(void)
        platform_driver_unregister(&ramoops_driver);
        platform_device_unregister(dummy);
        kfree(dummy_data);
+       atomic_notifier_chain_unregister(&panic_notifier_list, &ramoop_nb);
 }
 module_exit(ramoops_exit);
include/linux/pstore_ram.h +1 -0
index 9c9d6c1..826a35b 100644
@@ -52,6 +52,7 @@ struct persistent_ram_zone {
        size_t old_log_size;
 };
 
+extern void emergency_unlock_console(void);
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t
 size,
                        u32 sig, struct persistent_ram_ecc_info *ecc_info,

                        unsigned int memtype);
kernel/printk/printk.c +0 -0
index cf8c242..cfa22f5 100644

kernel/printk/printk.c void resume_console(void) · line 2113

Quoted 7 lines
kernel/printk/printk.c +4 -0
@@ -2107,6 +2107,13 @@ void resume_console(void)
        console_unlock();
 }
 
+void emergency_unlock_console(void)
+{
+       if (panic_timeout != 0)
+               resume_console();

Could you please explain the meaning of this test? I do not get it.

If you want to resume a potentially suspended console, you need to check if the console is really suspended.

Quoted 18 lines
kernel/printk/printk.c +3 -0
+}
+EXPORT_SYMBOL(emergency_unlock_console);
+
 /**
  * console_cpu_notify - print deferred console messages after CPU hotplug
  * @self: notifier struct
Signature · 11 lines
2.5.0


The above PATCH-v2 with following update:
        - Change subject "[PATCH] ramoops" to "[PATCH-v2] fs/pstore"
        - Add panic_timeout as emergency_unlock condition
        - Add Ethan Du to signed-off list

For Petr's concern on panic break, here adds panic_timeout check as
emergency_unlock_console condition. Because our original intention is
collecting suspend panic debugging information before system reboot.

I am afraid that you are too focused on your scenario and do not see other possibilities. Suspend is only one state of the system. The panic might happen at any time. Or did I miss anything?

Best Regards,
Petr