Boot failure regression on 6.0.10 stable kernel on iMX7

[Marek Vasut]

On 12/1/22 12:03, Francesco Dolcini wrote:
> + MTD maintainers/list
> 
> On Wed, Nov 30, 2022 at 11:59:04PM +0100, Marek Vasut wrote:
>> On 11/30/22 21:51, Francesco Dolcini wrote:
>>> On Wed, Nov 30, 2022 at 03:41:13PM +0100, Marek Vasut wrote:
>>>> On 11/30/22 14:52, Francesco Dolcini wrote:
>>>>> [    0.000000] Booting Linux on physical CPU 0x0
>>>>> [    0.000000] Linux version 6.0.10 (francesco at francesco-nb) (arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.
>>>>> 4.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #36 SMP Wed Nov 30 14:07:15 CET 2022
>>>>> ...
>>>>> [    4.407499] gpmi-nand: error parsing ofpart partition /soc/nand-controller at 33002000/partition at 0 (/soc/nand-controller
>>>>> @33002000)
>>>>> [    4.438401] gpmi-nand 33002000.nand-controller: driver registered.
>>>>> ...
>>>>> [    5.933906] VFS: Cannot open root device "ubi0:rootfs" or unknown-block(0,0): error -19
>>>>> [    5.946504] Please append a correct "root=" boot option; here are the available partitions:
>>>>> ...
>>>>>
>>>>> Any idea? I'm not familiar with the gpmi-nand driver and I would just revert it, but
>>>>> maybe you have a better idea.
>>>>
>>>> Can you share the relevant snippet of your nand controller DT node ?
>>>
>>> We just have
>>>
>>> from imx7-colibri.dtsi,
>>>
>>>     &gpmi {
>>>     	fsl,use-minimum-ecc;
>>>     	nand-ecc-mode = "hw";
>>>     	nand-on-flash-bbt;
>>>     	pinctrl-names = "default";
>>>     	pinctrl-0 = <&pinctrl_gpmi_nand>;
>>>     };
>>>
>>> OF partition are created by U-Boot from
>>>     mtdparts=mtdparts=gpmi-nand:512k(mx7-bcb),1536k(u-boot1)ro,1536k(u-boot2)ro,512k(u-boot-env),-(ubi)
>>> env variables calling fdt_fixup_mtdparts from colibri_imx7.c
>>>
>>> Everything is available in the upstream Linux/U-Boot git, no downstream
>>> repo of any sort.
>>>
>>>> Probably up to first partition is enough. I suspect you need to fill in the
>>>> correct address-cells/size-cells there, which might be currently missing in
>>>> your DT and worked by chance.
>>>
>>> This is generated by U-Boot, I would need to dump what he did generate
>>> from the standard fdt_fixup_mtdparts(). I will try to do it tomorrow
>>> unless what I wrote here is already enough to understand what's going
>>> on.
>>
>> Oh drat ... I see. It's the u-boot fdt_node_set_part_info() which checks the
>> current NAND controller #size-cells and uses that when generating MTD
>> partitions 'reg' properties. Since #size-cells is now zero, the reg
>> properties would be malformed.
> 
> I think the issue is slightly different, the u-boot code checks it and
> if not set it defaults to #size-cells = <1>. Said that u-boot
> never set #size-cells anywhere.

Which it really should, can you send a patch there too ?

> What is failing is ofpart_core.c:parse_fixed_partitions() in Linux with
> #size-cells = <0>.
> 
> 
>> Now, what I am unsure is whether the right fix is to update mx7 colibri DT
>> and include &gpmi { #size-cells=<1>; }; , or , revert this patch. The former
>> fixes the problem for colibri and retains the correct #size-cells=<0>
>> behavior for any other board which does not specify MTD partitions in the
>> GPMI NAND node. The later also covers boards which we don't know about which
>> might also use generated MTD partitions in DT using fdt_fixup_mtdparts() in
>> U-Boot, but I am not convinced that is correct.
>>
>> So, would you be OK with fixing up the colibri mx7 DT with #size-cells=<1> ?
> 
> I am also not sure what is the right fix, however I am convinced that
> the fix needs to be in Linux, we cannot really break the boot flow.

I agree

> In a very pragmatic way I could just add the property to colibri-imx7
> dtsi, but we are really breaking potential other users of it, anybody
> using U-Boot to generate the partitions in the end ... (and the list is
> not empty and not just the colibri*).

I agree here too

> Would it make any sense to do something like that (untested!) ?
> 
> diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> index 192190c42fc8..fffd60acd926 100644
> --- a/drivers/mtd/parsers/ofpart_core.c
> +++ b/drivers/mtd/parsers/ofpart_core.c
> @@ -122,6 +122,8 @@ static int parse_fixed_partitions(struct mtd_info *master,
> 
>                  a_cells = of_n_addr_cells(pp);
>                  s_cells = of_n_size_cells(pp);
> +               if (s_cells == 0)
> +                       s_cells = 1; // for backward compatibility
>                  if (len / 4 != a_cells + s_cells) {
>                          pr_debug("%s: ofpart partition %pOF (%pOF) error parsing reg property.\n",
>                                   master->name, pp,

You might want to print a warning too, so users would fix their DTs, 
since once there is MTD partition > 4 GiB, this would break. Otherwise I 
like this option.

Thanks for looking into this !