v2023.07-rc5 regression: Image overlaps SPL

[Tom Rini]

On Tue, Jul 04, 2023 at 11:39:59AM -0300, Fabio Estevam wrote:
> Hi Francesco,
> 
> On Mon, Jul 3, 2023 at 5:49 PM Francesco Dolcini <francesco at dolcini.it> wrote:
> 
> > If I do this small partial revert
> >
> > --- a/arch/arm/dts/imx7d-colibri-eval-v3-u-boot.dtsi
> > +++ b/arch/arm/dts/imx7d-colibri-eval-v3-u-boot.dtsi
> > @@ -15,7 +15,8 @@
> >         pinctrl-0 = <&pinctrl_lcdif_dat
> >                      &pinctrl_lcdif_ctrl>;
> >         display = <&display0>;
> > -       u-boot,dm-pre-reloc;
> > +       bootph-all;
> 
> I managed to reproduce the behavior on a imx7d-sabresd by doing:
> 
> --- a/board/freescale/mx7dsabresd/mx7dsabresd.c
> +++ b/board/freescale/mx7dsabresd/mx7dsabresd.c
> @@ -25,6 +25,7 @@
>  #include <i2c.h>
>  #include <asm/mach-imx/mxc_i2c.h>
>  #include <asm/arch/crm_regs.h>
> +#include <fdt_support.h>
> 
>  DECLARE_GLOBAL_DATA_PTR;
> 
> @@ -289,6 +290,20 @@ int power_init_board(void)
>  }
>  #endif
> 
> +int board_fix_fdt(void *rw_fdt_blob)
> +{
> +       int ret;
> +
> +       int offset = fdt_path_offset(rw_fdt_blob,
> "/soc/bus at 30800000/usb at 30b20000");
> +
> +       ret = fdt_status_disabled(rw_fdt_blob, offset);
> +
> +       printf("******** offset is 0x%x\n", offset);
> +       printf("******** ret is %d\n", ret);
> +
> +       return 0;
> +}
> +
>  int board_late_init(void)
> 
> --- a/configs/mx7dsabresd_defconfig
> +++ b/configs/mx7dsabresd_defconfig
> @@ -86,3 +86,4 @@ CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5
>  CONFIG_CI_UDC=y
>  CONFIG_USB_GADGET_DOWNLOAD=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_OF_BOARD_FIXUP=y
> 
> With top of tree U-Boot it fails with:
> 
> ******** offset is 0x7ba4
> ******** ret is -3
> 
> If u-boot.dtsi gets smaller, for example, by reverting 0aea5dda2928
> then it succeeds:
> 
> ******** offset is 0x7988
> ******** ret is 0
> 
> > fdt_status_disabled() returns 0 again.
> >
> > With the current master, fdt_status_disabled() returns -3,
> > -FDT_ERR_NOSPACE, and I assume I could just change my code to call
> > fdt_increase_size() and call it done.
> >
> > However, what the reason for this different behavior triggered by that
> > change in the *-u-boot.dtsi ? Is this expected?
> >
> > From a quick check multiple place in the code just disable/enable nodes
> > or set properties without taking care of this, are those going to be
> > affected by that commit that created the regression? Are those all
> > buggy?
> >
> > $ git grep fdt_setprop |wc -l
> > 461
> >
> > We have some helper around, for example
> > arch/arm/mach-imx/imx8/fdt.c:disable_fdt_node(), but this is for example
> > just specific on that file.
> 
> Here is my suggestion: let's increase the fdt size locally on your
> board for now, just like turris_omnia.c:
> 
> --- a/board/toradex/colibri_imx7/colibri_imx7.c
> +++ b/board/toradex/colibri_imx7/colibri_imx7.c
> @@ -315,6 +315,15 @@ int board_fix_fdt(void *rw_fdt_blob)
>         if (is_cpu_type(MXC_CPU_MX7S)) {
>                 int offset = fdt_path_offset(rw_fdt_blob,
> "/soc/bus at 30800000/usb at 30b20000");
> 
> +       /*
> +        * We're either adding status = "disabled" property, or changing
> +        * status = "okay" to status = "disabled". In both cases we'll need more
> +        * space. Increase the size a little.
> +        */
> +       if (fdt_increase_size(rw_fdt_blob, 32) < 0) {
> +               printf("Cannot increase FDT size.\n");
> +               return -ENOMEM;
> +       }
>                 return fdt_status_disabled(rw_fdt_blob, offset);
>         }
> 
> Then for the next cycle, we should plan on adding this
> fdt_increase_size() into the common fdt_status_disabled().

I'm a little leary of generic changes here having an unexpected size /
performance impact.  The API specifically does not "just" handle this
case like it does for some others. We should update the docs around
fdt_set_node_status and fdt_status_disabled to reference the return
codes of fdt_setprop_string itself and check for anyone else that hasn't
been considering this possible failure case.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230704/4388eca2/attachment.sig>