lkml.org 
[lkml]   [2020]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular reboot
    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

    \
     
     \ /
      Last update: 2020-05-04 21:29    [W:5.086 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site