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

[Francesco Dolcini <francesco@dolcini.it>]

On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:
> On 1/16/23 15:20, Francesco Dolcini wrote:
> > 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@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@denx.de>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@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@33002000 {
> > 	compatible = "fsl,imx7d-gpmi-nand";
> > 
> > 	#address-cells = <0x01>;
> > 	#size-cells = <0x01>;
> > 
> > [...snip...]
> > 
> > 	partition@0 {
> > 		label = "mx7-bcb";
> > 		reg = <0x00 0x80000>;
> > 	};
> > 
> > 	partition@400000 {
> > 		label = "ubi";
> > 		reg = <0x400000 0x1fc00000>;
> > 	};
> > 
> > 	partition@80000 {
> > 		read_only;
> > 		label = "u-boot1";
> > 		reg = <0x80000 0x180000>;
> > 	};
> > 
> > 	partition@380000 {
> > 		label = "u-boot-env";
> > 		reg = <0x380000 0x80000>;
> > 	};
> > 
> > 	partition@200000 {
> > 		read_only;
> > 		label = "u-boot2";
> > 		reg = <0x200000 0x180000>;
> > 	};
> > };
> 
> This is what I was afraid of, shouldn't this contain the partitions in
> per-chipselect sub-node instead of directly in the GPMI node ?

That does not exists in my source DTS, this function just look for a
partitions node and update it when it exists.
I do not have a nand chip, and I do not want to add.

The reason is what I wrote in my other email, if I would do something
like older U-Boot's would ignore it and just generate the partitions
as direct children of the nand-controller.

Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.

Francesco