Re: [PATCH 1/2] drivers: misc: external_state: add a barebox external state.

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

Hi,

On 03.11.25 05:38, chalianis1@gmail.com wrote:
> From: Chali Anis <chalianis1@gmail.com>
> 
> Add a driver to use an external state dtb, gives the ability to define
> an external state at compile time. useful for yocto or buildroot defining
> a state.dtb that will be passed to barebox at compile time vi a defconfig
> fragment.

We already had code that did this unconditionally for EFI.
Can you explain why this needs to be customizable?

> +config EXTERNAL_STATE
> +	tristate "Use external barebox state dtb"
> +	depends on OFDEVICE
> +	depends on STATE
> +	help
> +	  This permits the use of an extranl dtb state blob

external

> +	  which permits to dynamicly at compile time specify

dynamically

> +	  an external blob vi EXTERNAL_STATE_DTB_PATH
> +
> +config EXTERNAL_STATE_DTB_PATH
> +	string "the external barebox state dtb path"
> +	depends on EXTERNAL_STATE

Really needs help text as EXTERNAL makes me think of a file on the
build host.

> +static int state_external_init(void)
> +{
> +	const char *dt_path = CONFIG_EXTERNAL_STATE_DTB_PATH;
> +	struct device_node *state_root = NULL;
> +	size_t size;
> +	void *fdt;
> +	int ret;
> +
> +	if (strlen(dt_path) <= 0)
> +		return -EINVAL;

strlen can't return negative numbers.

The original function had a number of safety checks that are missing here:

- It only ran the code when in EFI payload mode, as the same barebox
  binary could be chainloaded without EFI

- It checked that /boot is mounted and gave a useful error message to hint
  at e.g., EFIFS missing, which we are missing out on here.


I am not yet convinced this is strictly an improvement.

If you have the need to customize where your state comes from, how about adding
a state -f /path/to/state.dtb command option and calling it from your init script?

Cheers,
Ahmad

> +
> +	fdt = read_file(dt_path, &size);
> +	if (!fdt) {
> +		pr_info("unable to read %s: %m\n", dt_path);
> +		return 0;
> +	}
> +
> +	state_root = of_unflatten_dtb(fdt, size);
> +	if (!IS_ERR(state_root)) {
> +		struct device_node *np = NULL;
> +		struct state *state;
> +
> +		ret = barebox_register_of(state_root);
> +		if (ret)
> +			pr_warn("Failed to register device-tree: %pe\n", ERR_PTR(ret));
> +
> +		np = of_find_node_by_alias(state_root, "state");
> +
> +		state = state_new_from_node(np, false);
> +		if (IS_ERR(state))
> +			return PTR_ERR(state);
> +
> +		ret = state_load(state);
> +		if (ret != -ENOMEDIUM)
> +			pr_warn("Failed to load persistent state, continuing with defaults, %d\n",
> +				ret);
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +late_initcall(state_external_init);


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