[PATCH v2 15/18] bootstd: Provide a command to select the bootdev order

[Simon Glass]

Hi Quentin,

On Mon, 5 May 2025 at 10:34, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 5/1/25 3:37 PM, Simon Glass wrote:
> [...]
> > diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> > index 9bee73ead58..294865feb64 100644
> > --- a/boot/bootstd-uclass.c
> > +++ b/boot/bootstd-uclass.c
> > @@ -6,6 +6,8 @@
> >    * Written by Simon Glass <sjg at chromium.org>
> >    */
> >
> > +#define LOG_CATEGORY UCLASS_BOOTSTD
> > +
> >   #include <alist.h>
> >   #include <blk.h>
> >   #include <bootdev.h>
> > @@ -132,6 +134,22 @@ const char *const *const bootstd_get_bootdev_order(struct udevice *dev,
> >       return std->bootdev_order;
> >   }
> >
> > +void bootstd_set_bootdev_order(struct udevice *dev, const char **order_str)
> > +{
> > +     struct bootstd_priv *std = dev_get_priv(dev);
> > +     const char **name;
> > +
> > +     free(std->bootdev_order);  /* leak; convert to use alist */
> > +
>
> leak? and aren't you using alist already?

Not for this, yet.

>
> [...]
>
> > diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
> > index 98a0f43c580..abede194cba 100644
> > --- a/doc/usage/cmd/bootdev.rst
> > +++ b/doc/usage/cmd/bootdev.rst
> > @@ -13,6 +13,7 @@ Synopsis
> >
> >       bootdev list [-p]        - list all available bootdevs (-p to probe)
> >       bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
> > +    bootdev order [clear] | [<spec> ...]  - view or update bootdev order
> >       bootdev select <bm>      - select a bootdev by name
> >       bootdev info [-p]        - show information about a bootdev
> >
> > @@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to run, e.g. "mmc". If no
> >   name is provided, all hunters are run.
> >
> >
> > +bootdev order
> > +~~~~~~~~~~~~~
> > +
> > +This allows the bootdev order to be examined or set. With no argument the
> > +current ordering is shown, one item per line.
> > +
> > +The argument can either be 'clear' or a space-separated list of labels. Each
> > +label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev sequence
> > +number ("3") or a media uclass ("mmc") with an optional sequence number (mmc2).
> > +
> > +Use `bootdev order clear` to clear any ordering and use the default.
> > +
> > +By default, the ordering is defined by the `boot_targets` environment variable
> > +or, failing that, the bootstd node in the devicetree ("bootdev-order" property).
> > +If no ordering is provided, then a default one is used.
> > +
>
> Not sure what's the benefit if we can simply set the environment variable?

The environment variable is there to maintain backwards compatibility
with the distro scripts. It now seems that we are unlikely to ever
drop those, but you never know.

>
> FWIW, it's what we do in
> board/theobroma-systems/common/common.c:setup_boottargets()
>
> One of the benefits I see is dumping the order if that variable isn't
> set (and also not in the DT), but shouldn't that be information we
> should be getting from `bootdev list` instead?

In general, the ordering is not known until a scan is completed, since
scanning can invoke hunters to find other bootdevs not visible prior.

>
> The point I'm trying to make is do we need yet another way to set the
> bootdev order? Printing the order would be welcomed I guess but that
> could be as simple as printing boot_targets environment variable, the DT
> property or whatever the default is if both are missing.

As above, the boot_targets var is for backwards compatibility. The DT
property is fixed and cannot be changed at runtime. So I believe we do
need this command.

>
> If we are to keep the clear command, I would use a flag instead of an
> argument to be consistent with the other bootdev commands, e.g.
> something like `bootdev order -c`.

Yes that sounds better, thanks.

>
> > +Note that this command does not check that the ordering is valid. In fact the
> > +meaning of the ordering depends on what the bootflow iterator discovers when it
> > +is used. Invalid entries will result in no bootdevs being found for that entry,
> > +so they are effectively skipped.
> > +
> >   bootdev select
> >   ~~~~~~~~~~~~~~
> >
> > @@ -171,6 +193,20 @@ This shows using one of the available hunters, then listing them::
> >                   Capacity: 0.0 MB = 0.0 GB (1 x 512)
> >       =>
> >
> > +This shows viewing and changing the ordering::
> > +
> > +    => bootdev order
> > +    mmc2
> > +    mmc1
> > +    => bootdev order 'mmc usb'
> > +    => bootdev order
> > +    mmc
> > +    usb
> > +    => bootdev order clear
> > +    => bootdev order
> > +    No ordering
>
> Shouldn't we be provided with a default one?

See above. I suppose we could implement scanning for bootdevs, but
that would need to be a separate change.

Regards,
Simon