Message-ID: <66fa7bb6.5d0a0220.2dcbde.bf69@mx.google.com>
Date: Mon, 30 Sep 2024 12:21:35 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Sascha Hauer <s.hauer@...gutronix.de>
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

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

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

Powered by blists - more mailing lists