[PATCH v2 3/3] fdt: introduce fsapply command

[Simon Glass]

Hi Andre,

On Mon, 13 Feb 2023 at 10:26, Andre Przywara <andre.przywara at arm.com> wrote:
>
> On Fri, 10 Feb 2023 09:05:34 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
> Hi Simon,
>
> > On Fri, 10 Feb 2023 at 04:32, Lothar Waßmann <LW at karo-electronics.de> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, 10 Feb 2023 11:02:13 +0000 Andre Przywara wrote:
> > > > Explicitly specifying the exact filenames of devicetree overlays files
> > > > on a U-Boot command line can be quite tedious for users, especially
> > > > when it should be made persistent for every boot.
> > > >
> > > > To simplify the task of applying (custom) DT overlays, introduce a
> > > > "fdt fsapply" subcommand, that iterates a given directory in any
> > > > supported filesystem, and tries to apply every .dtbo file found it
> > > > there.
> > > >
> > > > This allows users to simply drop a DT overlay file into a magic
> > > > directory, and it will be applied on the next boot automatically,
> > > > by the virtue of just a generic U-Boot command call.
> > > >
> > > > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > > > ---
> > > >  cmd/fdt.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 87 insertions(+)
> >
> > Please add some help at doc/usage/cmd
>
> Right, will do.
>
> > Also please add a test for this subcommand in test/cmd
>
> Yeah, I knew you would say that ;-)
> It's still the same problem as last time: sandboxfs doesn't implement
> .readdir, so this doesn't work easily there. So I started with filling this
> gap, and was just wondering if I should piggy back on the already existing
> sandbox_fs_ls abstraction, and somewhat re-translate this back into dirent
> structures, or whether I should properly wrap
> {open,read,close}dir in os_*dir() helpers, and build sandbox_fs_readdir()
> based on that?
>
> Any advice? Both seem equally doable.

I like option two as I think it will help in the future too!

Regards,
SImon

>
> Cheers,
> Andre
>
> > > > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > > > index 1972490bdc2..00f92dbbb5d 100644
> > > > --- a/cmd/fdt.c
> > > > +++ b/cmd/fdt.c
> > > > @@ -127,6 +129,81 @@ static int fdt_get_header_value(int argc, char *const argv[])
> > > >       return CMD_RET_FAILURE;
> > > >  }
> > > >
> > > > +#ifdef CONFIG_OF_LIBFDT_OVERLAY
> > > > +static int apply_all_overlays(const char *ifname, const char *dev_part_str,
> > > > +                           const char *dirname)
> > > > +{
> > > > +     unsigned long addr;
> > > > +     struct fdt_header *dtbo;
> > > > +     const char *addr_str;
> > > > +     struct fs_dir_stream *dirs;
> > > > +     struct fs_dirent *dent;
> > > > +     char fname[256], *name_beg;
> > > > +     int ret;
> > > > +
> > > > +     addr_str = env_get("fdtoverlay_addr_r");
> > > > +     if (!addr_str) {
> > > > +             printf("Invalid fdtoverlay_addr_r for loading overlays\n");
> > > > +             return CMD_RET_FAILURE;
> > > > +     }
> > > > +     addr = hextoul(addr_str, NULL);
> > > > +
> > > > +     ret = fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
> > > > +     if (ret)
> > > > +             return CMD_RET_FAILURE;
> > > > +
> > > > +     if (!dirname)
> > > > +             dirname = "/";
> > > > +     dirs = fs_opendir(dirname);
> > > > +     if (!dirs) {
> > > > +             printf("Cannot find directory \"%s\"\n", dirname);
> > > > +             return CMD_RET_FAILURE;
> > > > +     }
> > > > +
> > > > +     strcpy(fname, dirname);
> > > > +     name_beg = strchr(fname, 0);
> > > > +     if (name_beg[-1] != '/')
> > > > +             *name_beg++ = '/';
> > > > +
> > > > +     dtbo = map_sysmem(addr, 0);
> > > > +     while ((dent = fs_readdir(dirs))) {
> > > > +             loff_t size = 0;
> > > > +
> > > > +             if (dent->type == FS_DT_DIR)
> > > > +                     continue;
> > > > +
> > > > +             if (strcmp(dent->name + strlen(dent->name) - 5, ".dtbo"))
> > > > +                     continue;
> > > > +
> > > > +             printf("%s: ", dent->name);
> > > > +             strcpy(name_beg, dent->name);
> > > > +             fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
> > > > +             if (dent->size > SZ_2M)
> > > > +                     size = SZ_2M;
> > > > +             else
> > > > +                     size = dent->size;
> > > > +             ret = fs_read(fname, addr, 0, size, &size);
> > > > +             if (ret) {
> > > > +                     printf("  errno: %d\n", ret);
> > > > +                     continue;
> > > > +             }
> > > > +             if (!fdt_valid(&dtbo)) {
> > > > +                     /* fdt_valid() clears the pointer upon failure */
> > > > +                     dtbo = map_sysmem(addr, 0);
> > > > +                     continue;
> > > > +             }
> > > > +
> > > > +             if (fdt_overlay_apply_verbose(working_fdt, dtbo) == 0)
> > > > +                     printf("applied\n");
> > > > +     }
> > > > +     unmap_sysmem(dtbo);
> > > > +
> > > > +     fs_closedir(dirs);
> > > > +
> > > > +     return 0;
> > > return CMD_RET_SUCCESS;
> >
> > There is no need for that...0 means success in U-Boot. It is shorter
> > and clearer IMO.
> >
> > Regards,
> > SImon
>