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