Message-ID: <Zvpd48oOYletv7Ko@pengutronix.de>
Date: Mon, 30 Sep 2024 10:14:27 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Jens Axboe <axboe@...nel.dk>, Jonathan Corbet <corbet@....net>,
	Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	INAGAKI Hiroshi <musashino.open@...il.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Christian Brauner <brauner@...nel.org>,
	Al Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
	Li Lingfeng <lilingfeng3@...wei.com>,
	Christian Heusel <christian@...sel.eu>, linux-block@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
	Miquel Raynal <miquel.raynal@...tlin.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>, upstream@...oha.com
Subject: Re: [PATCH v3 3/4] block: add support for partition table defined in
 OF

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

Powered by blists - more mailing lists