[OSS-Tools] [PATCH v3 0/3] improve barebox-state support on EFI system

[OSS-Tools] [PATCH v3 0/3] improve barebox-state support on EFI system

[Michael Olbrich]

Hi,

so the discussion on the barebox ML resulted in a different binding for
this. Sascha has sent patches for that[1]. This is now mainline in Barebox.

So while this is 'v3' for this topic, all the patches except the last one
are actually different, so please drop the old series that is still present
in the next branch.

v2 had some Bugs that have been fixed in this new version.

In the device-tree it now looks like this:

----------------------------------------------------------------------
/ {
[...]
        state: state {
[...]
                backend = <&barebox_state>;
[...]
        };

        disk {
                compatible = "barebox,storage-by-uuid";
                uuid = "deadbeaf";

                partitions {
                        compatible = "fixed-partitions";
                        #address-cells = <2>;
                        #size-cells = <2>;

                        barebox_state: state@300000 {
                                label = "barebox-state";
                                reg = <0x0 0x300000 0x0 0x100000>;
                        };
                };
        };
};
----------------------------------------------------------------------

Regards,
Michael

[1] https://lore.barebox.org/barebox/20220207094953.949868-1-s.hauer@pengutronix.de/T/#t

Michael Olbrich (3):
  libdt: only requires a partname for mtd
  libdt: add support for barebox,storage-by-uuid
  state: automatically find state.dtb in the ESP

 src/barebox-state.c | 24 ++++++++++++
 src/libdt.c         | 91 ++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 102 insertions(+), 13 deletions(-)

-- 
2.30.2

[OSS-Tools] [PATCH v3 1/3] libdt: only requires a partname for mtd

[Michael Olbrich]

It's not used anywhere else.

Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
---

Unchanged in v3.

 src/libdt.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/libdt.c b/src/libdt.c
