On Fri, Jun 14, 2024 at 06:03:23PM +0200, Daniel Kiper wrote:
> On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> > The fdtdump command allows dumping arbitrary device tree properties
> > and saving them to a variable similar to the smbios command.
> >
> > This is useful in scripts where further actions such as selecting a
> > kernel or loading another device tree depend on the compatible or
> > model values of the device tree provided by the firmware.
> >
> > For now only the root level properties of the dtb are exposed.
> >
> > Signed-off-by: Tobias Heider <tobias.hei...@canonical.com>
> > ---
> > grub-core/loader/efi/fdt.c | 51 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > index 439964b9c..8fa0b3b09 100644
> > --- a/grub-core/loader/efi/fdt.c
> > +++ b/grub-core/loader/efi/fdt.c
> > @@ -1,6 +1,7 @@
> > /*
> > * GRUB -- GRand Unified Bootloader
> > * Copyright (C) 2013-2015 Free Software Foundation, Inc.
> > + * Copyright (C) 2024 Canonical, Ltd.
> > *
> > * GRUB is free software: you can redistribute it and/or modify
> > * it under the terms of the GNU General Public License as published by
> > @@ -18,15 +19,18 @@
> >
> > #include <grub/fdt.h>
> > #include <grub/mm.h>
> > +#include <grub/env.h>
> > #include <grub/err.h>
> > #include <grub/dl.h>
> > #include <grub/command.h>
> > +#include <grub/extcmd.h>
> > #include <grub/file.h>
> > #include <grub/efi/efi.h>
> > #include <grub/efi/fdtload.h>
> > #include <grub/efi/memory.h>
> > #include <grub/cpu/efi/memory.h>
> >
> > +static void *fw_fdt;
> > static void *loaded_fdt;
> > static void *fdt;
> >
> > @@ -36,6 +40,13 @@ static void *fdt;
> > sizeof (FDT_ADDR_CELLS_STRING) + \
> > sizeof (FDT_SIZE_CELLS_STRING))
> >
> > +static const struct grub_arg_option options_fdtdump[] = {
> > + {"prop", 'p', 0, N_("Get property."), N_("prop"), ARG_TYPE_STRING},
> > + {"set", '\0', 0, N_("Store the value in the given variable name."),
> > + N_("variable"), ARG_TYPE_STRING},
> > + {0, 0, 0, 0, 0, 0}
> > +};
> > +
> > void *
> > grub_fdt_load (grub_size_t additional_size)
> > {
> > @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
> > if (loaded_fdt)
> > raw_fdt = loaded_fdt;
> > else
> > - raw_fdt = grub_efi_get_firmware_fdt();
> > + raw_fdt = fw_fdt;
>
> This change seems suspicious for me. Could you explain why it is needed?
Since I added grub_efi_get_firmware_fdt() to module init function and the
device tree passed by the firmware is a fairly static property it made
sense to me to use the address we have instead of rereading it in
grub_fdt_load().
If you are more comfortable if I don't touch that code path I can drop that
change and simply read it twice.
>
> > if (raw_fdt)
> > size = grub_fdt_get_totalsize (raw_fdt);
> > @@ -167,10 +178,47 @@ out:
> > return grub_errno;
> > }
> >
> > +static grub_err_t
> > +grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> > + int argc __attribute__ ((unused)),
> > + char **argv __attribute__ ((unused)))
> > +{
> > + struct grub_arg_list *state = ctxt->state;
> > + const char *value = NULL;
> > +
> > + if (fw_fdt == NULL)
> > + return grub_error (GRUB_ERR_IO,
> > + N_("No device tree found"));
> > +
> > + if (state[0].set)
> > + {
> > + value = grub_fdt_get_prop(fw_fdt, 0, state[0].arg, NULL);
>
> Missing space before "(".
>
> > + }
>
> Please drop redundant curly braces.
Thanks for the review, I'll send an update with those fixed and the two commits
merged.
>
> > + if (value == NULL)
> > + return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > + N_("failed to retrieve the prop field"));
> > +
> > + if (state[1].set)
> > + grub_env_set (state[1].arg, value);
> > + else
> > + grub_printf ("%s\n", value);
> > +
> > + return GRUB_ERR_NONE;
> > +}
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
