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

For eMMC block, 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 as supported in
JEDEC 4.4+. If this suffix is detected, "partitions-boot0" or
"partitions-boot1" are used instead of the generic "partitions" for the
relevant disk.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 block/partitions/Kconfig  |   8 ++
 block/partitions/Makefile |   1 +
 block/partitions/check.h  |   1 +
 block/partitions/core.c   |   3 +
 block/partitions/of.c     | 151 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 164 insertions(+)
 create mode 100644 block/partitions/of.c

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/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..bc6200eb86b3
--- /dev/null
+++ b/block/partitions/of.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/blkdev.h>
+#include <linux/major.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,
+					       struct gendisk *disk)
+{
+	const char *node_name = "partitions";
+
+	/*
+	 * JEDEC specification 4.4 for eMMC introduced 3 additional partition
+	 * present on every eMMC. These additional partition are always hardcoded
+	 * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
+	 * store keys and exposed as a char device, the other 2 are exposed as
+	 * real separate disk with the boot0/1 appended to the disk name.
+	 *
+	 * Here we parse the disk_name in search for such suffix and select
+	 * the correct partition node.
+	 */
+	if (disk->major == MMC_BLOCK_MAJOR) {
+		const char *disk_name = disk->disk_name;
+
+		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 int validate_of_partition(struct device_node *np, int slot)
+{
+	int a_cells, s_cells;
+	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);
+
+	/*
+	 * Validate offset conversion from bytes to sectors.
+	 * Only the first partition is allowed to have offset 0.
+	 */
+	offset = of_read_number(reg, a_cells);
+	if (do_div(offset, SECTOR_SIZE) ||
+	    (slot > 1 && !offset))
+		return -EINVAL;
+
+	/* Validate size conversion from bytes to sectors */
+	size = of_read_number(reg + a_cells, s_cells);
+	if (do_div(size, SECTOR_SIZE) || !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];
+	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);
+
+	/* Convert bytes to sector size */
+	offset = of_read_number(reg, a_cells) / SECTOR_SIZE;
+	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_node *disk_np, *partitions_np, *np;
+	struct device *ddev = disk_to_dev(state->disk);
+	int slot;
+
+	disk_np = of_node_get(ddev->parent->of_node);
+	if (!disk_np)
+		return 0;
+
+	partitions_np = get_partitions_node(disk_np, state->disk);
+	if (!partitions_np ||
+	    !of_device_is_compatible(partitions_np, "fixed-partitions"))
+		return 0;
+
+	/* Check if child are over the limit */
+	slot = of_get_child_count(partitions_np);
+	if (slot >= state->limit)
+		goto err;
+
+	slot = 1;
+	/* Validate parition offset and size */
+	for_each_child_of_node(partitions_np, np) {
+		if (validate_of_partition(np, slot))
+			goto err;
+
+		slot++;
+	}
+
+	slot = 1;
+	for_each_child_of_node(partitions_np, np) {
+		add_of_partition(state, slot, np);
+
+		slot++;
+	}
+
+	strlcat(state->pp_buf, "\n", PAGE_SIZE);
+
+	return 1;
+err:
+	of_node_put(partitions_np);
+	of_node_put(disk_np);
+	return -1;
+}
-- 
2.45.2

Christian Marangi <ansuelsmth@gmail.com> writes:

> diff --git a/block/partitions/of.c b/block/partitions/of.c
> new file mode 100644
> index 000000000000..bc6200eb86b3
> --- /dev/null
> +++ b/block/partitions/of.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/blkdev.h>
> +#include <linux/major.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,
> +					       struct gendisk *disk)
> +{
> +	const char *node_name = "partitions";
> +
> +	/*
> +	 * JEDEC specification 4.4 for eMMC introduced 3 additional partition
> +	 * present on every eMMC. These additional partition are always hardcoded
> +	 * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
> +	 * store keys and exposed as a char device, the other 2 are exposed as
> +	 * real separate disk with the boot0/1 appended to the disk name.
> +	 *
> +	 * Here we parse the disk_name in search for such suffix and select
> +	 * the correct partition node.
> +	 */
> +	if (disk->major == MMC_BLOCK_MAJOR) {
> +		const char *disk_name = disk->disk_name;
> +
> +		if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT0_STR),
> +			    BOOT0_STR, sizeof(BOOT0_STR)))
> +			node_name = "partitions-boot0";