index 59e76d336d8d..48c31931e8a1 100644
--- a/src/libdt.c
+++ b/src/libdt.c
@@ -2381,7 +2381,6 @@ int of_get_devicepath(struct device_node *partition_node, char **devpath, off_t
 {
 	struct device_node *node;
 	struct udev_device *dev, *partdev, *mtd;
-	const char *partname;
 	int ret;
 
 	*offset = 0;
@@ -2451,16 +2450,18 @@ int of_get_devicepath(struct device_node *partition_node, char **devpath, off_t
 		return -ENODEV;
 	}
 
-	/* find the name of the partition... */
-	ret = of_property_read_string(partition_node, "label", &partname);
-	if (ret) {
-		fprintf(stderr, "%s: no 'label' property found in %s\n", __func__,
-			partition_node->full_name);
-		return ret;
-	}
-
 	mtd = of_find_mtd_device(dev);
 	if (mtd) {
+		const char *partname;
+
+		/* find the name of the partition... */
+		ret = of_property_read_string(partition_node, "label", &partname);
+		if (ret) {
+			fprintf(stderr, "%s: no 'label' property found in %s\n", __func__,
+				partition_node->full_name);
+			return ret;
+		}
+
 		/* ...find the udev_device by partition name... */
 		partdev = device_find_mtd_partition(dev, partname);
 		if (!partdev)
-- 
2.30.2

[OSS-Tools] [PATCH v3 2/3] libdt: add support for barebox, storage-by-uuid

[Michael Olbrich]

Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
---

uuid comparison fixed:
 - compare the correct uuids
 - don't crash when no uuid is present

 src/libdt.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 5 deletions(-)

diff --git a/src/libdt.c b/src/libdt.c
index 48c31931e8a1..f2c7c49ebcde 100644
--- a/src/libdt.c
+++ b/src/libdt.c
@@ -2358,6 +2358,52 @@ out:
 	return dev;
 }
 
+static struct udev_device *of_find_device_by_uuid(const char *uuid)
+{
+	struct udev *udev;
+	struct udev_enumerate *enumerate;
+	struct udev_list_entry *devices, *dev_list_entry;
+	int ret = 0;
+
+	udev = udev_new();
+	if (!udev) {
+		  fprintf(stderr, "Can't create udev\n");
+		  return NULL;
+	}
+
+	enumerate = udev_enumerate_new(udev);
+	udev_enumerate_add_match_subsystem(enumerate, "block");
+	udev_enumerate_scan_devices(enumerate);
+	devices = udev_enumerate_get_list_entry(enumerate);
+	udev_list_entry_foreach(dev_list_entry, devices) {
+		const char *path, *devtype, *outpath, *dev_uuid;
+		struct udev_device *device;
+
+		path = udev_list_entry_get_name(dev_list_entry);
+		device = udev_device_new_from_syspath(udev, path);
+
+		/* distinguish device (disk) from partitions */
+		devtype = udev_device_get_devtype(device);
+		if (!devtype)
+			continue;
+		if (!strcmp(devtype, "disk")) {
+			dev_uuid = udev_device_get_property_value(device, "ID_PART_TABLE_UUID");
+			if (dev_uuid && !strcmp(dev_uuid, uuid)) {
+				outpath = udev_device_get_devnode(device);
+				return device;
+			}
+		} else if (!strcmp(devtype, "partition")) {
+			dev_uuid = udev_device_get_property_value(device, "ID_PART_ENTRY_UUID");
+			if (dev_uuid && !strcmp(dev_uuid, uuid)) {
+				outpath = udev_device_get_devnode(device);
+				return device;
+			}
+		}
+
+	}
+	return NULL;
+}
+
 /*
  * of_get_devicepath - get information how to access device corresponding to a device_node
  * @partition_node:	The device_node which shall be accessed
@@ -2443,11 +2489,29 @@ int of_get_devicepath(struct device_node *partition_node, char **devpath, off_t
 	if (!strcmp(node->name, "partitions"))
 		node = node->parent;
 
-	dev = of_find_device_by_node_path(node->full_name);
-	if (!dev) {
-		fprintf(stderr, "%s: cannot find device from node %s\n", __func__,
-				node->full_name);
-		return -ENODEV;
+	if (of_device_is_compatible(node, "barebox,storage-by-uuid")) {
+		const char *uuid;
+
+		ret = of_property_read_string(node, "uuid", &uuid);
+		if (ret) {
+			fprintf(stderr, "%s: missing uuid property for %s\n", __func__,
+					node->full_name);
+			return -ENODEV;
+		}
+		dev = of_find_device_by_uuid(uuid);
+		if (!dev) {
+			fprintf(stderr, "%s: cannot find device for uuid %s\n", __func__,
+				uuid);
+			return -ENODEV;
+		}
+	}
+	else {
+		dev = of_find_device_by_node_path(node->full_name);
+		if (!dev) {
+			fprintf(stderr, "%s: cannot find device from node %s\n", __func__,
+					node->full_name);
+			return -ENODEV;
+		}
 	}
 
 	mtd = of_find_mtd_device(dev);
-- 
2.30.2

[OSS-Tools] [PATCH v3 3/3] state: automatically find state.dtb in the ESP

[Michael Olbrich]

Systemd mounts the EFI System Partition (ESP) to /boot or /efi.
So look there for the state.dtb when the devicetree in sysfs/procfs is
not available.

This way barebox-state can be used on EFI systems without manually
specifying the devicetree file.

Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
---
 src/barebox-state.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/barebox-state.c b/src/barebox-state.c
index 334aed6f3d43..bf67340d4dc6 100644
--- a/src/barebox-state.c
+++ b/src/barebox-state.c
@@ -342,6 +342,30 @@ struct state *state_get(const char *name, const char *filename, bool readonly, b
 		}
 	} else {
 		root = of_read_proc_devicetree();
+
+		/* No device-tree in procfs / sysfs, try dtb file in the ESP */
+		if (-PTR_ERR(root) == ENOENT) {
+			const char *paths[] = {
+				/* default mount paths used by systemd */
+				"/boot/EFI/BAREBOX/state.dtb",
+				"/efi/EFI/BAREBOX/state.dtb",
+				NULL
+			};
+			void *fdt;
+			int i;
+
+			for (i = 0; paths[i]; ++i) {
+				fdt = read_file(paths[i], NULL);
+				if (fdt)
+					break;
+			}
+			if (fdt) {
+				root = of_unflatten_dtb(fdt);
+				free(fdt);
+			}
+			else
+				root = ERR_PTR(-ENOENT);
+		}
 		if (IS_ERR(root)) {
 			pr_err("Unable to read devicetree. %s\n",
 			       strerror(-PTR_ERR(root)));
-- 
2.30.2

Re: [OSS-Tools] [PATCH v3 3/3] state: automatically find state.dtb in the ESP

[Ahmad Fatoum]

Hello Michael,

On 10.05.22 08:57, Michael Olbrich wrote:
> Systemd mounts the EFI System Partition (ESP) to /boot or /efi.
> So look there for the state.dtb when the devicetree in sysfs/procfs is
> not available.
> 
> This way barebox-state can be used on EFI systems without manually
> specifying the devicetree file.
> 
> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> ---
>  src/barebox-state.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/src/barebox-state.c b/src/barebox-state.c
> index 334aed6f3d43..bf67340d4dc6 100644
> --- a/src/barebox-state.c
> +++ b/src/barebox-state.c
> @@ -342,6 +342,30 @@ struct state *state_get(const char *name, const char *filename, bool readonly, b
>  		}
>  	} else {
>  		root = of_read_proc_devicetree();
> +
> +		/* No device-tree in procfs / sysfs, try dtb file in the ESP */
> +		if (-PTR_ERR(root) == ENOENT) {
> +			const char *paths[] = {
> +				/* default mount paths used by systemd */
> +				"/boot/EFI/BAREBOX/state.dtb",
> +				"/efi/EFI/BAREBOX/state.dtb",

On my Debian, it is /boot/efi/EFI. Do you mind appending that one to the list? :-)

> +				NULL
> +			};
> +			void *fdt;
> +			int i;
> +
> +			for (i = 0; paths[i]; ++i) {
> +				fdt = read_file(paths[i], NULL);
> +				if (fdt)
> +					break;
> +			}
> +			if (fdt) {
> +				root = of_unflatten_dtb(fdt);
> +				free(fdt);
> +			}
> +			else
> +				root = ERR_PTR(-ENOENT);

This duplicates code in the first if clause. Can you move this into
a common helper (just a static function above state_get suffices)
and use it?

Thanks,
Ahmad

> +		}
>  		if (IS_ERR(root)) {
>  			pr_err("Unable to read devicetree. %s\n",
>  			       strerror(-PTR_ERR(root)));


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

Re: [OSS-Tools] [PATCH v3 2/3] libdt: add support for barebox, storage-by-uuid

[Ahmad Fatoum]

On 10.05.22 08:57, Michael Olbrich wrote:
> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> ---
> 
> uuid comparison fixed:
>  - compare the correct uuids
>  - don't crash when no uuid is present
> 
>  src/libdt.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libdt.c b/src/libdt.c
> index 48c31931e8a1..f2c7c49ebcde 100644
> --- a/src/libdt.c
> +++ b/src/libdt.c
> @@ -2358,6 +2358,52 @@ out:
>  	return dev;
>  }
>  
> +static struct udev_device *of_find_device_by_uuid(const char *uuid)
> +{
> +	struct udev *udev;
> +	struct udev_enumerate *enumerate;
> +	struct udev_list_entry *devices, *dev_list_entry;
> +	int ret = 0;
> +
> +	udev = udev_new();
> +	if (!udev) {
> +		  fprintf(stderr, "Can't create udev\n");
> +		  return NULL;
> +	}
> +
> +	enumerate = udev_enumerate_new(udev);
> +	udev_enumerate_add_match_subsystem(enumerate, "block");
> +	udev_enumerate_scan_devices(enumerate);
> +	devices = udev_enumerate_get_list_entry(enumerate);
> +	udev_list_entry_foreach(dev_list_entry, devices) {
> +		const char *path, *devtype, *outpath, *dev_uuid;
> +		struct udev_device *device;
> +
> +		path = udev_list_entry_get_name(dev_list_entry);
> +		device = udev_device_new_from_syspath(udev, path);
> +
> +		/* distinguish device (disk) from partitions */
> +		devtype = udev_device_get_devtype(device);
> +		if (!devtype)
> +			continue;
> +		if (!strcmp(devtype, "disk")) {
> +			dev_uuid = udev_device_get_property_value(device, "ID_PART_TABLE_UUID");
> +			if (dev_uuid && !strcmp(dev_uuid, uuid)) {

The partuuid comparison is case insensitive. Making UUID case-sensitive
might be a bit surprising for users.

> +				outpath = udev_device_get_devnode(device);
> +				return device;
> +			}
> +		} else if (!strcmp(devtype, "partition")) {
> +			dev_uuid = udev_device_get_property_value(device, "ID_PART_ENTRY_UUID");
> +			if (dev_uuid && !strcmp(dev_uuid, uuid)) {
> +				outpath = udev_device_get_devnode(device);
> +				return device;
> +			}
> +		}
> +
> +	}
> +	return NULL;
> +}
> +
>  /*
>   * of_get_devicepath - get information how to access device corresponding to a device_node
>   * @partition_node:	The device_node which shall be accessed
> @@ -2443,11 +2489,29 @@ int of_get_devicepath(struct device_node *partition_node, char **devpath, off_t
>  	if (!strcmp(node->name, "partitions"))
>  		node = node->parent;
>  
> -	dev = of_find_device_by_node_path(node->full_name);
> -	if (!dev) {
> -		fprintf(stderr, "%s: cannot find device from node %s\n", __func__,
> -				node->full_name);
> -		return -ENODEV;
> +	if (of_device_is_compatible(node, "barebox,storage-by-uuid")) {
> +		const char *uuid;
> +
> +		ret = of_property_read_string(node, "uuid", &uuid);
> +		if (ret) {
> +			fprintf(stderr, "%s: missing uuid property for %s\n", __func__,
> +					node->full_name);
> +			return -ENODEV;
> +		}
> +		dev = of_find_device_by_uuid(uuid);
> +		if (!dev) {
> +			fprintf(stderr, "%s: cannot find device for uuid %s\n", __func__,
> +				uuid);
> +			return -ENODEV;
> +		}
> +	}
> +	else {
> +		dev = of_find_device_by_node_path(node->full_name);
> +		if (!dev) {
> +			fprintf(stderr, "%s: cannot find device from node %s\n", __func__,
> +					node->full_name);
> +			return -ENODEV;
> +		}
>  	}
>  
>  	mtd = of_find_mtd_device(dev);


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

Re: [OSS-Tools] [PATCH v3 3/3] state: automatically find state.dtb in the ESP

[Michael Olbrich]

On Tue, May 10, 2022 at 03:59:24PM +0200, Ahmad Fatoum wrote:
> Hello Michael,
> 
> On 10.05.22 08:57, Michael Olbrich wrote:
> > Systemd mounts the EFI System Partition (ESP) to /boot or /efi.
> > So look there for the state.dtb when the devicetree in sysfs/procfs is
> > not available.
> > 
> > This way barebox-state can be used on EFI systems without manually
> > specifying the devicetree file.
> > 
> > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> > ---
> >  src/barebox-state.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/src/barebox-state.c b/src/barebox-state.c
> > index 334aed6f3d43..bf67340d4dc6 100644
> > --- a/src/barebox-state.c
> > +++ b/src/barebox-state.c
> > @@ -342,6 +342,30 @@ struct state *state_get(const char *name, const char *filename, bool readonly, b
> >  		}
> >  	} else {
> >  		root = of_read_proc_devicetree();
> > +
> > +		/* No device-tree in procfs / sysfs, try dtb file in the ESP */
> > +		if (-PTR_ERR(root) == ENOENT) {
> > +			const char *paths[] = {
> > +				/* default mount paths used by systemd */
> > +				"/boot/EFI/BAREBOX/state.dtb",
> > +				"/efi/EFI/BAREBOX/state.dtb",
> 
> On my Debian, it is /boot/efi/EFI. Do you mind appending that one to the list? :-)

ok.

> > +				NULL
> > +			};
> > +			void *fdt;
> > +			int i;
> > +
> > +			for (i = 0; paths[i]; ++i) {
> > +				fdt = read_file(paths[i], NULL);
> > +				if (fdt)
> > +					break;
> > +			}
> > +			if (fdt) {
> > +				root = of_unflatten_dtb(fdt);
> > +				free(fdt);
> > +			}
> > +			else
> > +				root = ERR_PTR(-ENOENT);
> 
> This duplicates code in the first if clause. Can you move this into
> a common helper (just a static function above state_get suffices)
> and use it?

It's similar but not the same. Here we loop over multiple files and take
the first that works, so there should be no error messages when some of
them fail.
A share function would require conditional error messages and at that
point, I don't think it actually simplifies the code any more.

Michael

> > +		}
> >  		if (IS_ERR(root)) {
> >  			pr_err("Unable to read devicetree. %s\n",
> >  			       strerror(-PTR_ERR(root)));
> 
> 
> -- 
> 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 |
> 

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