| Message ID | 20240923105937.4374-4-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: partition table OF support | expand |
On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote: > +#define BOOT0_STR "boot0" > +#define BOOT1_STR "boot1" > + This boot0/1 stuff looks like black magic, so it should probably be documented at very least. > + partitions_np = get_partitions_node(disk_np, > + state->disk->disk_name); disk->disk_name is not a stable identifier and can change from boot to boot due to async probing. You'll need to check a uuid or label instead.
On Mon, Sep 23, 2024 at 11:34:53PM -0700, Christoph Hellwig wrote: > On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote: > > +#define BOOT0_STR "boot0" > > +#define BOOT1_STR "boot1" > > + > > This boot0/1 stuff looks like black magic, so it should probably be > documented at very least. > It is but from what I have read in the spec for flash in general (this is not limited to eMMC but also apply to UFS) these are hardware partition. If the version is high enough these are always present and have boot0 and boot1 name hardcoded by the driver. > > + partitions_np = get_partitions_node(disk_np, > > + state->disk->disk_name); > > disk->disk_name is not a stable identifier and can change from boot to > boot due to async probing. You'll need to check a uuid or label instead. This is really for the 2 special partition up to check the suffix, we don't really care about the name. I guess it's acceptable to use unstable identifier? Thanks a lot for the review!
On Tue, Sep 24, 2024 at 12:17:36PM +0200, Christian Marangi wrote: > On Mon, Sep 23, 2024 at 11:34:53PM -0700, Christoph Hellwig wrote: > > On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote: > > > +#define BOOT0_STR "boot0" > > > +#define BOOT1_STR "boot1" > > > + > > > > This boot0/1 stuff looks like black magic, so it should probably be > > documented at very least. > > > > It is but from what I have read in the spec for flash in general (this > is not limited to eMMC but also apply to UFS) these are hardware > partition. If the version is high enough these are always present and > have boot0 and boot1 name hardcoded by the driver. How does this belong into generic block layer code? > > > + partitions_np = get_partitions_node(disk_np, > > > + state->disk->disk_name); > > > > disk->disk_name is not a stable identifier and can change from boot to > > boot due to async probing. You'll need to check a uuid or label instead. > > This is really for the 2 special partition up to check the suffix, we > don't really care about the name. I guess it's acceptable to use > unstable identifier? No. ->disk_name is in no way reliable, we can't hardcode that into a partition parser.
On Tue, Oct 01, 2024 at 01:37:05AM -0700, Christoph Hellwig wrote: > On Tue, Sep 24, 2024 at 12:17:36PM +0200, Christian Marangi wrote: > > On Mon, Sep 23, 2024 at 11:34:53PM -0700, Christoph Hellwig wrote: > > > On Mon, Sep 23, 2024 at 12:59:32PM +0200, Christian Marangi wrote: > > > > +#define BOOT0_STR "boot0" > > > > +#define BOOT1_STR "boot1" > > > > + > > > > > > This boot0/1 stuff looks like black magic, so it should probably be > > > documented at very least. > > > > > > > It is but from what I have read in the spec for flash in general (this > > is not limited to eMMC but also apply to UFS) these are hardware > > partition. If the version is high enough these are always present and > > have boot0 and boot1 name hardcoded by the driver. > > How does this belong into generic block layer code? > (just as an info, we are at v4 where I added more info about this) The cmdline partition parser supports this already, just not clearly stated in the code but described in the Documentation example and info. > > > > + partitions_np = get_partitions_node(disk_np, > > > > + state->disk->disk_name); > > > > > > disk->disk_name is not a stable identifier and can change from boot to > > > boot due to async probing. You'll need to check a uuid or label instead. > > > > This is really for the 2 special partition up to check the suffix, we > > don't really care about the name. I guess it's acceptable to use > > unstable identifier? > > No. ->disk_name is in no way reliable, we can't hardcode that into > a partition parser. > Then any hint on this or alternative way? Again this is how it's done with cmdline partition so I'm just following how it's already done. Also I feel it's not clear enough that we really don't care about the identifier, eMMC driver hardcode and always append to disk_name boot0, boot1, the fact that one disk or another might have a different identifier and they change on different boot is not important for the task needed here. I can drop this thing entirely and make the implementation very simple but there are already request and happy dev that would benefits for the additional hardware partition supported by this.
On Tue, Oct 01, 2024 at 11:26:22AM +0200, Christian Marangi wrote: > > No. ->disk_name is in no way reliable, we can't hardcode that into > > a partition parser. > > > > Then any hint on this or alternative way? The normal way would be to use eui/ngui/uuid provided by the storage device. We have a interface for that in the block layer support by scsi and nvme, but I don't know how to wire that up for eMMC which I suspect is what you care about.
On Wed, Oct 02, 2024 at 12:45:37AM -0700, Christoph Hellwig wrote: > On Tue, Oct 01, 2024 at 11:26:22AM +0200, Christian Marangi wrote: > > > No. ->disk_name is in no way reliable, we can't hardcode that into > > > a partition parser. > > > > > > > Then any hint on this or alternative way? > > The normal way would be to use eui/ngui/uuid provided by the storage > device. We have a interface for that in the block layer support by > scsi and nvme, but I don't know how to wire that up for eMMC which > I suspect is what you care about. > I think I have found a better way to handle it, please check v5, did you receive the series? The new series just make the driver pass the partition node and the OF code just take it and use it. This way we don't have to parse the disk name at all and it's driver specific work to select the "partitions" node if multiple are present. I feel your hint produced a much better implementation without having to pollute the block code with specific case.
diff --git a/block/partitions/Kconfig b/block/partitions/Kconfig index 7aff4eb81c60..8534f7544f26 100644 --- a/block/partitions/Kconfig +++ b/block/partitions/Kconfig @@ -270,4 +270,12 @@ 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 "Command line 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 command line 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/of.c b/block/partitions/of.c new file mode 100644 index 000000000000..1c420b7f53c0 --- /dev/null +++ b/block/partitions/of.c @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/blkdev.h> +#include <linux/of.h> +#include "check.h" + +#define BOOT0_STR "boot0" +#define BOOT1_STR "boot1" + +static struct device_node *get_partitions_node(struct device_node *disk_np, + const char *disk_name) +{ + const char *node_name = "partitions"; + + /* Check if we are parsing boot0 or boot1 */ + if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT0_STR), + BOOT0_STR, sizeof(BOOT0_STR))) + node_name = "partitions-boot0"; + if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT1_STR), + BOOT1_STR, sizeof(BOOT1_STR))) + node_name = "partitions-boot1"; + + return of_get_child_by_name(disk_np, node_name); +} + +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]; + int a_cells, s_cells; + const char *partname; + const __be32 *reg; + u64 offset, size; + int len; + + reg = of_get_property(np, "reg", &len); + + a_cells = of_n_addr_cells(np); + s_cells = of_n_size_cells(np); + + offset = of_read_number(reg, a_cells); + size = of_read_number(reg + a_cells, s_cells); + + put_partition(state, slot, offset, size); + + if (of_property_read_bool(pp, "read-only")) + state->parts[slot].flags |= ADDPART_FLAG_READONLY; + + info = &state->parts[slot].info; + partname = of_get_property(np, "label", &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_node *disk_np, *partitions_np, *np; + struct device *ddev = disk_to_dev(state->disk); + int slot = 1; + + disk_np = of_node_get(ddev->parent->of_node); + if (!disk_np) + return 0; + + partitions_np = get_partitions_node(disk_np, + state->disk->disk_name); + if (!partitions_np) + return 0; + + for_each_child_of_node(partitions_np, np) { + if (slot >= state->limit) + return -1; + + add_of_partition(state, slot, np); + + slot++; + } + + strlcat(state->pp_buf, "\n", PAGE_SIZE); + + return 1; +}
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. 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 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. Eventually is also possible to declare the read-only property to flag the partition as read-only. The driver scan the disk name and check if it's suffixed with boot0 or boot1. This is to handle the additional disk provided by eMMC or UFS devices in general. If this suffix is detected, "partitions-boot0" or "partitions-boot1" are used instead of the generic "partitions" Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- block/partitions/Kconfig | 8 ++++ block/partitions/Makefile | 1 + block/partitions/of.c | 85 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+) create mode 100644 block/partitions/of.c