[PATCH v3 0/4] block: partition table OF support

[PATCH v3 0/4] block: partition table OF support

[Christian Marangi]

Hi,
this is an initial proposal to complete support for manually defining
partition table.

Some background on this. Many OEM on embedded device (modem, router...)
are starting to migrate from NOR/NAND flash to eMMC. The reason for this
is that OEM are starting to require more and more space for the firmware
and price difference is becoming so little that using eMMC is only benefits
and no cons.

Given these reason, OEM are also using very custom way to provide a
partition table and doesn't relay on common method like writing a table
on the eMMC.

One way that is commonly used is to hardcode the partition table and
pass it to the system via various way (cmdline, special glue driver,
block2mtd...)
This way is also used on Android where the partition table
is passed from the bootloader via cmdline.

One reason to use this method is to save space on the device and to
permit more flexibility on partition handling.

What this series does is complete support for this feature.
It's possible to use the cmdline to define a partition table similar
to how it's done for MTD but this is problematic for a number of device
where tweaking the cmdline is not possible. This series adds OF support
to make it possible to define a partition table in the Device Tree.

We implement a similar schema to the MTD fixed-partition, where we define
a "label" and a "reg" with "offset" and "size".

A new block partition parser is introduced that check if the block device
have an OF node attached and check if a fixed-partition table is defined.

If a correct node is found, then partition table is filled. cmdline will
still have priority to this new parser.

Some block device also implement boot1 and boot2 additional disk. Similar
to the cmdline parser, these disk can have OF support using the
"partitions-boot0" and "partitions-boot1" additional node.

It's also completed support for declaring partition as read-only as this
feature was introduced but never finished in the cmdline parser.

Posting as RFC for any comments or additional checks on OF parser code.

I hope this solution is better accepted as downstream this is becoming
a real problem with a growing number of strange solution for the simple
task of providing a fixed partition table.

Changes v3:
- Out of RFC
- Drop partition schema generalization and simplify it
- Require fixed-partitions compatible to adapt to MTD schema
- Make label property optional and fallback to node name
Changes v2:
- Reference bytes in DT instead of Sector Size
- Validate offset and size after Sector Size conversion
- Limit boot0 and boot1 to eMMC and add comments about JEDEC spec
- Generalize MTD partition schema and introduce block partitions schema
- Add missing code to actually attach the OF parser to block partition core
- Add reviewed by tag for read-only patch

Christian Marangi (4):
  block: add support for defining read-only partitions
  docs: block: Document support for read-only partition in cmdline part
  block: add support for partition table defined in OF
  dt-bindings: mmc: Document support for partition table in mmc-card

 Documentation/block/cmdline-partition.rst     |   5 +-
 .../devicetree/bindings/mmc/mmc-card.yaml     |  52 ++++++
 block/blk.h                                   |   1 +
 block/partitions/Kconfig                      |   8 +
 block/partitions/Makefile                     |   1 +
 block/partitions/check.h                      |   1 +
 block/partitions/cmdline.c                    |   3 +
 block/partitions/core.c                       |   6 +
 block/partitions/of.c                         | 151 ++++++++++++++++++
 9 files changed, 227 insertions(+), 1 deletion(-)
 create mode 100644 block/partitions/of.c

-- 
2.45.2

[PATCH v3 1/4] block: add support for defining read-only partitions

[Christian Marangi]

Add support for defining read-only partitions and complete support for
it in the cmdline partition parser as the additional "ro" after a
partition is scanned but never actually applied.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk.h                | 1 +
 block/partitions/cmdline.c | 3 +++
 block/partitions/core.c    | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/block/blk.h b/block/blk.h
index c718e4291db0..f300212d3e98 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -558,6 +558,7 @@ void blk_free_ext_minor(unsigned int minor);
 #define ADDPART_FLAG_NONE	0
 #define ADDPART_FLAG_RAID	1
 #define ADDPART_FLAG_WHOLEDISK	2
+#define ADDPART_FLAG_READONLY	4
 int bdev_add_partition(struct gendisk *disk, int partno, sector_t start,
 		sector_t length);
 int bdev_del_partition(struct gendisk *disk, int partno);
diff --git a/block/partitions/cmdline.c b/block/partitions/cmdline.c
index 152c85df92b2..da3e719d8e51 100644
--- a/block/partitions/cmdline.c
+++ b/block/partitions/cmdline.c
@@ -237,6 +237,9 @@ static int add_part(int slot, struct cmdline_subpart *subpart,
 	put_partition(state, slot, subpart->from >> 9,
 		      subpart->size >> 9);
 
+	if (subpart->flags & PF_RDONLY)
+		state->parts[slot].flags |= ADDPART_FLAG_READONLY;
+
 	info = &state->parts[slot].info;
 
 	strscpy(info->volname, subpart->name, sizeof(info->volname));
diff --git a/block/partitions/core.c b/block/partitions/core.c
index ab76e64f0f6c..abad6c83db8f 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -373,6 +373,9 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
 			goto out_del;
 	}
 
