Re: [PATCH v2 1/2] Add support for extlinux.conf

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Alexander,

On 4/28/26 3:28 PM, Alexander Shiyan wrote:

> +static int extlinux_boot(struct bootentry *be, int verbose, int dryrun)
> +{
> +	struct extlinux_entry *e =
> +		container_of(be, struct extlinux_entry, entry);
> +	char *kernel_abs, *initrd_abs = NULL, *fdt_abs = NULL;
> +	struct bootm_data data = {};
> +	int ret;
> +
> +	bootm_data_init_defaults(&data);
> +
> +	data.dryrun = max_t(int, dryrun, data.dryrun);
> +	data.verbose = max(verbose, data.verbose);
> +
> +	kernel_abs = basprintf("%s/%s", e->rootpath, e->kernel);
> +	data.os_file = kernel_abs;
> +
> +	if (e->initrd) {
> +		initrd_abs = basprintf("%s/%s", e->rootpath, e->initrd);
> +		data.initrd_file = initrd_abs;
> +	}
> +
> +	if (e->fdt) {
> +		char *fdtdir = e->fdtdir ? : e->rootpath;
> +
> +		fdt_abs = basprintf("%s/%s", fdtdir, e->fdt);
> +		data.oftree_file = fdt_abs;
> +	}
> +
> +	if (e->append) {
> +		char *append;
> +
> +		/*
> +		 * The same rootfs image may be launched from eMMC or SD card.
> +		 * Remove any hardcoded root= parameter from "append" to avoid
> +		 * conflicts, then let barebox automatically add the correct
> +		 * root= (via global.bootm.appendroot) based on the boot device.
> +		 */
> +		if (data.appendroot)
> +			append = remove_param(e->append, "ROOT=");

Why use the upper case form here? Does the kernel even support uppercase
ROOT=? I suspect not, in which case we can replace the case-insensitive
comparison with a regular one.


I thought about this a bit and I still see multiple issues:

- Even in absence of root=, an initramfs can decide its own root anyway
- Giving this extra meaning to bootm.appendroot will make extlinux.conf
behave differently to blspec
- The extlinux.conf files are not necessarily within a rootfs to begin
with (maybe it's partition with just boot/kernel images)
- Other kernel options that are related to the rootfs aren't touched by this

I am not sure these can be resolved properly.

I thus have a question to you: What happens if you drop remove_param()
completely? Sure we will have two root=, but the later gets precedence,
so your use case would continue to work without giving users a wrong
expectation.

I just Cc'd you on [1] which ensures appendroot is later than whatever
is added by blspec or extlinux.conf.

[1]:
https://lore.barebox.org/barebox/20260505095137.1123867-2-a.fatoum@pengutronix.de/T/#u

> +		else
> +			append = xstrdup(e->append);
> +
> +		globalvar_add_simple("linux.bootargs.dyn.extlinux", append);

Please rename to linux.bootargs.dyn.bootentries like blspec.

These kernel parameters are assembled in alphabetical order and have the
same name both here and in blspec will make interaction with other
parameters less surprising.

> +	ret = bootm_boot(&data);

As mentioned for v1, this should be bootm_entry(), not bootm_boot().

> +	if (ret)
> +		pr_err("bootm failed: %s\n", strerror(-ret));

... %pe\n", ERR_PTR(ret));

Cheers,
Ahmad


-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |