[PATCH v1 1/3] fdt: validate/fix cells count on mtdpart fixup

[Francesco Dolcini]

On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
> On 1/13/23 19:45, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini at toradex.com>
> > 
> > Fixup #size-cells value when updating the MTD partitions, this is
> > required to prevent issues in case the MTD parent set #size-cells to
> > zero.
> > This could happen for example in the legacy case in which the partitions
> > are created as direct child of the mtd node and that specific node has
> > no children. Recent clean-up on Linux device tree files created a boot
> > regression on colibri-imx7.
> > 
> > This fixup has the limitation to assume 32-bit (#size-cells=1)
> > addressing, therefore it will not work with device bigger than 4GiB.
> > 
> > This change also enforce #address-cells to be the same as #size-cells,
> > this was already silently enforced by fdt_node_set_part_info(), now this
> > is checked explicitly and partition fixup will just fail in such case.
> > 
> > In general board should not generally need nor use this functionality
> > and should be just deprecated, passing mtdparts= in the kernel command
> > line is the preferred way according to Linux MTD subsystem maintainer.
> > 
> > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > Cc: Marek Vasut <marex at denx.de>
> > Cc: Miquel Raynal <miquel.raynal at bootlin.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini at toradex.com>
> > ---
> >   common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
> >   1 file changed, 35 insertions(+), 10 deletions(-)
> > 
> > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > index dbceec6f2dcc..3aee826e60cf 100644
> > --- a/common/fdt_support.c
> > +++ b/common/fdt_support.c
> > @@ -877,27 +877,20 @@ static int fdt_del_partitions(void *blob, int parent_offset)
> >   	return 0;
> >   }
> > -static int fdt_node_set_part_info(void *blob, int parent_offset,
> > +/* This expects #address-cells and #size-cells to have same value */
> > +static int fdt_node_set_part_info(void *blob, int parent_offset, int sizecell,
> >   				  struct mtd_device *dev)
> >   {
> >   	struct list_head *pentry;
> >   	struct part_info *part;
> >   	int off, ndepth = 0;
> >   	int part_num, ret;
> > -	int sizecell;
> >   	char buf[64];
> >   	ret = fdt_del_partitions(blob, parent_offset);
> >   	if (ret < 0)
> >   		return ret;
> > -	/*
> > -	 * Check if size/address is 1 or 2 cells.
> > -	 * We assume #address-cells and #size-cells have same value.
> > -	 */
> > -	sizecell = fdt_getprop_u32_default_node(blob, parent_offset,
> > -						0, "#size-cells", 1);
> > -
> >   	/*
> >   	 * Check if it is nand {}; subnode, adjust
> >   	 * the offset in this case
> > @@ -992,6 +985,31 @@ err_prop:
> >   	return ret;
> >   }
> > +static int fdt_mtdparts_cell_cnt(void *fdt, int off)
> > +{
> > +	int sizecell, addrcell;
> > +
> > +	sizecell = fdt_getprop_u32_default_node(fdt, off, 0, "#size-cells", 0);
> > +	if (sizecell != 1 && sizecell != 2) {
> > +		printf("%s: Invalid or missing #size-cells %d value, assuming 1\n",
> > +		       __func__, sizecell);
> > +
> > +		sizecell = 1;
> > +		if (fdt_setprop_u32(fdt, off, "#size-cells", sizecell))
> > +			return -1;
> > +	}
> > +
> > +	addrcell = fdt_getprop_u32_default_node(fdt, off, 0,
> > +						"#address-cells", 0);
> > +	if (addrcell != sizecell) {
> > +		printf("%s: Invalid #address-cells %d != #size-cells %d, aborting\n",
> > +		       __func__, addrcell, sizecell);
> > +		return -1;
> > +	}
> > +
> > +	return sizecell;
> > +}
> > +
> >   /*
> >    * Update partitions in nor/nand nodes using info from
> >    * mtdparts environment variable. The nodes to update are
> > @@ -1037,12 +1055,19 @@ void fdt_fixup_mtdparts(void *blob, const struct node_info *node_info,
> >   			dev = device_find(node_info[i].type, idx++);
> >   			if (dev) {
> > +				int cell;
> > +
> >   				parts = fdt_subnode_offset(blob, noff,
> >   							   "partitions");
> >   				if (parts < 0)
> >   					parts = noff;
> > -				if (fdt_node_set_part_info(blob, parts, dev))
> > +				cell = fdt_mtdparts_cell_cnt(blob, parts);
> > +				if (cell < 0)
> > +					return;
> > +
> > +				if (fdt_node_set_part_info(blob, parts,
> > +							   cell, dev))
> >   					return; /* return on error */
> >   			}
> >   		}
> 
> Can you please include the resulting gpmi node content with this fixup
> applied in the commit message , so it can be validated ?

I will add it to v2, I would wait a little bit more time to get
additional feedback sending it however.

In the meantime here the output, but nothing really changed!
What this change is doing is just
 - setting #size-cells to <1> when it is invalid
 - skip generation at all when #size-cells != #address-cells. Former
   code was just generating a broken table without any error
   message.

Here what is generated for colibri-imx7

nand-controller at 33002000 {
	compatible = "fsl,imx7d-gpmi-nand";

	#address-cells = <0x01>;
	#size-cells = <0x01>;

[...snip...]

	partition at 0 {
		label = "mx7-bcb";
		reg = <0x00 0x80000>;
	};

	partition at 400000 {
		label = "ubi";
		reg = <0x400000 0x1fc00000>;
	};

	partition at 80000 {
		read_only;
		label = "u-boot1";
		reg = <0x80000 0x180000>;
	};

	partition at 380000 {
		label = "u-boot-env";
		reg = <0x380000 0x80000>;
	};

	partition at 200000 {
		read_only;
		label = "u-boot2";
		reg = <0x200000 0x180000>;
	};
};