[OSS-Tools] [PATCH 0/3] state: void partition table reread when possible

[OSS-Tools] [PATCH 0/3] state: void partition table reread when possible

[Ahmad Fatoum]

So far, barebox-state with a MMC backend always opened the whole disk
read/write, which triggers a partition table reread when closing the
device. This can be avoided in some cases:

  - If we are only interested in reading, we can open it O_RDONLY

  - If there happens to be an on-disk-described partition at exactly
    the region we are interested, we can open that instead.

So let's do that. The result can be easily verified with strace.
Without this patch:

  openat(AT_FDCWD, "/dev/mmcblk0", O_RDWR|O_LARGEFILE) = 4

With this patch:

  openat(AT_FDCWD, "/dev/mmcblk0p6", O_RDONLY|O_LARGEFILE) = 4

Ahmad Fatoum (3):
  state: backend: direct: open block device in read-only mode if
    possible
  libdt: factor out u64 sysattr parsing into helper
  libdt: use block device partition instead of parent if found

 src/barebox-state/backend_bucket_direct.c |   5 +-
 src/barebox-state/backend_storage.c       |   2 +-
 src/barebox-state/state.c                 |   6 +-
 src/barebox-state/state.h                 |   3 +-
 src/libdt.c                               | 104 +++++++++++++++++-----
 5 files changed, 92 insertions(+), 28 deletions(-)

-- 
2.39.2

[OSS-Tools] [PATCH 1/3] state: backend: direct: open block device in read-only mode if possible

[Ahmad Fatoum]

We unconditionally open the device backing a direct bucket in read-write
mode. The barebox_state utility already populates struct
state_backend_storage::readonly though, which we could consult at device
open time. Do so. This could possibly be done for MTD as well, but until
that's tested, for now, we do it only for the direct backend meant for
use with block devices.

Tested-by: Ulrich Ölmann <u.oelmann@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 src/barebox-state/backend_bucket_direct.c | 5 +++--
 src/barebox-state/backend_storage.c       | 2 +-
 src/barebox-state/state.c                 | 6 +++---
 src/barebox-state/state.h                 | 3 ++-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/barebox-state/backend_bucket_direct.c b/src/barebox-state/backend_bucket_direct.c
index 4522f0170f3d..48c1596c9add 100644
--- a/src/barebox-state/backend_bucket_direct.c
+++ b/src/barebox-state/backend_bucket_direct.c
@@ -164,12 +164,13 @@ static void state_backend_bucket_direct_free(struct
 
 int state_backend_bucket_direct_create(struct device_d *dev, const char *path,
 				       struct state_backend_storage_bucket **bucket,
-				       off_t offset, ssize_t max_size)
+				       off_t offset, ssize_t max_size,
+				       bool readonly)
 {
 	int fd;
 	struct state_backend_storage_bucket_direct *direct;
 
-	fd = open(path, O_RDWR);
+	fd = open(path, readonly ? O_RDONLY : O_RDWR);
 	if (fd < 0) {
 		dev_err(dev, "Failed to open file '%s', %d\n", path, -errno);
 		return -errno;
diff --git a/src/barebox-state/backend_storage.c b/src/barebox-state/backend_storage.c
index 509427f16f1d..39c2b9803f2d 100644
--- a/src/barebox-state/backend_storage.c
+++ b/src/barebox-state/backend_storage.c
@@ -324,7 +324,7 @@ static int state_storage_file_buckets_init(struct state_backend_storage *storage
 		offset = storage->offset + n * stridesize;
 		ret = state_backend_bucket_direct_create(storage->dev, storage->path,
 							 &bucket, offset,
-							 stridesize);
+							 stridesize, storage->readonly);
 		if (ret) {
 			dev_warn(storage->dev, "Failed to create direct bucket at '%s' offset %lld\n",
 				 storage->path, (long long) offset);
diff --git a/src/barebox-state/state.c b/src/barebox-state/state.c
index f528b3e19f21..7fa10f2eeb6b 100644
--- a/src/barebox-state/state.c
+++ b/src/barebox-state/state.c
@@ -650,14 +650,14 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	if (ret)
 		goto out_release_state;
 
+	if (readonly)
+		state_backend_set_readonly(state);
+
 	ret = state_storage_init(state, state->backend_path, offset,
 				 size, stridesize, storage_type);
 	if (ret)
 		goto out_release_state;
 
-	if (readonly)
-		state_backend_set_readonly(state);
-
 	ret = state_from_node(state, node, 1);
 	if (ret) {
 		goto out_release_state;
diff --git a/src/barebox-state/state.h b/src/barebox-state/state.h
index 912d6d484823..2e9b4b83f093 100644
--- a/src/barebox-state/state.h
+++ b/src/barebox-state/state.h
@@ -227,7 +227,8 @@ void state_backend_set_readonly(struct state *state);
 void state_storage_free(struct state_backend_storage *storage);
 int state_backend_bucket_direct_create(struct device_d *dev, const char *path,
 				       struct state_backend_storage_bucket **bucket,
-				       off_t offset, ssize_t max_size);
+				       off_t offset, ssize_t max_size,
+				       bool readonly);
 int state_storage_write(struct state_backend_storage *storage,
 			const void * buf, ssize_t len);
 int state_storage_read(struct state_backend_storage *storage,
-- 
2.39.2

[OSS-Tools] [PATCH 2/3] libdt: factor out u64 sysattr parsing into helper

[Ahmad Fatoum]

We will need to parse two more sysattrs in a follow-up patch, so factor
this out for reuse. While at it switch to 64-bit strtoull: This may
be more useful for future users and unlike atol doesn't invoke undefined
behavior when parsing fails.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 src/libdt.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/libdt.c b/src/libdt.c
index 59e76d336d8d..6f1a9e309d53 100644
--- a/src/libdt.c
+++ b/src/libdt.c
@@ -2132,6 +2132,33 @@ static struct udev_device *device_find_mtd_partition(struct udev_device *dev,
 	return NULL;
 }
 
+/*
+ * udev_device_parse_sysattr_u64 - parse sysattr value as unsigned 64-bit integer
+ * @dev:	the udev_device to extract the attribute from
+ * @attr:	name of the attribute to lookup
+ * @outvalue:	returns the value parsed out of the attribute
+ *
+ * returns 0 for success or negative error value on failure.
+ */
+static int udev_device_parse_sysattr_u64(struct udev_device *dev, const char *attr,
+		u64 *outvalue)
+{
+	char *endptr;
+	u64 value;
+	const char *str;
+
+	str = udev_device_get_sysattr_value(dev, attr);
+	if (!str)
+		return -EINVAL;
+
+	value = strtoull(str, &endptr, 0);
+	if (!*str || *endptr)
+		return -EINVAL;
+
+	*outvalue = value;
+	return 0;
+}
+
 /*
  * device_find_block_device - extract device path from udev block device
  *
@@ -2258,24 +2285,25 @@ static int udev_device_is_eeprom(struct udev_device *dev)
  * udev_parse_mtd - get information from a mtd udev_device
  * @dev:	the udev_device to extract information from
  * @devpath:	returns the devicepath under which the mtd device is accessible
- * @size:	returns the size of the mtd device
+ * @outsize:	returns the size of the mtd device
  *
  * returns 0 for success or negative error value on failure. *devpath
  * will be valid on success and must be freed after usage.
  */
-static int udev_parse_mtd(struct udev_device *dev, char **devpath, size_t *size)
+static int udev_parse_mtd(struct udev_device *dev, char **devpath, size_t *outsize)
 {
-	const char *sizestr;
 	const char *outpath;
+	u64 size;
+	int ret;
 
 	if (!udev_device_is_mtd(dev))
 		return -EINVAL;
 
-	sizestr = udev_device_get_sysattr_value(dev, "size");
-	if (!sizestr)
-		return -EINVAL;
+	ret = udev_device_parse_sysattr_u64(dev, "size", &size);
+	if (ret)
+		return ret;
 
-	*size = atol(sizestr);
+	*outsize = size;
 
 	outpath = udev_device_get_devnode(dev);
 	if (!outpath)
-- 
2.39.2

[OSS-Tools] [PATCH 3/3] libdt: use block device partition instead of parent if found

[Ahmad Fatoum]

Linux doesn't parse MTD fixed-partitions described in the device tree
for block devices and EEPROMs. For block devices, The dt-utils work around
this by doing the lookup in userspace and then arrive at the parent block
device along with an offset that is passed along. We can do better
though: If there's a partition block device that contains the region
we are interested in, we should just be using that.

This avoids the issue of triggering a reread of the partition table
after writing barebox state because udev is notified that we had
been opening the whole block device writable.

Tested-by: Ulrich Ölmann <u.oelmann@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 src/libdt.c | 62 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/src/libdt.c b/src/libdt.c
index 6f1a9e309d53..7d03ac1d4394 100644
--- a/src/libdt.c
+++ b/src/libdt.c
@@ -2159,23 +2159,32 @@ static int udev_device_parse_sysattr_u64(struct udev_device *dev, const char *at
 	return 0;
 }
 
+/* True if A completely contains B */
+static bool region_contains(loff_t starta, loff_t enda,
+			    loff_t startb, loff_t endb)
+{
+        return starta <= startb && enda >= endb;
+}
+
 /*
  * device_find_block_device - extract device path from udev block device
  *
  * @dev:	the udev_device to extract information from
  * @devpath:	returns the devicepath under which the block device is accessible
+ * @start:	start of partition in bytes
+ * @size:	size of partition in bytes
  *
- * returns 0 for success or negative error value on failure.
+ * returns 0 for success or negative error value on failure. Start
+ * is adjusted if a suitable partition is found within the parent block device.
  */
 int device_find_block_device(struct udev_device *dev,
-		char **devpath)
+		char **devpath, off_t *start, size_t size)
 {
 
 	struct udev *udev;
 	struct udev_enumerate *enumerate;
 	struct udev_list_entry *devices, *dev_list_entry;
-	struct udev_device *part;
-	const char *outpath;
+	struct udev_device *part, *best_match = NULL;
 	int ret;
 
 	udev = udev_new();
@@ -2199,19 +2208,43 @@ int device_find_block_device(struct udev_device *dev,
 		if (!devtype)
 			continue;
 		if (!strcmp(devtype, "disk")) {
-			outpath = udev_device_get_devnode(part);
-			*devpath = strdup(outpath);
-			ret = 0;
-			goto out;
+			best_match = part;
+
+			/* Should we try to find  a matching partition first? */
+			if (!size)
+				break;
+		} else if (start && size && !strcmp(devtype, "partition")) {
+			u64 partstart, partsize;
+
+			ret = udev_device_parse_sysattr_u64(part, "start", &partstart);
+			if (ret)
+				continue;
+
+			ret = udev_device_parse_sysattr_u64(part, "size", &partsize);
+			if (ret)
+				continue;
+
+			/* start/size sys attributes are always in 512-byte units */
+			partstart *= 512;
+			partsize *= 512;
+
+			if (!region_contains(*start, *start + size,
+					     partstart, partstart + partsize))
+				continue;
+
+			best_match = part;
+			*start -= partstart;
+			break;
 		}
 	}
-	ret = -ENODEV;
 
-out:
+	if (best_match)
+		*devpath = strdup(udev_device_get_devnode(best_match));
+
 	udev_enumerate_unref(enumerate);
 	udev_unref(udev);
 
-	return ret;
+	return best_match ? 0 : -ENODEV;
 }
 
 /*
@@ -2428,7 +2461,7 @@ int of_get_devicepath(struct device_node *partition_node, char **devpath, off_t
 			return udev_parse_eeprom(dev, devpath);
 		} else if (!udev_parse_mtd(dev, devpath, size)) {
 			return 0;
-		} else if (device_find_block_device(dev, devpath)) {
+		} else if (device_find_block_device(dev, devpath, NULL, 0)) {
 			return of_parse_partition(partition_node, offset, size);
 		}
 
@@ -2505,10 +2538,11 @@ int of_get_devicepath(struct device_node *partition_node, char **devpath, off_t
 
 		return of_parse_partition(partition_node, offset, size);
 	} else {
-		ret = device_find_block_device(dev, devpath);
+		ret = of_parse_partition(partition_node, offset, size);
 		if (ret)
 			return ret;
-		return of_parse_partition(partition_node, offset, size);
+
+		return device_find_block_device(dev, devpath, offset, *size);
 	}
 
 	return -EINVAL;
-- 
2.39.2