[U-Boot] [PATCH 2/2] fdt_support: Don't panic if unable to perform stdout fixup

[Simon Glass]

Hi Scott,

On 1 September 2015 at 21:00, Scott Wood <scottwood at freescale.com> wrote:
> On Tue, 2015-09-01 at 20:48 -0600, Simon Glass wrote:
>> Hi Scott,
>>
>> On 31 August 2015 at 21:16, Scott Wood <scottwood at freescale.com> wrote:
>> > On Mon, 2015-08-31 at 21:13 -0600, Simon Glass wrote:
>> > > Hi Scott,
>> > >
>> > > On 31 August 2015 at 20:11, Scott Wood <scottwood at freescale.com> wrote:
>> > > > Currently, using fdt_fixup_stdout() on a device tree that is missing
>> > > > the relevant alias results in this:
>> > > >
>> > > > WARNING: could not set linux,stdout-path FDT_ERR_NOTFOUND.
>> > > > ERROR: /chosen node create failed
>> > > >  - must RESET the board to recover.
>> > > >
>> > > > FDT creation failed! hanging...### ERROR ### Please RESET the board
>> > > > ###
>> > > >
>> > > > There is no reason for this to be a fatal error rather than a warning,
>> > > > and removing this allows for a smooth transition on a platform where
>> > > > the device tree currently lacks the correct aliases but will have them
>> > > > in the future.
>> > >
>> > > Why do we need this patch - what platform?
>> >
>> > LS2085A
>> >
>> > >
>> > > >
>> > > > Signed-off-by: Scott Wood <scottwood at freescale.com>
>> > > > Cc: Kumar Gala <galak at kernel.crashing.org>
>> > > > Cc: Simon Glass <sjg at chromium.org>
>> > > > ---
>> > > > Resent with correct address for Simon Glass.
>> > > >
>> > > >  common/fdt_support.c | 3 ++-
>> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/common/fdt_support.c b/common/fdt_support.c
>> > > > index f86365e..6052c77 100644
>> > > > --- a/common/fdt_support.c
>> > > > +++ b/common/fdt_support.c
>> > > > @@ -308,7 +308,8 @@ int fdt_chosen(void *fdt)
>> > > >                 }
>> > > >         }
>> > > >
>> > > > -       return fdt_fixup_stdout(fdt, nodeoffset);
>> > > > +       fdt_fixup_stdout(fdt, nodeoffset);
>> > >
>> > > Will some platforms will not boot correctly with this failing? Should
>> > > we make your new feature a Kconfig options perhaps? I worry that it
>> > > will become the default behaviour and then it will be hard to remove
>> > > later.
>> >
>> > A warning will still be printed.  I'm not sure how "### ERROR ### Please
>> > RESET the board ###" is more useful than trying to continue and possibly
>> > failing.
>>
>> Only that if it indicates a fatal error the board code can at least
>> find out about it and deal with it. Perhaps booting will just result
>> in a hang?
>
> If booting results in a hang, then you're no worse off than the current
>
> situation.  Either way, the user sees a message that indicates the problem.
>
>> I think ignoring errors is fine but here we make it impossible to
>> detect a failure. So I think that a Kconfig is the best idea, so we
>> can remove it later.
>
> It seems excessive...  Config options aren't free, from a maintenance
>
> perspective, and I have a hard time imagining a scenario where proceeding to
>
> boot causes a real problem.
>
> Also, from a consistency perspective:
>  - The OF_STDOUT_PATH version doesn't cause a panic if the destination path
> doesn't exist.
>  - None of the callers of fdt_status_<foo>_by_alias() panic if the alias is
> not found.
>  - do_fixup_by_path() doesn't panic if the node is not found.
>  - Neither does do_fixup_by_compatible().
>  - Neither does fdt_fixup_ethernet().

Yes this code needs cleaning up.

The particular problem you have is that there are no aliases, right?
How about returning 0 from fdt_fixup_stdout() in that case? Then you
will not affect the behaviour of other boards which perhaps do have
/aliases but do not have space in their tree for the fdt_setprop().

Regards,
Simon