+	if (flags & ADDPART_FLAG_READONLY)
+		bdev_set_flag(bdev, BD_READ_ONLY);
+
 	/* everything is up and running, commence */
 	err = xa_insert(&disk->part_tbl, partno, bdev, GFP_KERNEL);
 	if (err)
-- 
2.45.2

[PATCH v3 2/4] docs: block: Document support for read-only partition in cmdline part

[Christian Marangi]

Document support for read-only partition in cmdline partition for block
devices by appending "ro" after the (partition name).

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/block/cmdline-partition.rst | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/block/cmdline-partition.rst b/Documentation/block/cmdline-partition.rst
index 530bedff548a..526ba201dddc 100644
--- a/Documentation/block/cmdline-partition.rst
+++ b/Documentation/block/cmdline-partition.rst
@@ -39,13 +39,16 @@ blkdevparts=<blkdev-def>[;<blkdev-def>]
     create a link to block device partition with the name "PARTNAME".
     User space application can access partition by partition name.
 
+ro
+    read-only. Flag the partition as read-only.
+
 Example:
 
     eMMC disk names are "mmcblk0" and "mmcblk0boot0".
 
   bootargs::
 
-    'blkdevparts=mmcblk0:1G(data0),1G(data1),-;mmcblk0boot0:1m(boot),-(kernel)'
+    'blkdevparts=mmcblk0:1G(data0),1G(data1),-;mmcblk0boot0:1m(boot)ro,-(kernel)'
 
   dmesg::
 
-- 
2.45.2

[PATCH v3 3/4] block: add support for partition table defined in OF

[Christian Marangi]

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

[PATCH v3 4/4] dt-bindings: mmc: Document support for partition table in mmc-card

[Christian Marangi]

Document support for defining a partition table in the mmc-card node.

This is needed if the eMMC doesn't have a partition table written and
the bootloader of the device load data by using absolute offset of the
block device. This is common on embedded device that have eMMC installed
to save space and have non removable block devices.

If an OF partition table is detected, any partition table written in the
eMMC will be ignored and won't be parsed.

eMMC provide a generic disk for user data and if supported (JEDEC 4.4+)
also provide two additional disk ("boot0" and "boot1") for special usage
of boot operation where normally is stored the bootloader or boot info.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/mmc/mmc-card.yaml     | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-card.yaml b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
index fd347126449a..5f93bb77f246 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-card.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-card.yaml
@@ -13,6 +13,10 @@ description: |
   This documents describes the devicetree bindings for a mmc-host controller
   child node describing a mmc-card / an eMMC.
 
+  It's possible to define a fixed partition table for an eMMC for the user
+  partition and one of the 2 boot partition (boot0/boot1) if supported by the
+  eMMC.
+
 properties:
   compatible:
     const: mmc-card
@@ -26,6 +30,24 @@ properties:
       Use this to indicate that the mmc-card has a broken hpi
       implementation, and that hpi should not be used.
 
+patternProperties:
+  "^partitions(-boot[01])?$":
+    $ref: /schemas/mtd/partitions/partitions.yaml
+
+    patternProperties:
+      "^partition@[0-9a-f]+$":
+        $ref: /schemas/mtd/partitions/partition.yaml
+
+        properties:
+          reg:
+            description: Must be multiple of 512 as it's converted
+              internally from bytes to SECTOR_SIZE (512 bytes)
+
+        required:
+          - reg
+
+        unevaluatedProperties: false
+
 required:
   - compatible
   - reg
@@ -42,6 +64,36 @@ examples:
             compatible = "mmc-card";
             reg = <0>;
             broken-hpi;
+
+            partitions {
+                compatible = "fixed-partitions";
+
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                partition@0 {
+                    label = "kernel"; /* Kernel */
+                    reg = <0x0 0x2000000>; /* 32 MB */
+                };
+
+                partition@2000000 {
+                    label = "rootfs";
+                    reg = <0x2000000 0x40000000>; /* 1GB */
+                };
+            };
+
+            partitions-boot0 {
+                compatible = "fixed-partitions";
+
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                partition@0 {
+                    label = "bl";
+                    reg = <0x0 0x2000000>; /* 32MB */
+                    read-only;
+                };
+            };
         };
     };
 
-- 
2.45.2

Re: [PATCH v3 3/4] block: add support for partition table defined in OF

[Sascha Hauer]

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 |

Re: [PATCH v3 3/4] block: add support for partition table defined in OF

[Rasmus Villemoes]

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

Re: [PATCH v3 3/4] block: add support for partition table defined in OF

[Christian Marangi]

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

Re: [PATCH v3 3/4] block: add support for partition table defined in OF

[Christian Marangi]

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