Messages in this thread | | | Date | Mon, 4 May 2020 13:54:51 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular reboot |
| |
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
| |