Re: [PATCH] ramoops: provide panic data even in suspend

[Petr Mladek <pmladek@suse.com>]

On Thu 2015-08-06 22:37:15, Dongdong Yang wrote:
> >From 6b28004d7b3144d4d26d60fe0bf6a750bbab39f6 Mon Sep 17 00:00:00 2001
> From: yangdongdong <yangdongdong@xiaomi.com>
> 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 <yangdongdong@xiaomi.com>
> Signed-off-by: gulinghua <gulinghua@xiaomi.com>
> Signed-off-by: duhui <duhui@xiaomi.com>
> ---
>  fs/pstore/ram.c            | 16 ++++++++++++++++
>  include/linux/pstore_ram.h |  1 +
>  kernel/printk/printk.c     |  7 +++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 6c26c4d..3d981a1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -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);
> 
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 9c9d6c1..826a35b 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -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);
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index cf8c242..cfa22f5 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -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.

> +}
> +EXPORT_SYMBOL(emergency_unlock_console);
> +
>  /**
>   * console_cpu_notify - print deferred console messages after CPU hotplug
>   * @self: notifier struct
> -- 
> 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