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@xxxxxxxxx>
> ---
>  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 |