[PATCH v2] spl: fit: Report fdt error for loading u-boot

[Mark Kettenis]

> From: Simon Glass <sjg at chromium.org>
> Date: Wed, 19 Oct 2022 07:18:10 -0600
> 
> Hi,
> 
> On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng <baocheng.su at siemens.com> wrote:
> >
> > Hi Simon,
> >
> 
> +Tom Rini for guidance
> 
> > On Sat, 2022-07-30 at 19:27 -0600, Simon Glass wrote:
> > > Hi Bao Cheng,
> > >
> > > On Sat, 30 Jul 2022 at 03:05, Su, Bao Cheng <baocheng.su at siemens.com> wrote:
> > > >
> > > > Commit 71551055cbdb ("spl: fit: Load devicetree when a Linux payload is
> > > > found") made a change to not report the spl_fit_append_fdt error at all
> > > > if next-stage image is u-boot.
> > > >
> > > > However for u-boot image without CONFIG_OF_EMBED, the error should be
> > > > reported to uplevel caller. Otherwise, uplevel caller would think the
> > > > fdt is already loaded which is obviously not true.
> > > >
> > > > Signed-off-by: Baocheng Su <baocheng.su at siemens.com>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Fix the wrong wrapping
> > > >
> > > >  common/spl/spl_fit.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > > > index a35be52965..00404935cb 100644
> > > > --- a/common/spl/spl_fit.c
> > > > +++ b/common/spl/spl_fit.c
> > > > @@ -770,8 +770,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
> > > >          */
> > > >         if (os_takes_devicetree(spl_image->os)) {
> > > >                 ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> > > > -               if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
> > > > -                       return ret;
> > > > +               if (ret < 0) {
> > > > +                       if (spl_image->os != IH_OS_U_BOOT)
> > > > +                               return ret;
> > > > +                       else if (!IS_ENABLED(CONFIG_OF_EMBED))
> > > > +                               return ret;
> > >
> > > This is a pretty unpleasant condition. I think we would be better to
> > > report the error and let the caller figure it out.
> > >
> > > There are no tests associated with this, so it is hard to know what is
> > > actually going on.
> > >
> > > If we must have this workaround, I suggest adding a Kconfig so boards
> > > that need it can turn it on, and other boards can use normal
> > > operation, which is to report errors.
> > >
> >
> > Since there is no particular error code stands for such kind of
> > scenario, it would be hard for the caller to determine which step has
> > the problem.
> >
> > Or below code is more clear?
> >
> >         if (os_takes_devicetree(spl_image->os)) {
> >                 ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> > -               if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
> > -                       return ret;
> > +               if (ret < 0
> > +                    && (spl_image->os != IH_OS_U_BOOT
> > +                      || !IS_ENABLED(CONFIG_OF_EMBED)))
> > +                               return ret;
> >         }
> >
> > Actually there is already the `CONFIG_OF_EMBED` to tell them apart, see
> > the previous logic before commit 71551055cbdb:
> >
> >          * Booting a next-stage U-Boot may require us to append the FDT.
> >          * We allow this to fail, as the U-Boot image might embed its
> > FDT.
> >          */
> > -       if (spl_image->os == IH_OS_U_BOOT) {
> > +       if (os_takes_devicetree(spl_image->os)) {
> >                 ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> > -               if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
> > +               if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
> >                         return ret;
> >         }
> >
> > So before the commit 71551055cbdb, the normal case would be to report
> > the error, but the commit in question changed this to not report the
> > error for normal spl to boot u-boot, only reports error for SPL to boot
> > kernel, i.e. falcon mode.
> 
> We don't (or shouldn't) have boards which use OF_EMBED in mainline, so
> that condition doesn't seem to make sense to me.

We have plenty of boards that set OF_EMBED, and as some of us have
pointed out to you more than once before, there are several valid use
cases for it.