Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Hi,
On Tue, 11 Apr 2023 at 04:24, Su, Bao Cheng wrote:
>
> Dear all,
>
> Any updates or new comments on this? How should I proceed?
This expands error reporting, so I think this is better than what we have.
Reviewed-by: Simon Glass
More comments below.
>
> BRs/Baocheng Su
>
> On Thu, 2022-10-20 at 15:44 +0200, Mark Kettenis wrote:
> > > From: Simon Glass
> > > Date: Wed, 19 Oct 2022 08:15:35 -0600
> > >
> > > Hi Mark,
> > >
> > > On Wed, 19 Oct 2022 at 08:08, Mark Kettenis
> > > wrote:
> > > >
> > > > > From: Simon Glass
> > > > > Date: Wed, 19 Oct 2022 07:18:10 -0600
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng
> > > > > 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
> > > > > > > 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
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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, );
> > > > > > > > - 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, );
> > > > > > - 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, );
> > > > > > - if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
> > > > > > + if (ret < 0 && spl_image->os !=
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Dear all,
Any updates or new comments on this? How should I proceed?
BRs/Baocheng Su
On Thu, 2022-10-20 at 15:44 +0200, Mark Kettenis wrote:
> > From: Simon Glass
> > Date: Wed, 19 Oct 2022 08:15:35 -0600
> >
> > Hi Mark,
> >
> > On Wed, 19 Oct 2022 at 08:08, Mark Kettenis
> > wrote:
> > >
> > > > From: Simon Glass
> > > > Date: Wed, 19 Oct 2022 07:18:10 -0600
> > > >
> > > > Hi,
> > > >
> > > > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng
> > > > 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
> > > > > > 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
> > > > > > > ---
> > > > > > >
> > > > > > > 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, );
> > > > > > > - 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, );
> > > > > - 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, );
> > > > > - 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.
> > > >
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
> From: Simon Glass
> Date: Wed, 19 Oct 2022 08:15:35 -0600
>
> Hi Mark,
>
> On Wed, 19 Oct 2022 at 08:08, Mark Kettenis wrote:
> >
> > > From: Simon Glass
> > > Date: Wed, 19 Oct 2022 07:18:10 -0600
> > >
> > > Hi,
> > >
> > > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng
> > > 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
> > > > > 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
> > > > > > ---
> > > > > >
> > > > > > 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,
> > > > > > );
> > > > > > - 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, );
> > > > - 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, );
> > > > - 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.
>
> Can you point me to the discussion about the valid use cases?
Not easily since there were several lengthy discussions about device
trees.
Most of the use cases boil down to the following:
* There is some low-level firmware or virtualization
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Hi Mark,
On Wed, 19 Oct 2022 at 08:08, Mark Kettenis wrote:
>
> > From: Simon Glass
> > Date: Wed, 19 Oct 2022 07:18:10 -0600
> >
> > Hi,
> >
> > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng 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
> > > > 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
> > > > > ---
> > > > >
> > > > > 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,
> > > > > );
> > > > > - 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, );
> > > - 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, );
> > > - 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.
Can you point me to the discussion about the valid use cases?
Regards,
Simon
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
> From: Simon Glass
> Date: Wed, 19 Oct 2022 07:18:10 -0600
>
> Hi,
>
> On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng 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
> > > 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
> > > > ---
> > > >
> > > > 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, );
> > > > - 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, );
> > - 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, );
> > - 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.
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Hi,
On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng 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 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
> > > ---
> > >
> > > 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, );
> > > - 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, );
> - 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, );
> - 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.
Perhaps Tom can decode all of this?
Regards,
Simon
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Hi Simon,
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 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
> > ---
> >
> > 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, );
> > - 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, );
- 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, );
- 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.
> Regards,
> Simon
[PATCH v2] spl: fit: Report fdt error for loading u-boot
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
---
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, );
- 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;
+ }
}
firmware_node = node;
--
2.30.2
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Hi Bao Cheng,
On Sat, 30 Jul 2022 at 03:05, Su, Bao Cheng 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
> ---
>
> 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, );
> - 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.
Regards,
Simon
