| Message ID | 20241002221306.4403-6-ansuelsmth@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: partition table OF support | expand |
Hi, sorry for not writing sooner. I only noticed this now. On 03.10.24 00:11, Christian Marangi wrote: > Add support for partition table defined in Device Tree. Similar to how > it's done with MTD, add support for defining a fixed partition table in > device tree. > > A common scenario for this is fixed block (eMMC) embedded devices that > have no MBR or GPT partition table to save storage space. Bootloader > access the block device with absolute address of data. How common are these? I never worked with a system that didn't use MBR or GPT for the user partition. > This is to complete the functionality with an equivalent implementation > with providing partition table with bootargs, for case where the booargs > can't be modified and tweaking the Device Tree is the only solution to > have an usabe partition table. > > The implementation follow the fixed-partitions parser used on MTD > devices where a "partitions" node is expected to be declared with > "fixed-partitions" compatible in the OF node of the disk device > (mmc-card for eMMC for example) and each child node declare a label > and a reg with offset and size. If label is not declared, the node name > is used as fallback. Eventually is also possible to declare the read-only > property to flag the partition as read-only. barebox has for many years supported defining fixed partitions on SD/MMC nodes and it's used heavily to define e.g. the location of the barebox environment. Many who do so, do this either before the first partition of the MBR/GPT or overlay the fixed partition to be identical to an existing MBR/GPT partition. barebox also by default copies all fixed partitions it is aware of into the kernel DT, so if the kernel now stops parsing GPT/MBR when a fixed partition node is defined, this would break compatibility of existing barebox-booting systems with new kernels. > +config OF_PARTITION > + bool "Device Tree partition support" if PARTITION_ADVANCED > + depends on OF > + help > + Say Y here if you want to enable support for partition table > + defined in Device Tree. (mainly for eMMC) > + The format for the device tree node is just like MTD fixed-partition > + schema. Thanks for making this configurable and disabled by default, so users won't experience breakage if they just do a make olddefconfig. > diff --git a/block/partitions/core.c b/block/partitions/core.c > index abad6c83db8f..dc21734b00ec 100644 > --- a/block/partitions/core.c > +++ b/block/partitions/core.c > @@ -43,6 +43,9 @@ static int (*const check_part[])(struct parsed_partitions *) = { > #ifdef CONFIG_CMDLINE_PARTITION > cmdline_partition, > #endif > +#ifdef CONFIG_OF_PARTITION > + of_partition, /* cmdline have priority to OF */ > +#endif > #ifdef CONFIG_EFI_PARTITION > efi_partition, /* this must come before msdos */ > #endif If I understand correctly, it's possible to have both partitions-boot1 and a GPT on the user area with your patch, right? So this only leaves the matter of dealing with both fixed-partitions and GPT for the same device node. What are the thoughts on this? An easy way out would be to make of_partition come later than efi_partition/mbr_partition, but I think it would be nice if the kernel could consume partition info out of both of_partition and efi_partition as long they don't collide. Thanks, Ahmad
> > Hi, > > sorry for not writing sooner. I only noticed this now. > > On 03.10.24 00:11, Christian Marangi wrote: > > Add support for partition table defined in Device Tree. Similar to how > > it's done with MTD, add support for defining a fixed partition table in > > device tree. > > > > A common scenario for this is fixed block (eMMC) embedded devices that > > have no MBR or GPT partition table to save storage space. Bootloader > > access the block device with absolute address of data. > > How common are these? I never worked with a system that didn't use MBR > or GPT for the user partition. > On router devices this is the approach for Mediatek and Airoha and also other vendor for anything that have an eMMC. Other device have a mixed nor/nand + eMMC but the eMMC is used entirely for rootfs and not foor bootloader or other special partition. > > This is to complete the functionality with an equivalent implementation > > with providing partition table with bootargs, for case where the booargs > > can't be modified and tweaking the Device Tree is the only solution to > > have an usabe partition table. > > > > The implementation follow the fixed-partitions parser used on MTD > > devices where a "partitions" node is expected to be declared with > > "fixed-partitions" compatible in the OF node of the disk device > > (mmc-card for eMMC for example) and each child node declare a label > > and a reg with offset and size. If label is not declared, the node name > > is used as fallback. Eventually is also possible to declare the read-only > > property to flag the partition as read-only. > > barebox has for many years supported defining fixed partitions on SD/MMC > nodes and it's used heavily to define e.g. the location of the barebox > environment. Many who do so, do this either before the first partition > of the MBR/GPT or overlay the fixed partition to be identical to > an existing MBR/GPT partition. > > barebox also by default copies all fixed partitions it is aware of > into the kernel DT, so if the kernel now stops parsing GPT/MBR when > a fixed partition node is defined, this would break compatibility of > existing barebox-booting systems with new kernels. > I'm not following... is that a downstream thing? Also fixed-partition in DT for SD/MMC were never supported, why the partition was copied in DT? Userspace tools made use of them? > > +config OF_PARTITION > > + bool "Device Tree partition support" if PARTITION_ADVANCED > > + depends on OF > > + help > > + Say Y here if you want to enable support for partition table > > + defined in Device Tree. (mainly for eMMC) > > + The format for the device tree node is just like MTD fixed-partition > > + schema. > > Thanks for making this configurable and disabled by default, so users > won't experience breakage if they just do a make olddefconfig. > > > diff --git a/block/partitions/core.c b/block/partitions/core.c > > index abad6c83db8f..dc21734b00ec 100644 > > --- a/block/partitions/core.c > > +++ b/block/partitions/core.c > > @@ -43,6 +43,9 @@ static int (*const check_part[])(struct parsed_partitions *) = { > > #ifdef CONFIG_CMDLINE_PARTITION > > cmdline_partition, > > #endif > > +#ifdef CONFIG_OF_PARTITION > > + of_partition, /* cmdline have priority to OF */ > > +#endif > > #ifdef CONFIG_EFI_PARTITION > > efi_partition, /* this must come before msdos */ > > #endif > > If I understand correctly, it's possible to have both partitions-boot1 and > a GPT on the user area with your patch, right? > No, this array works by, first is found WIN. If OF_PARTITION is enabled and an OF partition is declared in DT, then efi partition parse is skipped. > So this only leaves the matter of dealing with both fixed-partitions and > GPT for the same device node. > The logic is applied to skip exactly this scenario. GPT partition can be edited at runtime and change, DT is more deterministic. It's one or the other. If downstream someone have GPT then OF_PARTITION should not be used at all... Eventually downstream for this special approach, an additional downstream patch can be added that define a special property in the node to disable OF parsing. (it's a 3 line patch and since everything is downstream it really doesn't matter) > What are the thoughts on this? An easy way out would be to make of_partition > come later than efi_partition/mbr_partition, but I think it would be > nice if the kernel could consume partition info out of both of_partition > and efi_partition as long they don't collide. > The 2 thing would conflicts and would introduce so much complexity it might be not worth at all. Also you would have situation where someone declare OF partition in the space where the GPT partition table is located, adding the possibility of corrupting it. Again would love more explanation of your case because by the looks of it, you use GPT for partition parsing and just overload the DT with the additional info maybe for userspace usage. (and that case can be handled by just keeping OF_PARTITION disabled or adding a little downstream patch) Or you are telling me you had a downstream patch that declares additional partition in addition to a disk with a GPT partition table? If that's the case, I'm confused of why the additional partition can't be declared directly in GPT.
Hello Christian, Thanks for the prompt response. On 05.12.24 12:54, Christian Marangi (Ansuel) wrote: >> How common are these? I never worked with a system that didn't use MBR >> or GPT for the user partition. >> > > On router devices this is the approach for Mediatek and Airoha and also > other vendor for anything that have an eMMC. Good to know. Thanks. >> barebox has for many years supported defining fixed partitions on SD/MMC >> nodes and it's used heavily to define e.g. the location of the barebox >> environment. Many who do so, do this either before the first partition >> of the MBR/GPT or overlay the fixed partition to be identical to >> an existing MBR/GPT partition. >> >> barebox also by default copies all fixed partitions it is aware of >> into the kernel DT, so if the kernel now stops parsing GPT/MBR when >> a fixed partition node is defined, this would break compatibility of >> existing barebox-booting systems with new kernels. >> > > I'm not following... is that a downstream thing? Also fixed-partition > in DT for SD/MMC were never supported, why the partition was > copied in DT? Userspace tools made use of them? The kernel isn't modified, but the barebox-state utility can parse the fixed partitions in the DT and map it via udev to a block device partition (if one exists) or to a block device + offset. >> If I understand correctly, it's possible to have both partitions-boot1 and >> a GPT on the user area with your patch, right? >> > > No, this array works by, first is found WIN. If OF_PARTITION is enabled > and an OF partition is declared in DT, then efi partition parse is skipped. Yes, but Boot partitions and the user area are different block devices, so it should be possible to use OF partition for the boot partitions and GPT for the user area, right? >> So this only leaves the matter of dealing with both fixed-partitions and >> GPT for the same device node. >> > > The logic is applied to skip exactly this scenario. GPT partition can > be edited at > runtime and change, DT is more deterministic. It's one or the other. > > If downstream someone have GPT then OF_PARTITION should not > be used at all... Eventually downstream for this special approach, an additional > downstream patch can be added that define a special property in the node to > disable OF parsing. (it's a 3 line patch and since everything is downstream it > really doesn't matter) As mentioned, the kernel itself isn't patched, but there was an implicit assumption that MBR/GPT parsing would continue to work, even when a fixed partition node is specified... >> What are the thoughts on this? An easy way out would be to make of_partition >> come later than efi_partition/mbr_partition, but I think it would be >> nice if the kernel could consume partition info out of both of_partition >> and efi_partition as long they don't collide. >> > > The 2 thing would conflicts and would introduce so much complexity it might > be not worth at all. Also you would have situation where someone declare > OF partition in the space where the GPT partition table is located, adding > the possibility of corrupting it. If we go this way, the implementation should of course refuse creating partitions that conflict. The barebox implementation allows partitions only in the unpartitioned space or to be identical to an existing GPT partition. But I agree, this needs to be thought through thoroughly to determine how it should interact with runtime repartitioning. > Again would love more explanation of your case because by the looks of it, > you use GPT for partition parsing and just overload the DT with the additional > info maybe for userspace usage. (and that case can be handled by just keeping > OF_PARTITION disabled or adding a little downstream patch) Yes, keeping OF_PARTITION disabled would be required to not break barebox users that made use of the non-upstream binding. I wonder though, if something can be done to reconcile Linux and barebox' view of this and allow in the future enabling both Linux OF_PARTITION and barebox OF partition fixups. > Or you are telling me you had a downstream patch that declares additional > partition in addition to a disk with a GPT partition table? > If that's the case, I'm confused of why the additional partition can't > be declared > directly in GPT. Many of the older boards supported by barebox used to place the barebox image and the environment prior to the first partition in the unpartitioned area. To still be able to access them, fixed partitions were used and the rest of the system was described by MBR/GPT partitions. This was partially made necessary by BootROMs having strange expectations of where the bootloader needs to be placed, which partially overlapped the MBR/GPT itself, making it difficult to define a partition for the bootloader. For newer boards, it's more common to place the bootloader in a GPT partition now. barebox has no DT binding for generically describing such a GPT partition though, so boards may create a fixed-partition "alias" and use that. Cheers, Ahmad
diff --git a/block/partitions/Kconfig b/block/partitions/Kconfig index 7aff4eb81c60..ce17e41451af 100644 --- a/block/partitions/Kconfig +++ b/block/partitions/Kconfig @@ -270,4 +270,13 @@ config CMDLINE_PARTITION Say Y here if you want to read the partition table from bootargs. The format for the command line is just like mtdparts. +config OF_PARTITION + bool "Device Tree partition support" if PARTITION_ADVANCED + depends on OF + help + Say Y here if you want to enable support for partition table + defined in Device Tree. (mainly for eMMC) + The format for the device tree node is just like MTD fixed-partition + schema. + endmenu diff --git a/block/partitions/Makefile b/block/partitions/Makefile index a7f05cdb02a8..25d424922c6e 100644 --- a/block/partitions/Makefile +++ b/block/partitions/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_CMDLINE_PARTITION) += cmdline.o obj-$(CONFIG_MAC_PARTITION) += mac.o obj-$(CONFIG_LDM_PARTITION) += ldm.o obj-$(CONFIG_MSDOS_PARTITION) += msdos.o +obj-$(CONFIG_OF_PARTITION) += of.o obj-$(CONFIG_OSF_PARTITION) += osf.o obj-$(CONFIG_SGI_PARTITION) += sgi.o obj-$(CONFIG_SUN_PARTITION) += sun.o diff --git a/block/partitions/check.h b/block/partitions/check.h index 8d70a880c372..e5c1c61eb353 100644 --- a/block/partitions/check.h +++ b/block/partitions/check.h @@ -62,6 +62,7 @@ int karma_partition(struct parsed_partitions *state); int ldm_partition(struct parsed_partitions *state); int mac_partition(struct parsed_partitions *state); int msdos_partition(struct parsed_partitions *state); +int of_partition(struct parsed_partitions *state); int osf_partition(struct parsed_partitions *state); int sgi_partition(struct parsed_partitions *state); int sun_partition(struct parsed_partitions *state); diff --git a/block/partitions/core.c b/block/partitions/core.c index abad6c83db8f..dc21734b00ec 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -43,6 +43,9 @@ static int (*const check_part[])(struct parsed_partitions *) = { #ifdef CONFIG_CMDLINE_PARTITION cmdline_partition, #endif +#ifdef CONFIG_OF_PARTITION + of_partition, /* cmdline have priority to OF */ +#endif #ifdef CONFIG_EFI_PARTITION efi_partition, /* this must come before msdos */ #endif diff --git a/block/partitions/of.c b/block/partitions/of.c new file mode 100644 index 000000000000..4e760fdffb3f --- /dev/null +++ b/block/partitions/of.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/blkdev.h> +#include <linux/major.h> +#include <linux/of.h> +#include <linux/string.h> +#include "check.h" + +static int validate_of_partition(struct device_node *np, int slot) +{ + u64 offset, size; + int len; + + const __be32 *reg = of_get_property(np, "reg", &len); + int a_cells = of_n_addr_cells(np); + int s_cells = of_n_size_cells(np); + + /* Make sure reg len match the expected addr and size cells */ + if (len / sizeof(*reg) != a_cells + s_cells) + return -EINVAL; + + /* Validate offset conversion from bytes to sectors */ + offset = of_read_number(reg, a_cells); + if (offset % SECTOR_SIZE) + return -EINVAL; + + /* Validate size conversion from bytes to sectors */ + size = of_read_number(reg + a_cells, s_cells); + if (!size || size % SECTOR_SIZE) + return -EINVAL; + + return 0; +} + +static void add_of_partition(struct parsed_partitions *state, int slot, + struct device_node *np) +{ + struct partition_meta_info *info; + char tmp[sizeof(info->volname) + 4]; + const char *partname; + int len; + + const __be32 *reg = of_get_property(np, "reg", &len); + int a_cells = of_n_addr_cells(np); + int s_cells = of_n_size_cells(np); + + /* Convert bytes to sector size */ + u64 offset = of_read_number(reg, a_cells) / SECTOR_SIZE; + u64 size = of_read_number(reg + a_cells, s_cells) / SECTOR_SIZE; + + put_partition(state, slot, offset, size); + + if (of_property_read_bool(np, "read-only")) + state->parts[slot].flags |= ADDPART_FLAG_READONLY; + + /* + * Follow MTD label logic, search for label property, + * fallback to node name if not found. + */ + info = &state->parts[slot].info; + partname = of_get_property(np, "label", &len); + if (!partname) + partname = of_get_property(np, "name", &len); + strscpy(info->volname, partname, sizeof(info->volname)); + + snprintf(tmp, sizeof(tmp), "(%s)", info->volname); + strlcat(state->pp_buf, tmp, PAGE_SIZE); +} + +int of_partition(struct parsed_partitions *state) +{ + struct device *ddev = disk_to_dev(state->disk); + struct device_node *np; + int slot; + + struct device_node *partitions_np = of_node_get(ddev->of_node); + + if (!partitions_np || + !of_device_is_compatible(partitions_np, "fixed-partitions")) + return 0; + + slot = 1; + /* Validate parition offset and size */ + for_each_child_of_node(partitions_np, np) { + if (validate_of_partition(np, slot)) { + of_node_put(np); + of_node_put(partitions_np); + + return -1; + } + + slot++; + } + + slot = 1; + for_each_child_of_node(partitions_np, np) { + if (slot >= state->limit) { + of_node_put(np); + break; + } + + add_of_partition(state, slot, np); + + slot++; + } + + strlcat(state->pp_buf, "\n", PAGE_SIZE); + + return 1; +}