* [PATCH 0/3] state: find backend with UUID but without a partition @ 2022-01-24 10:04 Michael Olbrich 2022-01-24 10:04 ` [PATCH 1/3] cdev: rename partuuid to uuid Michael Olbrich ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Michael Olbrich @ 2022-01-24 10:04 UTC (permalink / raw) To: barebox; +Cc: Michael Olbrich Hi, When adding state with a barebox update on existing devices, then it's not always possible to add a partition for it. But currently that's the only way to specify the state backend on x86_64/EFI devices. This adds support backend-diskuuid / backend-offset. This way the backend is defined by a UUID that selects a disk and an offset within that disk. Michael Michael Olbrich (3): cdev: rename partuuid to uuid cdev: add diskuuid support state: support backend-diskuuid / backend-offset common/bootm.c | 6 ++--- common/partitions.c | 2 +- common/partitions/dos.c | 3 +++ common/partitions/efi.c | 2 ++ common/partitions/parser.h | 2 +- common/state/state.c | 55 +++++++++++++++++++++++++------------- fs/devfs-core.c | 16 ++++++++++- fs/fs.c | 4 +-- include/driver.h | 5 ++-- include/fs.h | 12 +++++++++ 10 files changed, 79 insertions(+), 28 deletions(-) -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] cdev: rename partuuid to uuid 2022-01-24 10:04 [PATCH 0/3] state: find backend with UUID but without a partition Michael Olbrich @ 2022-01-24 10:04 ` Michael Olbrich 2022-01-24 10:04 ` [PATCH 2/3] cdev: add diskuuid support Michael Olbrich 2022-01-24 10:04 ` [PATCH 3/3] state: support backend-diskuuid / backend-offset Michael Olbrich 2 siblings, 0 replies; 12+ messages in thread From: Michael Olbrich @ 2022-01-24 10:04 UTC (permalink / raw) To: barebox; +Cc: Michael Olbrich Partitions are not the only devices that can have UUIDs. Change the name to something more generic to prepare for other users. Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> --- common/bootm.c | 6 +++--- common/partitions.c | 2 +- common/partitions/parser.h | 2 +- fs/devfs-core.c | 2 +- fs/fs.c | 4 ++-- include/driver.h | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/bootm.c b/common/bootm.c index 89e3e93f2ce0..4652467448eb 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -709,15 +709,15 @@ int bootm_boot(struct bootm_data *bootm_data) const char *root_dev_name = devpath_to_name(bootm_data->root_dev); const struct cdev *root_cdev = cdev_by_name(root_dev_name); - if (root_cdev && root_cdev->partuuid[0] != 0) { - rootarg = basprintf("root=PARTUUID=%s", root_cdev->partuuid); + if (root_cdev && root_cdev->uuid[0] != 0) { + rootarg = basprintf("root=PARTUUID=%s", root_cdev->uuid); } else { rootarg = ERR_PTR(-EINVAL); if (!root_cdev) pr_err("no cdev found for %s, cannot set root= option\n", root_dev_name); - else if (!root_cdev->partuuid[0]) + else if (!root_cdev->uuid[0]) pr_err("%s doesn't have a PARTUUID, cannot set root= option\n", root_dev_name); } diff --git a/common/partitions.c b/common/partitions.c index b579559672a0..9cca5c4a1546 100644 --- a/common/partitions.c +++ b/common/partitions.c @@ -51,7 +51,7 @@ static int register_one_partition(struct block_device *blk, cdev->flags |= DEVFS_PARTITION_FROM_TABLE; cdev->dos_partition_type = part->dos_partition_type; - strcpy(cdev->partuuid, part->partuuid); + strcpy(cdev->uuid, part->partuuid); free(partition_name); diff --git a/common/partitions/parser.h b/common/partitions/parser.h index 69508932b361..d67f8e1d6a09 100644 --- a/common/partitions/parser.h +++ b/common/partitions/parser.h @@ -17,7 +17,7 @@ struct partition { char name[MAX_PARTITION_NAME]; u8 dos_partition_type; - char partuuid[MAX_PARTUUID_STR]; + char partuuid[MAX_UUID_STR]; uint64_t first_sec; uint64_t size; }; diff --git a/fs/devfs-core.c b/fs/devfs-core.c index 2d016e0e4861..82e4811b384a 100644 --- a/fs/devfs-core.c +++ b/fs/devfs-core.c @@ -101,7 +101,7 @@ struct cdev *cdev_by_partuuid(const char *partuuid) return NULL; list_for_each_entry(cdev, &cdev_list, list) { - if (!strcasecmp(cdev->partuuid, partuuid)) + if (cdev->master && !strcasecmp(cdev->uuid, partuuid)) return cdev; } return NULL; diff --git a/fs/fs.c b/fs/fs.c index 7da3a050c1cb..a92b4e9dfca1 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -2969,8 +2969,8 @@ int mount(const char *device, const char *fsname, const char *pathname, cdev_is_mci_main_part_dev(fsdev->cdev->master)) str = get_linux_mmcblkdev(fsdev); - if (!str && fsdev->cdev->partuuid[0] != 0) - str = basprintf("root=PARTUUID=%s", fsdev->cdev->partuuid); + if (!str && fsdev->cdev->uuid[0] != 0) + str = basprintf("root=PARTUUID=%s", fsdev->cdev->uuid); if (str) fsdev_set_linux_rootarg(fsdev, str); diff --git a/include/driver.h b/include/driver.h index 4f6d40e17c14..409ed7e02afd 100644 --- a/include/driver.h +++ b/include/driver.h @@ -447,7 +447,7 @@ struct cdev_operations { int (*truncate)(struct cdev*, size_t size); }; -#define MAX_PARTUUID_STR sizeof("00112233-4455-6677-8899-AABBCCDDEEFF") +#define MAX_UUID_STR sizeof("00112233-4455-6677-8899-AABBCCDDEEFF") struct cdev { const struct cdev_operations *ops; @@ -460,7 +460,7 @@ struct cdev { char *partname; /* the partition name, usually the above without the * device part, i.e. name = "nand0.barebox" -> partname = "barebox" */ - char partuuid[MAX_PARTUUID_STR]; + char uuid[MAX_UUID_STR]; loff_t offset; loff_t size; unsigned int flags; -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] cdev: add diskuuid support 2022-01-24 10:04 [PATCH 0/3] state: find backend with UUID but without a partition Michael Olbrich 2022-01-24 10:04 ` [PATCH 1/3] cdev: rename partuuid to uuid Michael Olbrich @ 2022-01-24 10:04 ` Michael Olbrich 2022-01-24 10:04 ` [PATCH 3/3] state: support backend-diskuuid / backend-offset Michael Olbrich 2 siblings, 0 replies; 12+ messages in thread From: Michael Olbrich @ 2022-01-24 10:04 UTC (permalink / raw) To: barebox; +Cc: Michael Olbrich This allows identifying disks by UUID. For disks with GPT the disk GUID is used. For DOS partition tables the NT signature ist used, similar to how the partuuid is generated. Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> --- common/partitions/dos.c | 3 +++ common/partitions/efi.c | 2 ++ fs/devfs-core.c | 14 ++++++++++++++ include/driver.h | 1 + 4 files changed, 20 insertions(+) diff --git a/common/partitions/dos.c b/common/partitions/dos.c index 6c76aac37172..258b10a9ed3c 100644 --- a/common/partitions/dos.c +++ b/common/partitions/dos.c @@ -182,6 +182,9 @@ static void dos_partition(void *buf, struct block_device *blk, struct disk_signature_priv *dsp; uint32_t signature = get_unaligned_le32(buf + 0x1b8); + if (signature) + sprintf(blk->cdev.uuid, "%08x", signature); + table = (struct partition_entry *)&buffer[446]; for (i = 0; i < 4; i++) { diff --git a/common/partitions/efi.c b/common/partitions/efi.c index 6d811bfb3b01..0787b93f120b 100644 --- a/common/partitions/efi.c +++ b/common/partitions/efi.c @@ -445,6 +445,8 @@ static void efi_partition(void *buf, struct block_device *blk, return; } + snprintf(blk->cdev.uuid, sizeof(blk->cdev.uuid), "%pUl", &gpt->disk_guid); + nb_part = le32_to_cpu(gpt->num_partition_entries); if (nb_part > MAX_PARTITION) { diff --git a/fs/devfs-core.c b/fs/devfs-core.c index 82e4811b384a..2475ab959a10 100644 --- a/fs/devfs-core.c +++ b/fs/devfs-core.c @@ -107,6 +107,20 @@ struct cdev *cdev_by_partuuid(const char *partuuid) return NULL; } +struct cdev *cdev_by_diskuuid(const char *diskuuid) +{ + struct cdev *cdev; + + if (!diskuuid) + return NULL; + + list_for_each_entry(cdev, &cdev_list, list) { + if (!cdev->master && !strcasecmp(cdev->uuid, diskuuid)) + return cdev; + } + return NULL; +} + /** * device_find_partition - find a partition belonging to a physical device * diff --git a/include/driver.h b/include/driver.h index 409ed7e02afd..e38a15f51f00 100644 --- a/include/driver.h +++ b/include/driver.h @@ -484,6 +484,7 @@ struct cdev *lcdev_by_name(const char *filename); struct cdev *cdev_readlink(struct cdev *cdev); struct cdev *cdev_by_device_node(struct device_node *node); struct cdev *cdev_by_partuuid(const char *partuuid); +struct cdev *cdev_by_diskuuid(const char *partuuid); struct cdev *cdev_open(const char *name, unsigned long flags); struct cdev *cdev_create_loop(const char *path, ulong flags, loff_t offset); void cdev_remove_loop(struct cdev *cdev); -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-24 10:04 [PATCH 0/3] state: find backend with UUID but without a partition Michael Olbrich 2022-01-24 10:04 ` [PATCH 1/3] cdev: rename partuuid to uuid Michael Olbrich 2022-01-24 10:04 ` [PATCH 2/3] cdev: add diskuuid support Michael Olbrich @ 2022-01-24 10:04 ` Michael Olbrich 2022-01-26 7:57 ` Sascha Hauer 2 siblings, 1 reply; 12+ messages in thread From: Michael Olbrich @ 2022-01-24 10:04 UTC (permalink / raw) To: barebox; +Cc: Michael Olbrich On some platforms (e.g. EFI on x86_64) the state backend can only be selected by a partiton UUID. On existing devices with a DOS partition table, there may be no spare partition available for state. This makes it possible to select the disk via UUID. The exact position is defined by an explicitly specified offset. Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> --- I wasn't sure where to add the helper function. Is include/fs.h ok or should I put it somewhere else? I'll implement the same helper for dt-utils, so we can avoid additional #ifdef here. common/state/state.c | 55 +++++++++++++++++++++++++++++--------------- include/fs.h | 12 ++++++++++ 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/common/state/state.c b/common/state/state.c index 8c34ae83e52b..2a8b12d20c5a 100644 --- a/common/state/state.c +++ b/common/state/state.c @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) const char *backend_type; const char *storage_type = NULL; const char *alias; + const char *diskuuid; uint32_t stridesize; struct device_node *partition_node; off_t offset = 0; @@ -607,30 +608,48 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) if (IS_ERR(state)) return state; - partition_node = of_parse_phandle(node, "backend", 0); - if (!partition_node) { - dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); - ret = -EINVAL; - goto out_release_state; - } + ret = of_property_read_string(node, "backend-diskuuid", &diskuuid); + if (ret == 0) { + u64 off; + + ret = devpath_from_diskuuid(diskuuid, &state->backend_path); + if (ret) { + dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n", + diskuuid); + goto out_release_state; + } + ret = of_property_read_u64(node, "backend-offset", &off); + if (ret) { + dev_err(&state->dev, "'backend-offset' property undefined\n"); + goto out_release_state; + } + offset = off; + } else { + partition_node = of_parse_phandle(node, "backend", 0); + if (!partition_node) { + dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); + ret = -EINVAL; + goto out_release_state; + } #ifdef __BAREBOX__ - ret = of_partition_ensure_probed(partition_node); - if (ret) - goto out_release_state; + ret = of_partition_ensure_probed(partition_node); + if (ret) + goto out_release_state; - ret = of_find_path_by_node(partition_node, &state->backend_path, 0); + ret = of_find_path_by_node(partition_node, &state->backend_path, 0); #else - ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size); + ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size); #endif - if (ret) { - if (ret != -EPROBE_DEFER) - dev_err(&state->dev, "state failed to parse path to backend: %s\n", - strerror(-ret)); - goto out_release_state; - } + if (ret) { + if (ret != -EPROBE_DEFER) + dev_err(&state->dev, "state failed to parse path to backend: %s\n", + strerror(-ret)); + goto out_release_state; + } - state->backend_reproducible_name = of_get_reproducible_name(partition_node); + state->backend_reproducible_name = of_get_reproducible_name(partition_node); + } ret = of_property_read_string(node, "backend-type", &backend_type); if (ret) { diff --git a/include/fs.h b/include/fs.h index cd5eb571e08e..1a2f9c7f8e16 100644 --- a/include/fs.h +++ b/include/fs.h @@ -177,4 +177,16 @@ static inline const char *devpath_to_name(const char *devpath) return devpath; } +static inline int devpath_from_diskuuid(const char *diskuuid, char **devpath) +{ + struct cdev *cdev; + + cdev = cdev_by_diskuuid(diskuuid); + if (!cdev) + return -EINVAL; + + *devpath = xasprintf("/dev/%s", cdev->name); + return 0; +} + #endif /* __FS_H */ -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-24 10:04 ` [PATCH 3/3] state: support backend-diskuuid / backend-offset Michael Olbrich @ 2022-01-26 7:57 ` Sascha Hauer 2022-01-26 9:35 ` Michael Olbrich 0 siblings, 1 reply; 12+ messages in thread From: Sascha Hauer @ 2022-01-26 7:57 UTC (permalink / raw) To: Michael Olbrich; +Cc: barebox On Mon, Jan 24, 2022 at 11:04:58AM +0100, Michael Olbrich wrote: > On some platforms (e.g. EFI on x86_64) the state backend can only be > selected by a partiton UUID. On existing devices with a DOS partition > table, there may be no spare partition available for state. > > This makes it possible to select the disk via UUID. The exact position is > defined by an explicitly specified offset. > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > --- > > I wasn't sure where to add the helper function. Is include/fs.h ok or > should I put it somewhere else? > > I'll implement the same helper for dt-utils, so we can avoid additional > #ifdef here. > > common/state/state.c | 55 +++++++++++++++++++++++++++++--------------- > include/fs.h | 12 ++++++++++ > 2 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/common/state/state.c b/common/state/state.c > index 8c34ae83e52b..2a8b12d20c5a 100644 > --- a/common/state/state.c > +++ b/common/state/state.c > @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > const char *backend_type; > const char *storage_type = NULL; > const char *alias; > + const char *diskuuid; > uint32_t stridesize; > struct device_node *partition_node; > off_t offset = 0; > @@ -607,30 +608,48 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > if (IS_ERR(state)) > return state; > > - partition_node = of_parse_phandle(node, "backend", 0); > - if (!partition_node) { > - dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); > - ret = -EINVAL; > - goto out_release_state; > - } > + ret = of_property_read_string(node, "backend-diskuuid", &diskuuid); This needs some documentation in Documentation/devicetree/bindings/barebox/barebox,state.rst. > + if (ret == 0) { > + u64 off; > + > + ret = devpath_from_diskuuid(diskuuid, &state->backend_path); > + if (ret) { > + dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n", > + diskuuid); > + goto out_release_state; > + } > + ret = of_property_read_u64(node, "backend-offset", &off); I stumbled upon this because you have to use a 64bit type here instead of using 'offset' directly. I think 'offset' should be 64bit instead so that larger offsets can be used. > + if (ret) { > + dev_err(&state->dev, "'backend-offset' property undefined\n"); > + goto out_release_state; > + } > + offset = off; What about the size of the state partition? This is not set anywhere in this case so it's still zero. It should be specified the in device tree as well. At the same time I'm a bit nervous that it apparently still works with size zero. Sascha -- 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-26 7:57 ` Sascha Hauer @ 2022-01-26 9:35 ` Michael Olbrich 2022-01-26 10:15 ` Sascha Hauer 0 siblings, 1 reply; 12+ messages in thread From: Michael Olbrich @ 2022-01-26 9:35 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Wed, Jan 26, 2022 at 08:57:27AM +0100, Sascha Hauer wrote: > On Mon, Jan 24, 2022 at 11:04:58AM +0100, Michael Olbrich wrote: > > On some platforms (e.g. EFI on x86_64) the state backend can only be > > selected by a partiton UUID. On existing devices with a DOS partition > > table, there may be no spare partition available for state. > > > > This makes it possible to select the disk via UUID. The exact position is > > defined by an explicitly specified offset. > > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > > --- > > > > I wasn't sure where to add the helper function. Is include/fs.h ok or > > should I put it somewhere else? > > > > I'll implement the same helper for dt-utils, so we can avoid additional > > #ifdef here. > > > > common/state/state.c | 55 +++++++++++++++++++++++++++++--------------- > > include/fs.h | 12 ++++++++++ > > 2 files changed, 49 insertions(+), 18 deletions(-) > > > > diff --git a/common/state/state.c b/common/state/state.c > > index 8c34ae83e52b..2a8b12d20c5a 100644 > > --- a/common/state/state.c > > +++ b/common/state/state.c > > @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > const char *backend_type; > > const char *storage_type = NULL; > > const char *alias; > > + const char *diskuuid; > > uint32_t stridesize; > > struct device_node *partition_node; > > off_t offset = 0; > > @@ -607,30 +608,48 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > if (IS_ERR(state)) > > return state; > > > > - partition_node = of_parse_phandle(node, "backend", 0); > > - if (!partition_node) { > > - dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); > > - ret = -EINVAL; > > - goto out_release_state; > > - } > > + ret = of_property_read_string(node, "backend-diskuuid", &diskuuid); > > This needs some documentation in > Documentation/devicetree/bindings/barebox/barebox,state.rst. I can do that. > > + if (ret == 0) { > > + u64 off; > > + > > + ret = devpath_from_diskuuid(diskuuid, &state->backend_path); > > + if (ret) { > > + dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n", > > + diskuuid); > > + goto out_release_state; > > + } > > + ret = of_property_read_u64(node, "backend-offset", &off); > > I stumbled upon this because you have to use a 64bit type here instead > of using 'offset' directly. I think 'offset' should be 64bit instead so > that larger offsets can be used. It's not that simple. 'offset' used as a 'off_t' and 'ssize_t' all over the place in the state framework. On 32bit architecture both are defined as 'long' or 'int'. Both are 32 bit types so changing 'offset' to a 64bit type here doesn't really help. On 64bit architectures 'off_t' and 'ssize_t' are both 'long' which is a 64bit type. We still need to cast from unsigned to signed, but I don't think that's really a problem here. So it works for large offsets on 64bit architectures as it is, and it can be extended to work in 32bit architectures without changing the binding. But doing that requires larger code changes. And I have no way to test those. > > + if (ret) { > > + dev_err(&state->dev, "'backend-offset' property undefined\n"); > > + goto out_release_state; > > + } > > + offset = off; > > What about the size of the state partition? This is not set anywhere in > this case so it's still zero. It should be specified the in device tree > as well. At the same time I'm a bit nervous that it apparently still > works with size zero. The code explicitly checks if the size is specified and skips any range checks if it's not. From what I can tell, it has been like that from the beginning. If that's not ok, then I could add a 'backend-size' property, or do you have something else in mind? Regards, Michael -- 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-26 9:35 ` Michael Olbrich @ 2022-01-26 10:15 ` Sascha Hauer 2022-01-26 11:16 ` Michael Olbrich 0 siblings, 1 reply; 12+ messages in thread From: Sascha Hauer @ 2022-01-26 10:15 UTC (permalink / raw) To: Michael Olbrich; +Cc: barebox On Wed, Jan 26, 2022 at 10:35:13AM +0100, Michael Olbrich wrote: > On Wed, Jan 26, 2022 at 08:57:27AM +0100, Sascha Hauer wrote: > > On Mon, Jan 24, 2022 at 11:04:58AM +0100, Michael Olbrich wrote: > > > On some platforms (e.g. EFI on x86_64) the state backend can only be > > > selected by a partiton UUID. On existing devices with a DOS partition > > > table, there may be no spare partition available for state. > > > > > > This makes it possible to select the disk via UUID. The exact position is > > > defined by an explicitly specified offset. > > > > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > > > --- > > > > > > I wasn't sure where to add the helper function. Is include/fs.h ok or > > > should I put it somewhere else? > > > > > > I'll implement the same helper for dt-utils, so we can avoid additional > > > #ifdef here. > > > > > > common/state/state.c | 55 +++++++++++++++++++++++++++++--------------- > > > include/fs.h | 12 ++++++++++ > > > 2 files changed, 49 insertions(+), 18 deletions(-) > > > > > > diff --git a/common/state/state.c b/common/state/state.c > > > index 8c34ae83e52b..2a8b12d20c5a 100644 > > > --- a/common/state/state.c > > > +++ b/common/state/state.c > > > @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > > const char *backend_type; > > > const char *storage_type = NULL; > > > const char *alias; > > > + const char *diskuuid; > > > uint32_t stridesize; > > > struct device_node *partition_node; > > > off_t offset = 0; > > > @@ -607,30 +608,48 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > > if (IS_ERR(state)) > > > return state; > > > > > > - partition_node = of_parse_phandle(node, "backend", 0); > > > - if (!partition_node) { > > > - dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); > > > - ret = -EINVAL; > > > - goto out_release_state; > > > - } > > > + ret = of_property_read_string(node, "backend-diskuuid", &diskuuid); > > > > This needs some documentation in > > Documentation/devicetree/bindings/barebox/barebox,state.rst. > > I can do that. > > > > + if (ret == 0) { > > > + u64 off; > > > + > > > + ret = devpath_from_diskuuid(diskuuid, &state->backend_path); > > > + if (ret) { > > > + dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n", > > > + diskuuid); > > > + goto out_release_state; > > > + } > > > + ret = of_property_read_u64(node, "backend-offset", &off); > > > > I stumbled upon this because you have to use a 64bit type here instead > > of using 'offset' directly. I think 'offset' should be 64bit instead so > > that larger offsets can be used. > > It's not that simple. 'offset' used as a 'off_t' and 'ssize_t' all over the > place in the state framework. On 32bit architecture both are defined as > 'long' or 'int'. Both are 32 bit types so changing 'offset' to a 64bit type > here doesn't really help. Of course not, we would have to replace all variables which are used as offset into a device to 64bit types. That's a separate topic which doesn't have to be solved as part of this series. > > > + } > > > + offset = off; > > > > What about the size of the state partition? This is not set anywhere in > > this case so it's still zero. It should be specified the in device tree > > as well. At the same time I'm a bit nervous that it apparently still > > works with size zero. > > The code explicitly checks if the size is specified and skips any range > checks if it's not. From what I can tell, it has been like that from the > beginning. That's likely ok for real partitions. When reading/writing them past the end we'll get an error from the lower layers which can be handled, but not so in this case where size is potentially the rest of the whole disk. > > If that's not ok, then I could add a 'backend-size' property, or do you > have something else in mind? Yes, that's what I had in mind. And I think it should be mandatory. Sascha -- 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-26 10:15 ` Sascha Hauer @ 2022-01-26 11:16 ` Michael Olbrich 2022-01-27 0:18 ` Trent Piepho 0 siblings, 1 reply; 12+ messages in thread From: Michael Olbrich @ 2022-01-26 11:16 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Wed, Jan 26, 2022 at 11:15:24AM +0100, Sascha Hauer wrote: > On Wed, Jan 26, 2022 at 10:35:13AM +0100, Michael Olbrich wrote: > > On Wed, Jan 26, 2022 at 08:57:27AM +0100, Sascha Hauer wrote: > > > On Mon, Jan 24, 2022 at 11:04:58AM +0100, Michael Olbrich wrote: > > > > On some platforms (e.g. EFI on x86_64) the state backend can only be > > > > selected by a partiton UUID. On existing devices with a DOS partition > > > > table, there may be no spare partition available for state. > > > > > > > > This makes it possible to select the disk via UUID. The exact position is > > > > defined by an explicitly specified offset. > > > > > > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > > > > --- > > > > > > > > I wasn't sure where to add the helper function. Is include/fs.h ok or > > > > should I put it somewhere else? > > > > > > > > I'll implement the same helper for dt-utils, so we can avoid additional > > > > #ifdef here. > > > > > > > > common/state/state.c | 55 +++++++++++++++++++++++++++++--------------- > > > > include/fs.h | 12 ++++++++++ > > > > 2 files changed, 49 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/common/state/state.c b/common/state/state.c > > > > index 8c34ae83e52b..2a8b12d20c5a 100644 > > > > --- a/common/state/state.c > > > > +++ b/common/state/state.c > > > > @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > > > const char *backend_type; > > > > const char *storage_type = NULL; > > > > const char *alias; > > > > + const char *diskuuid; > > > > uint32_t stridesize; > > > > struct device_node *partition_node; > > > > off_t offset = 0; > > > > @@ -607,30 +608,48 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > > > if (IS_ERR(state)) > > > > return state; > > > > > > > > - partition_node = of_parse_phandle(node, "backend", 0); > > > > - if (!partition_node) { > > > > - dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); > > > > - ret = -EINVAL; > > > > - goto out_release_state; > > > > - } > > > > + ret = of_property_read_string(node, "backend-diskuuid", &diskuuid); > > > > > > This needs some documentation in > > > Documentation/devicetree/bindings/barebox/barebox,state.rst. > > > > I can do that. > > > > > > + if (ret == 0) { > > > > + u64 off; > > > > + > > > > + ret = devpath_from_diskuuid(diskuuid, &state->backend_path); > > > > + if (ret) { > > > > + dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n", > > > > + diskuuid); > > > > + goto out_release_state; > > > > + } > > > > + ret = of_property_read_u64(node, "backend-offset", &off); > > > > > > I stumbled upon this because you have to use a 64bit type here instead > > > of using 'offset' directly. I think 'offset' should be 64bit instead so > > > that larger offsets can be used. > > > > It's not that simple. 'offset' used as a 'off_t' and 'ssize_t' all over the > > place in the state framework. On 32bit architecture both are defined as > > 'long' or 'int'. Both are 32 bit types so changing 'offset' to a 64bit type > > here doesn't really help. > > Of course not, we would have to replace all variables which are used as > offset into a device to 64bit types. That's a separate topic which > doesn't have to be solved as part of this series. So what should I do here? - use 'u64' for 'offset' and remove the separate variable - use 'loff_t' for 'offset' - keep it as it is - something else? > > > > + } > > > > + offset = off; > > > > > > What about the size of the state partition? This is not set anywhere in > > > this case so it's still zero. It should be specified the in device tree > > > as well. At the same time I'm a bit nervous that it apparently still > > > works with size zero. > > > > The code explicitly checks if the size is specified and skips any range > > checks if it's not. From what I can tell, it has been like that from the > > beginning. > > That's likely ok for real partitions. When reading/writing them past the end > we'll get an error from the lower layers which can be handled, but not > so in this case where size is potentially the rest of the whole disk. > > > > > If that's not ok, then I could add a 'backend-size' property, or do you > > have something else in mind? > > Yes, that's what I had in mind. And I think it should be mandatory. So, if we add a size, then it should also be validated in some way, right? Maybe ensure that it does not overlap with existing partitions? Michael -- 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-26 11:16 ` Michael Olbrich @ 2022-01-27 0:18 ` Trent Piepho 2022-01-27 12:39 ` Sascha Hauer 0 siblings, 1 reply; 12+ messages in thread From: Trent Piepho @ 2022-01-27 0:18 UTC (permalink / raw) To: Sascha Hauer, barebox DTS ranges are usually specified as <offset> <length> in one property. The sizes of offset and length fields are done with the #address-cells and #size-cells of the bus the node is on. I.e., barebox state shouldn't be defining if offsets or lengths are 32 or 64 bits, it should/is done by the device the offset or length refers to. Like the normal 'reg' property in most nodes for register banks, or the various "ranges" properties map an address space in the current node to one in another node. This backend-diskuuid, backend-offset, and backend-length seems like a custom alternative version of a range that is specific to barebox state. Also, if the backend is a partition defined in the dts, then the node of the partition specifies its size. But if the partition is found by uuid, then the barebox state device specifies the size of the partition. Seems inconsistent. It seems like there should be a better and more consistent way to do this. Here's an idea. Identify the device by uuid and use existing fixed-partitions. Example: { compatible = "storage-by-uuid"; uuid = "abcd-1234"; // Everything below here already exists and is unchanged partitions { compatible = "fixed-partitions"; barebox_state: state@1000 { label = "barebox-state"; reg = <0x1000 0x200>; }; barebox_env: env@1200 { label = "barebox-env"; reg = <0x1200 0x1000>; }; }; }; When the top level node here is found, the matching device is located by uuid and contents of the node are added to that device. Adding fixed partitions is done the same way it's already done. The difference is we can specify the device by uuid instead of needing to locate the path of the exact hardware device. On Wed, Jan 26, 2022 at 3:23 AM Michael Olbrich <m.olbrich@pengutronix.de> wrote: > > On Wed, Jan 26, 2022 at 11:15:24AM +0100, Sascha Hauer wrote: > > On Wed, Jan 26, 2022 at 10:35:13AM +0100, Michael Olbrich wrote: > > > On Wed, Jan 26, 2022 at 08:57:27AM +0100, Sascha Hauer wrote: > > > > On Mon, Jan 24, 2022 at 11:04:58AM +0100, Michael Olbrich wrote: > > > > > On some platforms (e.g. EFI on x86_64) the state backend can only be > > > > > selected by a partiton UUID. On existing devices with a DOS partition > > > > > table, there may be no spare partition available for state. > > > > > > > > > > This makes it possible to select the disk via UUID. The exact position is > > > > > defined by an explicitly specified offset. > > > > > > > > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > > > > > --- > > > > > > > > > > I wasn't sure where to add the helper function. Is include/fs.h ok or > > > > > should I put it somewhere else? > > > > > > > > > > I'll implement the same helper for dt-utils, so we can avoid additional > > > > > #ifdef here. > > > > > > > > > > common/state/state.c | 55 +++++++++++++++++++++++++++++--------------- > > > > > include/fs.h | 12 ++++++++++ > > > > > 2 files changed, 49 insertions(+), 18 deletions(-) > > > > > > > > > > diff --git a/common/state/state.c b/common/state/state.c > > > > > index 8c34ae83e52b..2a8b12d20c5a 100644 > > > > > --- a/common/state/state.c > > > > > +++ b/common/state/state.c > > > > > @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > > > > const char *backend_type; > > > > > const char *storage_type = NULL; > > > > > const char *alias; > > > > > + const char *diskuuid; > > > > > uint32_t stridesize; > > > > > struct device_node *partition_node; > > > > > off_t offset = 0; > > > > > @@ -607,30 +608,48 @@ struct state *state_new_from_node(struct device_node *node, bool readonly) > > > > > if (IS_ERR(state)) > > > > > return state; > > > > > > > > > > - partition_node = of_parse_phandle(node, "backend", 0); > > > > > - if (!partition_node) { > > > > > - dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n"); > > > > > - ret = -EINVAL; > > > > > - goto out_release_state; > > > > > - } > > > > > + ret = of_property_read_string(node, "backend-diskuuid", &diskuuid); > > > > > > > > This needs some documentation in > > > > Documentation/devicetree/bindings/barebox/barebox,state.rst. > > > > > > I can do that. > > > > > > > > + if (ret == 0) { > > > > > + u64 off; > > > > > + > > > > > + ret = devpath_from_diskuuid(diskuuid, &state->backend_path); > > > > > + if (ret) { > > > > > + dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n", > > > > > + diskuuid); > > > > > + goto out_release_state; > > > > > + } > > > > > + ret = of_property_read_u64(node, "backend-offset", &off); > > > > > > > > I stumbled upon this because you have to use a 64bit type here instead > > > > of using 'offset' directly. I think 'offset' should be 64bit instead so > > > > that larger offsets can be used. > > > > > > It's not that simple. 'offset' used as a 'off_t' and 'ssize_t' all over the > > > place in the state framework. On 32bit architecture both are defined as > > > 'long' or 'int'. Both are 32 bit types so changing 'offset' to a 64bit type > > > here doesn't really help. > > > > Of course not, we would have to replace all variables which are used as > > offset into a device to 64bit types. That's a separate topic which > > doesn't have to be solved as part of this series. > > So what should I do here? > - use 'u64' for 'offset' and remove the separate variable > - use 'loff_t' for 'offset' > - keep it as it is > - something else? > > > > > > + } > > > > > + offset = off; > > > > > > > > What about the size of the state partition? This is not set anywhere in > > > > this case so it's still zero. It should be specified the in device tree > > > > as well. At the same time I'm a bit nervous that it apparently still > > > > works with size zero. > > > > > > The code explicitly checks if the size is specified and skips any range > > > checks if it's not. From what I can tell, it has been like that from the > > > beginning. > > > > That's likely ok for real partitions. When reading/writing them past the end > > we'll get an error from the lower layers which can be handled, but not > > so in this case where size is potentially the rest of the whole disk. > > > > > > > > If that's not ok, then I could add a 'backend-size' property, or do you > > > have something else in mind? > > > > Yes, that's what I had in mind. And I think it should be mandatory. > > So, if we add a size, then it should also be validated in some way, right? > Maybe ensure that it does not overlap with existing partitions? > > Michael > > -- > 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 | > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-27 0:18 ` Trent Piepho @ 2022-01-27 12:39 ` Sascha Hauer 2022-01-27 18:53 ` Trent Piepho 0 siblings, 1 reply; 12+ messages in thread From: Sascha Hauer @ 2022-01-27 12:39 UTC (permalink / raw) To: Trent Piepho; +Cc: barebox On Wed, Jan 26, 2022 at 04:18:51PM -0800, Trent Piepho wrote: > DTS ranges are usually specified as <offset> <length> in one property. > The sizes of offset and length fields are done with the #address-cells > and #size-cells of the bus the node is on. I.e., barebox state > shouldn't be defining if offsets or lengths are 32 or 64 bits, it > should/is done by the device the offset or length refers to. > > Like the normal 'reg' property in most nodes for register banks, or > the various "ranges" properties map an address space in the current > node to one in another node. > > This backend-diskuuid, backend-offset, and backend-length seems like a > custom alternative version of a range that is specific to barebox > state. Also, if the backend is a partition defined in the dts, then > the node of the partition specifies its size. But if the partition is > found by uuid, then the barebox state device specifies the size of the > partition. Seems inconsistent. > > It seems like there should be a better and more consistent way to do this. > > Here's an idea. Identify the device by uuid and use existing > fixed-partitions. Example: > > { > compatible = "storage-by-uuid"; > uuid = "abcd-1234"; > // Everything below here already exists and is unchanged > partitions { > compatible = "fixed-partitions"; > barebox_state: state@1000 { > label = "barebox-state"; > reg = <0x1000 0x200>; > }; > barebox_env: env@1200 { > label = "barebox-env"; > reg = <0x1200 0x1000>; > }; > }; > }; > > When the top level node here is found, the matching device is located > by uuid and contents of the node are added to that device. Adding > fixed partitions is done the same way it's already done. The > difference is we can specify the device by uuid instead of needing to > locate the path of the exact hardware device. We discussed this approach internally as well and decided for the way Michael has implemented it for two reasons. First it is easier to implement, if not in barebox, but in Linux userspace where we need to parse that binding as well. Second we don't need another barebox custom binding when we extend the existing custom binding. The fact that with the current approach we have to know that an arbitrary property contains a 64bit value rather than the default 32bit made me think if the approach of having a compatible = "storage-by-uuid" driver is really the better one. I know that Ahmad agrees here as well :) Sascha -- 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-27 12:39 ` Sascha Hauer @ 2022-01-27 18:53 ` Trent Piepho 2022-01-28 7:48 ` Sascha Hauer 0 siblings, 1 reply; 12+ messages in thread From: Trent Piepho @ 2022-01-27 18:53 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox Could a linux dtb fixup be done in barebox? This way userspace will only have existing properties: storage devices with partitions node(s), and barebox-state backend that points to a partition. In this case, the partition node was added by Barebox. In fact, Barebox already does partition fixups exactly like this. So I think this would probably already happen. It won't work if Linux dtb does not have the storage device at all, as the partition fixup considers that an error. But maybe the Linux dtb will have the storage device node? Or if it doesn't have it because the storage is "too dynamic" to put into the dts, then another fixup can make the node. On Thu, Jan 27, 2022 at 4:39 AM Sascha Hauer <sha@pengutronix.de> wrote: > > On Wed, Jan 26, 2022 at 04:18:51PM -0800, Trent Piepho wrote: > > DTS ranges are usually specified as <offset> <length> in one property. > > The sizes of offset and length fields are done with the #address-cells > > and #size-cells of the bus the node is on. I.e., barebox state > > shouldn't be defining if offsets or lengths are 32 or 64 bits, it > > should/is done by the device the offset or length refers to. > > > > Like the normal 'reg' property in most nodes for register banks, or > > the various "ranges" properties map an address space in the current > > node to one in another node. > > > > This backend-diskuuid, backend-offset, and backend-length seems like a > > custom alternative version of a range that is specific to barebox > > state. Also, if the backend is a partition defined in the dts, then > > the node of the partition specifies its size. But if the partition is > > found by uuid, then the barebox state device specifies the size of the > > partition. Seems inconsistent. > > > > It seems like there should be a better and more consistent way to do this. > > > > Here's an idea. Identify the device by uuid and use existing > > fixed-partitions. Example: > > > > { > > compatible = "storage-by-uuid"; > > uuid = "abcd-1234"; > > // Everything below here already exists and is unchanged > > partitions { > > compatible = "fixed-partitions"; > > barebox_state: state@1000 { > > label = "barebox-state"; > > reg = <0x1000 0x200>; > > }; > > barebox_env: env@1200 { > > label = "barebox-env"; > > reg = <0x1200 0x1000>; > > }; > > }; > > }; > > > > When the top level node here is found, the matching device is located > > by uuid and contents of the node are added to that device. Adding > > fixed partitions is done the same way it's already done. The > > difference is we can specify the device by uuid instead of needing to > > locate the path of the exact hardware device. > > We discussed this approach internally as well and decided for the way > Michael has implemented it for two reasons. First it is easier to > implement, if not in barebox, but in Linux userspace where we need to > parse that binding as well. Second we don't need another barebox custom > binding when we extend the existing custom binding. > > The fact that with the current approach we have to know that an > arbitrary property contains a 64bit value rather than the default 32bit > made me think if the approach of having a compatible = > "storage-by-uuid" driver is really the better one. I know that Ahmad > agrees here as well :) > > Sascha > > -- > 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset 2022-01-27 18:53 ` Trent Piepho @ 2022-01-28 7:48 ` Sascha Hauer 0 siblings, 0 replies; 12+ messages in thread From: Sascha Hauer @ 2022-01-28 7:48 UTC (permalink / raw) To: Trent Piepho; +Cc: barebox On Thu, Jan 27, 2022 at 10:53:33AM -0800, Trent Piepho wrote: > Could a linux dtb fixup be done in barebox? This way userspace will > only have existing properties: storage devices with partitions > node(s), and barebox-state backend that points to a partition. In > this case, the partition node was added by Barebox. > > In fact, Barebox already does partition fixups exactly like this. So > I think this would probably already happen. It won't work if Linux > dtb does not have the storage device at all, as the partition fixup > considers that an error. But maybe the Linux dtb will have the > storage device node? Or if it doesn't have it because the storage is > "too dynamic" to put into the dts, then another fixup can make the > node. Michael wants to use this for EFI, so there is no full device tree under Linux, there will only be the device tree snippet containing the state node and now some dummy device describing the disk UUID. While it sounds like an interesting idea it won't help us here. Sascha -- 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 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-28 7:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-24 10:04 [PATCH 0/3] state: find backend with UUID but without a partition Michael Olbrich 2022-01-24 10:04 ` [PATCH 1/3] cdev: rename partuuid to uuid Michael Olbrich 2022-01-24 10:04 ` [PATCH 2/3] cdev: add diskuuid support Michael Olbrich 2022-01-24 10:04 ` [PATCH 3/3] state: support backend-diskuuid / backend-offset Michael Olbrich 2022-01-26 7:57 ` Sascha Hauer 2022-01-26 9:35 ` Michael Olbrich 2022-01-26 10:15 ` Sascha Hauer 2022-01-26 11:16 ` Michael Olbrich 2022-01-27 0:18 ` Trent Piepho 2022-01-27 12:39 ` Sascha Hauer 2022-01-27 18:53 ` Trent Piepho 2022-01-28 7:48 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox