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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3419 bytes --]

On Mon, May 05, 2025 at 05:37:33PM +0200, Simon Glass wrote:
> Hi Quentin,
> 
> On Mon, 5 May 2025 at 10:34, Quentin Schulz <quentin.schulz@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@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.

Repeating myself from some other discussion before, it's not just "distro
scripts", managing things with the environment is an intentional
feature, not a backwards compatible legacy thing. Solutions to "boot
this device" that do not have some support for using the environment to
modify things as needed are unlikely to be flexible enough for all use
cases.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]