If strlen(disk_name) is less than 5 (and I don't know if that's actually
possible), this well end up doing out-of-bounds access.

We have a strstarts() helper, could you also add a strends() helper that
handles this correctly? Something like

/**
 * strends - does @str end with @suffix?
 * @str: string to examine
 * @suffix: suffix to look for.
 */
static inline bool strends(const char *str, const char *suffix)
{
	size_t n = strlen(str);
        size_t m = strlen(suffix);
        return n >= m && !memcmp(str + n - m, suffix, m);
}

[or name it str_has_suffix() or str_ends_with(), "strends" is not
particularly readable, it's unfortunate that the existing strstarts is
spelled like that].

Rasmus

On Mon, Sep 30, 2024 at 11:21:53AM +0200, Rasmus Villemoes wrote:
> Christian Marangi <ansuelsmth@gmail.com> writes:
> 
> > diff --git a/block/partitions/of.c b/block/partitions/of.c
> > new file mode 100644
> > index 000000000000..bc6200eb86b3
> > --- /dev/null
> > +++ b/block/partitions/of.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/blkdev.h>
> > +#include <linux/major.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,
> > +					       struct gendisk *disk)
> > +{
> > +	const char *node_name = "partitions";
> > +
> > +	/*
> > +	 * JEDEC specification 4.4 for eMMC introduced 3 additional partition
> > +	 * present on every eMMC. These additional partition are always hardcoded
> > +	 * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
> > +	 * store keys and exposed as a char device, the other 2 are exposed as
> > +	 * real separate disk with the boot0/1 appended to the disk name.
> > +	 *
> > +	 * Here we parse the disk_name in search for such suffix and select
> > +	 * the correct partition node.
> > +	 */
> > +	if (disk->major == MMC_BLOCK_MAJOR) {
> > +		const char *disk_name = disk->disk_name;
> > +
> > +		if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT0_STR),
> > +			    BOOT0_STR, sizeof(BOOT0_STR)))
> > +			node_name = "partitions-boot0";
> 
> If strlen(disk_name) is less than 5 (and I don't know if that's actually
> possible), this well end up doing out-of-bounds access.
> 
> We have a strstarts() helper, could you also add a strends() helper that
> handles this correctly? Something like
> 
> /**
>  * strends - does @str end with @suffix?
>  * @str: string to examine
>  * @suffix: suffix to look for.
>  */
> static inline bool strends(const char *str, const char *suffix)
> {
> 	size_t n = strlen(str);
>         size_t m = strlen(suffix);
>         return n >= m && !memcmp(str + n - m, suffix, m);
> }
> 
> [or name it str_has_suffix() or str_ends_with(), "strends" is not
> particularly readable, it's unfortunate that the existing strstarts is
> spelled like that].
>

Nice idea and thanks for checking the problem with the out-of-bounds
read.

Out of consistency with the unreadable strstarts I'm tempted to use
strends.

Since checking suffix of a string can't be something that unreal I
searched for the 3 function name and to my surprise all 3 suggested name
have a variant of the function statically defined hahaha.

To not pollute this series I will just introduce the helper but I will
add on my TODO list to convert the other function to make use of this
helper instead.

-- 
	Ansuel

Hi Christian,

Thanks for working on this, it will be useful for me as well.
Some comments inside.

On Sun, Sep 29, 2024 at 04:06:19PM +0200, 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.
> 
> 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.
> 
> For eMMC block, 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 as supported in
> JEDEC 4.4+. If this suffix is detected, "partitions-boot0" or
> "partitions-boot1" are used instead of the generic "partitions" for the
> relevant disk.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  block/partitions/Kconfig  |   8 ++
>  block/partitions/Makefile |   1 +
>  block/partitions/check.h  |   1 +
>  block/partitions/core.c   |   3 +
>  block/partitions/of.c     | 151 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 164 insertions(+)
>  create mode 100644 block/partitions/of.c
> 
> 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

Should be "device tree partition support".

