| Message ID | 20221006224212.569555-8-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some pstore improvements | expand |
On Thu, Oct 06, 2022 at 07:42:11PM -0300, Guilherme G. Piccoli wrote: > For some reason, the efi-pstore backend name (exposed through the > pstore infrastructure) is hardcoded as "efi", whereas all the other > backends follow a kind of convention in using the module name. > > Let's do it here as well, to make user's life easier (they might > use this info for unloading the module backend, for example). > > Cc: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> Looks fine to me. Ard, if you don't object, I can carry this in the pstore tree.
On Fri, 7 Oct 2022 at 01:16, Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 06, 2022 at 07:42:11PM -0300, Guilherme G. Piccoli wrote: > > For some reason, the efi-pstore backend name (exposed through the > > pstore infrastructure) is hardcoded as "efi", whereas all the other > > backends follow a kind of convention in using the module name. > > > > Let's do it here as well, to make user's life easier (they might > > use this info for unloading the module backend, for example). > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > > Looks fine to me. Ard, if you don't object, I can carry this in the > pstore tree. > Acked-by: Ard Biesheuvel <ardb@kernel.org>
On Thu, 6 Oct 2022 19:42:11 -0300, Guilherme G. Piccoli wrote: > For some reason, the efi-pstore backend name (exposed through the > pstore infrastructure) is hardcoded as "efi", whereas all the other > backends follow a kind of convention in using the module name. > > Let's do it here as well, to make user's life easier (they might > use this info for unloading the module backend, for example). > > [...] Applied to for-next/pstore, thanks! [7/8] efi: pstore: Follow convention for the efi-pstore backend name https://git.kernel.org/kees/c/39bae0ee0656
Quoting Guilherme G. Piccoli (2022-10-06 15:42:11) > For some reason, the efi-pstore backend name (exposed through the > pstore infrastructure) is hardcoded as "efi", whereas all the other > backends follow a kind of convention in using the module name. > > Let's do it here as well, to make user's life easier (they might > use this info for unloading the module backend, for example). This patch broke ChromeOS' crash reporter when running on EFI[1], which luckily isn't the typical mode of operation for Chromebooks. The problem was that we had hardcoded something like dmesg-efi-<number> into the regex logic that finds EFI pstore records. I didn't write the original code but I think the idea was to speed things up by parsing the filenames themselves to collect the files related to a crash record instead of opening and parsing the header from the files to figure out which file corresponds to which record. I suspect the fix is pretty simple (make the driver name match either one via a regex) but I just wanted to drop a note here that this made some lives harder, not easier. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > --- > drivers/firmware/efi/efi-pstore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c > index 3bddc152fcd4..97a9e84840a0 100644 > --- a/drivers/firmware/efi/efi-pstore.c > +++ b/drivers/firmware/efi/efi-pstore.c > @@ -207,7 +207,7 @@ static int efi_pstore_erase(struct pstore_record *record) > > static struct pstore_info efi_pstore_info = { > .owner = THIS_MODULE, > - .name = "efi", > + .name = KBUILD_MODNAME, > .flags = PSTORE_FLAGS_DMESG, > .open = efi_pstore_open, > .close = efi_pstore_close, [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/crash-reporter/kernel_collector.cc;l=54;drc=7a522166f0b2b32ece60f520b5d3d571c7545b0b
On 03/06/2024 20:02, Stephen Boyd wrote: > [...] > This patch broke ChromeOS' crash reporter when running on EFI[1], which > luckily isn't the typical mode of operation for Chromebooks. The problem > was that we had hardcoded something like dmesg-efi-<number> into the > regex logic that finds EFI pstore records. I didn't write the original > code but I think the idea was to speed things up by parsing the > filenames themselves to collect the files related to a crash record > instead of opening and parsing the header from the files to figure out > which file corresponds to which record. > > I suspect the fix is pretty simple (make the driver name match either > one via a regex) but I just wanted to drop a note here that this made > some lives harder, not easier. Oh, many apologies for that Stephen - of course if I was aware of the hardcoding in the tool, I'd not mess it up or would fix the tooling first, before changing the kernel code. At least, as a bright side here you found the tool's limitation and there's the obvious improvement/fix for that =) Cheers, Guilherme
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index 3bddc152fcd4..97a9e84840a0 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -207,7 +207,7 @@ static int efi_pstore_erase(struct pstore_record *record) static struct pstore_info efi_pstore_info = { .owner = THIS_MODULE, - .name = "efi", + .name = KBUILD_MODNAME, .flags = PSTORE_FLAGS_DMESG, .open = efi_pstore_open, .close = efi_pstore_close,
For some reason, the efi-pstore backend name (exposed through the pstore infrastructure) is hardcoded as "efi", whereas all the other backends follow a kind of convention in using the module name. Let's do it here as well, to make user's life easier (they might use this info for unloading the module backend, for example). Cc: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- drivers/firmware/efi/efi-pstore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)