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

[Marek Vasut]

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 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>;
> 	};
> };

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 ?