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

[Quentin Schulz <quentin.schulz@cherry.de>]

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?

[...]

> 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?

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?

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.

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`.

> +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?

Cheers,
Quentin