> +	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/of.c b/block/partitions/of.c
> new file mode 100644
> index 000000000000..bc6200eb86b3
> --- /dev/null
> +++ b/block/partitions/of.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/blkdev.h>
> +#include <linux/major.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,
> +					       struct gendisk *disk)
> +{
> +	const char *node_name = "partitions";
> +
> +	/*
> +	 * JEDEC specification 4.4 for eMMC introduced 3 additional partition
> +	 * present on every eMMC. These additional partition are always hardcoded
> +	 * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
> +	 * store keys and exposed as a char device, the other 2 are exposed as
> +	 * real separate disk with the boot0/1 appended to the disk name.
> +	 *
> +	 * Here we parse the disk_name in search for such suffix and select
> +	 * the correct partition node.
> +	 */
> +	if (disk->major == MMC_BLOCK_MAJOR) {
> +		const char *disk_name = disk->disk_name;
> +
> +		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 int validate_of_partition(struct device_node *np, int slot)
> +{
> +	int a_cells, s_cells;
> +	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);
> +

The corresponding mtd ofpart parser validates a_cells + s_cells against
len, like this:

	if (len / 4 != a_cells + s_cells) {
		pr_debug("%s: ofpart partition %pOF (%pOF) error parsing reg property.\n",
			 master->name, pp,
			 mtd_node);
		goto ofpart_fail;
	}

I think you should do it here as well.

> +	/*
> +	 * Validate offset conversion from bytes to sectors.
> +	 * Only the first partition is allowed to have offset 0.
> +	 */

Where is this constraint coming from? I would put the partitions in
order into the device tree as well, but the binding doesn't enforce it
and I see no reason to do so.

> +	offset = of_read_number(reg, a_cells);
> +	if (do_div(offset, SECTOR_SIZE) ||

How about (offset % SECTOR_SIZE) or (offset & (SECTOR_SIZE - 1))? Might
be a bit more intuitive to read.

> +	    (slot > 1 && !offset))
> +		return -EINVAL;
> +
> +	/* Validate size conversion from bytes to sectors */
> +	size = of_read_number(reg + a_cells, s_cells);
> +	if (do_div(size, SECTOR_SIZE) || !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];
> +	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);
> +
> +	/* Convert bytes to sector size */
> +	offset = of_read_number(reg, a_cells) / SECTOR_SIZE;
> +	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_node *disk_np, *partitions_np, *np;
> +	struct device *ddev = disk_to_dev(state->disk);
> +	int slot;
> +
> +	disk_np = of_node_get(ddev->parent->of_node);
> +	if (!disk_np)
> +		return 0;
> +
> +	partitions_np = get_partitions_node(disk_np, state->disk);
> +	if (!partitions_np ||
> +	    !of_device_is_compatible(partitions_np, "fixed-partitions"))
> +		return 0;

of_node_put(disk_np) missing here before return.

> +
> +	/* Check if child are over the limit */
> +	slot = of_get_child_count(partitions_np);
> +	if (slot >= state->limit)
> +		goto err;

Other partition parsers just silently ignore the partitions
exceeding state->limit instead of throwing an error. Maybe do the same
here?

> +
> +	slot = 1;
> +	/* Validate parition offset and size */
> +	for_each_child_of_node(partitions_np, np) {
> +		if (validate_of_partition(np, slot))
> +			goto err;
> +
> +		slot++;
> +	}
> +
> +	slot = 1;
> +	for_each_child_of_node(partitions_np, np) {
> +		add_of_partition(state, slot, np);
> +
> +		slot++;
> +	}
> +
> +	strlcat(state->pp_buf, "\n", PAGE_SIZE);
> +
> +	return 1;
> +err:
> +	of_node_put(partitions_np);
> +	of_node_put(disk_np);

You should put the nodes for the non error case 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 |

On Mon, Sep 30, 2024 at 10:14:27AM +0200, Sascha Hauer wrote:
> Hi Christian,
> 
> Thanks for working on this, it will be useful for me as well.
> Some comments inside.
> 
> On Sun, Sep 29, 2024 at 04:06:19PM +0200, 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.
> > 
> > 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.
> > 
> > For eMMC block, 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 as supported in
> > JEDEC 4.4+. If this suffix is detected, "partitions-boot0" or
> > "partitions-boot1" are used instead of the generic "partitions" for the
> > relevant disk.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  block/partitions/Kconfig  |   8 ++
> >  block/partitions/Makefile |   1 +
> >  block/partitions/check.h  |   1 +
> >  block/partitions/core.c   |   3 +
> >  block/partitions/of.c     | 151 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 164 insertions(+)
> >  create mode 100644 block/partitions/of.c
> > 
> > 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
> 
> Should be "device tree partition support".
> 
> > +	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/of.c b/block/partitions/of.c
> > new file mode 100644
> > index 000000000000..bc6200eb86b3
> > --- /dev/null
> > +++ b/block/partitions/of.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/blkdev.h>
> > +#include <linux/major.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,
> > +					       struct gendisk *disk)
> > +{
> > +	const char *node_name = "partitions";
> > +
> > +	/*
> > +	 * JEDEC specification 4.4 for eMMC introduced 3 additional partition
> > +	 * present on every eMMC. These additional partition are always hardcoded
> > +	 * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
> > +	 * store keys and exposed as a char device, the other 2 are exposed as
> > +	 * real separate disk with the boot0/1 appended to the disk name.
> > +	 *
> > +	 * Here we parse the disk_name in search for such suffix and select
> > +	 * the correct partition node.
> > +	 */
> > +	if (disk->major == MMC_BLOCK_MAJOR) {
> > +		const char *disk_name = disk->disk_name;
> > +
> > +		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 int validate_of_partition(struct device_node *np, int slot)
> > +{
> > +	int a_cells, s_cells;
> > +	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);
> > +
> 
> The corresponding mtd ofpart parser validates a_cells + s_cells against
> len, like this:
> 
> 	if (len / 4 != a_cells + s_cells) {
> 		pr_debug("%s: ofpart partition %pOF (%pOF) error parsing reg property.\n",
> 			 master->name, pp,
> 			 mtd_node);
> 		goto ofpart_fail;
> 	}
> 
> I think you should do it here as well.
> 
> > +	/*
> > +	 * Validate offset conversion from bytes to sectors.
> > +	 * Only the first partition is allowed to have offset 0.
> > +	 */
> 
> Where is this constraint coming from? I would put the partitions in
> order into the device tree as well, but the binding doesn't enforce it
> and I see no reason to do so.
>

It's to handle case where offset is 0. But I think I can just check the
value on validation.

> > +	offset = of_read_number(reg, a_cells);
> > +	if (do_div(offset, SECTOR_SIZE) ||
> 
> How about (offset % SECTOR_SIZE) or (offset & (SECTOR_SIZE - 1))? Might
> be a bit more intuitive to read.
> 

do_div was useful to check the result of the division at the same time
but now that I think about it, it's not really needed. Checking the % of
the devision is enough to validate the value are alligned to 512bytes.

> > +	    (slot > 1 && !offset))
> > +		return -EINVAL;
> > +
> > +	/* Validate size conversion from bytes to sectors */
> > +	size = of_read_number(reg + a_cells, s_cells);
> > +	if (do_div(size, SECTOR_SIZE) || !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];
> > +	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);
> > +
> > +	/* Convert bytes to sector size */
> > +	offset = of_read_number(reg, a_cells) / SECTOR_SIZE;
> > +	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_node *disk_np, *partitions_np, *np;
> > +	struct device *ddev = disk_to_dev(state->disk);
> > +	int slot;
> > +
> > +	disk_np = of_node_get(ddev->parent->of_node);
> > +	if (!disk_np)
> > +		return 0;
> > +
> > +	partitions_np = get_partitions_node(disk_np, state->disk);
> > +	if (!partitions_np ||
> > +	    !of_device_is_compatible(partitions_np, "fixed-partitions"))
> > +		return 0;
> 
> of_node_put(disk_np) missing here before return.
> 

Thjanks forgot about it when I added the compatible check.

> > +
> > +	/* Check if child are over the limit */
> > +	slot = of_get_child_count(partitions_np);
> > +	if (slot >= state->limit)
> > +		goto err;
> 
> Other partition parsers just silently ignore the partitions
> exceeding state->limit instead of throwing an error. Maybe do the same
> here?
> 

Ehhh I didn't understand if this is correct or not. Ok I will follow how
it's done by the other.

> > +
> > +	slot = 1;
> > +	/* Validate parition offset and size */
> > +	for_each_child_of_node(partitions_np, np) {
> > +		if (validate_of_partition(np, slot))
> > +			goto err;
> > +
> > +		slot++;
> > +	}
> > +
> > +	slot = 1;
> > +	for_each_child_of_node(partitions_np, np) {
> > +		add_of_partition(state, slot, np);
> > +
> > +		slot++;
> > +	}
> > +
> > +	strlcat(state->pp_buf, "\n", PAGE_SIZE);
> > +
> > +	return 1;
> > +err:
> > +	of_node_put(partitions_np);
> > +	of_node_put(disk_np);
> 
> You should put the nodes for the non error case as well.
> 

ack.

-- 
	Ansuel