| Message ID | 1441073484-15640-1-git-send-email-scottwood@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Scott, On 31 August 2015 at 20:11, Scott Wood <scottwood@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? > > Signed-off-by: Scott Wood <scottwood@freescale.com> > Cc: Kumar Gala <galak@kernel.crashing.org> > Cc: Simon Glass <sjg@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. > + return 0; > } > > void do_fixup_by_path(void *fdt, const char *path, const char *prop, > -- > 2.1.4 > Regards, Simon
On Mon, 2015-08-31 at 21:13 -0600, Simon Glass wrote: > Hi Scott, > > On 31 August 2015 at 20:11, Scott Wood <scottwood@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@freescale.com> > > Cc: Kumar Gala <galak@kernel.crashing.org> > > Cc: Simon Glass <sjg@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. -Scott
Hi Scott, On 31 August 2015 at 21:16, Scott Wood <scottwood@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@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@freescale.com> >> > Cc: Kumar Gala <galak@kernel.crashing.org> >> > Cc: Simon Glass <sjg@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? 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. Regards, Simon
On Tue, 2015-09-01 at 20:48 -0600, Simon Glass wrote: > Hi Scott, > > On 31 August 2015 at 21:16, Scott Wood <scottwood@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@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@freescale.com> > > > > Cc: Kumar Gala <galak@kernel.crashing.org> > > > > Cc: Simon Glass <sjg@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(). -Scott
On 09/01/2015 09:48 PM, Simon Glass wrote: > Hi Scott, > > On 31 August 2015 at 21:16, Scott Wood <scottwood@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@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@freescale.com> >>>> Cc: Kumar Gala <galak@kernel.crashing.org> >>>> Cc: Simon Glass <sjg@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? > > 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. How about a big warning instead? In general, having the message to reset the board doesn't help much if it is a fatal condition. We have to use external tool to recover the board. On the other side, if the error is not fatal, continue to boot may give the user a chance to reflash an update. York
On Tue, 2015-09-01 at 22:04 -0500, York Sun wrote: > On 09/01/2015 09:48 PM, Simon Glass wrote: > > Hi Scott, > > > > On 31 August 2015 at 21:16, Scott Wood <scottwood@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@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@freescale.com> > > > > > Cc: Kumar Gala <galak@kernel.crashing.org> > > > > > Cc: Simon Glass <sjg@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? > > > > 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. > > How about a big warning instead? There is still a warning with this patch applied. > In general, having the message to reset the > board doesn't help much if it is a fatal condition. We have to use external > tool > to recover the board. > On the other side, if the error is not fatal, continue to > boot may give the user a chance to reflash an update. That doesn't really apply to errors that happen when loading a kernel... -Scott
Hi Scott, On 1 September 2015 at 21:00, Scott Wood <scottwood@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@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@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@freescale.com> >> > > > Cc: Kumar Gala <galak@kernel.crashing.org> >> > > > Cc: Simon Glass <sjg@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
On Tue, 2015-09-01 at 21:10 -0600, Simon Glass wrote: > Hi Scott, > > On 1 September 2015 at 21:00, Scott Wood <scottwood@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@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@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@freescale.com> > > > > > > Cc: Kumar Gala <galak@kernel.crashing.org> > > > > > > Cc: Simon Glass <sjg@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. I wasn't citing them as problems. :-P > 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(). That would be fine. -Scott
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); + return 0; } void do_fixup_by_path(void *fdt, const char *path, const char *prop,
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. Signed-off-by: Scott Wood <scottwood@freescale.com> Cc: Kumar Gala <galak@kernel.crashing.org> Cc: Simon Glass <sjg@chromium.org> --- Resent with correct address for Simon Glass. common/fdt_support.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)