| Message ID | 20180809190420.28093-2-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Get socfpga gen5 SPL working again. | expand |
On 08/09/2018 09:04 PM, Simon Goldschmidt wrote: > There were NULL pointers dereferenced because DM was used > too early without correct initialization: > - malloc_simple returned NULL when called from preloader_console_init() > because gd->malloc_limit was 0 > - uclass_add dereferenced gd->uclass_root members which were NULL because > dm_init (or one of its relatives) has not been called. > > All this is fixed by calling spl_early_init before calling > preloader_console_init. > > This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial") > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > v2: > - Don't remove gd->malloc_base assignment at the end of board_init_f() > (moved to an extra patch) > > arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c > index d6fe7d35af..9bdfaa3c1e 100644 > --- a/arch/arm/mach-socfpga/spl_gen5.c > +++ b/arch/arm/mach-socfpga/spl_gen5.c > @@ -86,6 +86,7 @@ void board_init_f(ulong dummy) > const struct cm_config *cm_default_cfg = cm_get_default_config(); > unsigned long sdram_size; > unsigned long reg; > + int ret; > > /* > * First C code to run. Clear fake OCRAM ECC first as SBE > @@ -152,6 +153,12 @@ void board_init_f(ulong dummy) > /* unfreeze / thaw all IO banks */ > sys_mgr_frzctrl_thaw_req(); > > + ret = spl_early_init(); Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA SPL is a bit weird. > + if (ret) { > + debug("spl_early_init() failed: %d\n", ret); > + hang(); > + } > + > /* enable console uart printing */ > preloader_console_init(); > >
On 09.08.2018 23:42, Marek Vasut wrote: > On 08/09/2018 09:04 PM, Simon Goldschmidt wrote: >> There were NULL pointers dereferenced because DM was used >> too early without correct initialization: >> - malloc_simple returned NULL when called from preloader_console_init() >> because gd->malloc_limit was 0 >> - uclass_add dereferenced gd->uclass_root members which were NULL because >> dm_init (or one of its relatives) has not been called. >> >> All this is fixed by calling spl_early_init before calling >> preloader_console_init. >> >> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial") >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> --- >> v2: >> - Don't remove gd->malloc_base assignment at the end of board_init_f() >> (moved to an extra patch) >> >> arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c >> index d6fe7d35af..9bdfaa3c1e 100644 >> --- a/arch/arm/mach-socfpga/spl_gen5.c >> +++ b/arch/arm/mach-socfpga/spl_gen5.c >> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy) >> const struct cm_config *cm_default_cfg = cm_get_default_config(); >> unsigned long sdram_size; >> unsigned long reg; >> + int ret; >> >> /* >> * First C code to run. Clear fake OCRAM ECC first as SBE >> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy) >> /* unfreeze / thaw all IO banks */ >> sys_mgr_frzctrl_thaw_req(); >> >> + ret = spl_early_init(); > Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA > SPL is a bit weird. Ehrm, I copied this from spl_s10.c, but other boards seem to do this, too. Honestly, I don't know how any SPL can use DM serial without this being called. Maybe other SPLs initialize the serial port later (not in board_init_f). common/spl/spl.c calls spl_init(), which also calls the part that spl_early_init() calls. I can only take other SPLs as reference and from reading all the code, I think this should be good. Simon > >> + if (ret) { >> + debug("spl_early_init() failed: %d\n", ret); >> + hang(); >> + } >> + >> /* enable console uart printing */ >> preloader_console_init(); >> >> >
On 08/10/2018 02:39 PM, Simon Goldschmidt wrote: > On 09.08.2018 23:42, Marek Vasut wrote: >> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote: >>> There were NULL pointers dereferenced because DM was used >>> too early without correct initialization: >>> - malloc_simple returned NULL when called from preloader_console_init() >>> because gd->malloc_limit was 0 >>> - uclass_add dereferenced gd->uclass_root members which were NULL >>> because >>> dm_init (or one of its relatives) has not been called. >>> >>> All this is fixed by calling spl_early_init before calling >>> preloader_console_init. >>> >>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial") >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>> --- >>> v2: >>> - Don't remove gd->malloc_base assignment at the end of board_init_f() >>> (moved to an extra patch) >>> >>> arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c >>> b/arch/arm/mach-socfpga/spl_gen5.c >>> index d6fe7d35af..9bdfaa3c1e 100644 >>> --- a/arch/arm/mach-socfpga/spl_gen5.c >>> +++ b/arch/arm/mach-socfpga/spl_gen5.c >>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy) >>> const struct cm_config *cm_default_cfg = cm_get_default_config(); >>> unsigned long sdram_size; >>> unsigned long reg; >>> + int ret; >>> /* >>> * First C code to run. Clear fake OCRAM ECC first as SBE >>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy) >>> /* unfreeze / thaw all IO banks */ >>> sys_mgr_frzctrl_thaw_req(); >>> + ret = spl_early_init(); >> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA >> SPL is a bit weird. > > Ehrm, I copied this from spl_s10.c, but other boards seem to do this, > too. Honestly, I don't know how any SPL can use DM serial without this > being called. Maybe other SPLs initialize the serial port later (not in > board_init_f). I mean, spl_early_init() is called in common/spl/spl.c , which is common code. Maybe the socfpga SPL is structured in a really weird way (I think it is). > common/spl/spl.c calls spl_init(), which also calls the part that > spl_early_init() calls. > > I can only take other SPLs as reference and from reading all the code, I > think this should be good. Right > Simon > >> >>> + if (ret) { >>> + debug("spl_early_init() failed: %d\n", ret); >>> + hang(); >>> + } >>> + >>> /* enable console uart printing */ >>> preloader_console_init(); >>> >> >
On 10.08.2018 14:41, Marek Vasut wrote: > On 08/10/2018 02:39 PM, Simon Goldschmidt wrote: >> On 09.08.2018 23:42, Marek Vasut wrote: >>> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote: >>>> There were NULL pointers dereferenced because DM was used >>>> too early without correct initialization: >>>> - malloc_simple returned NULL when called from preloader_console_init() >>>> because gd->malloc_limit was 0 >>>> - uclass_add dereferenced gd->uclass_root members which were NULL >>>> because >>>> dm_init (or one of its relatives) has not been called. >>>> >>>> All this is fixed by calling spl_early_init before calling >>>> preloader_console_init. >>>> >>>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial") >>>> >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>> --- >>>> v2: >>>> - Don't remove gd->malloc_base assignment at the end of board_init_f() >>>> (moved to an extra patch) >>>> >>>> arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c >>>> b/arch/arm/mach-socfpga/spl_gen5.c >>>> index d6fe7d35af..9bdfaa3c1e 100644 >>>> --- a/arch/arm/mach-socfpga/spl_gen5.c >>>> +++ b/arch/arm/mach-socfpga/spl_gen5.c >>>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy) >>>> const struct cm_config *cm_default_cfg = cm_get_default_config(); >>>> unsigned long sdram_size; >>>> unsigned long reg; >>>> + int ret; >>>> /* >>>> * First C code to run. Clear fake OCRAM ECC first as SBE >>>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy) >>>> /* unfreeze / thaw all IO banks */ >>>> sys_mgr_frzctrl_thaw_req(); >>>> + ret = spl_early_init(); >>> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA >>> SPL is a bit weird. >> Ehrm, I copied this from spl_s10.c, but other boards seem to do this, >> too. Honestly, I don't know how any SPL can use DM serial without this >> being called. Maybe other SPLs initialize the serial port later (not in >> board_init_f). > I mean, spl_early_init() is called in common/spl/spl.c , which is common > code. Maybe the socfpga SPL is structured in a really weird way (I think > it is). Not exactly: common/spl/spl.c calls spl_common_init(), just like spl_early_init() does. Given the names, I think spl_early_init() is meant to be called early, e.g. from board_init_f() ;-) Oh, I just saw spl_common_init() emits a debug print "spl_early_init()", so that might have tricked you...? >> common/spl/spl.c calls spl_init(), which also calls the part that >> spl_early_init() calls. >> >> I can only take other SPLs as reference and from reading all the code, I >> think this should be good. > Right So is this change OK for v2018.09 once I fix the dts thing? Given that v2018.07 is broken for socfpga gen5, it would be good to merge it before. I can prepare a v3 of the series with only minimal changes in the socfpga files and resend the rest as detached patches. Simon >>>> + if (ret) { >>>> + debug("spl_early_init() failed: %d\n", ret); >>>> + hang(); >>>> + } >>>> + >>>> /* enable console uart printing */ >>>> preloader_console_init(); >>>> >
On 08/10/2018 02:55 PM, Simon Goldschmidt wrote: > On 10.08.2018 14:41, Marek Vasut wrote: >> On 08/10/2018 02:39 PM, Simon Goldschmidt wrote: >>> On 09.08.2018 23:42, Marek Vasut wrote: >>>> On 08/09/2018 09:04 PM, Simon Goldschmidt wrote: >>>>> There were NULL pointers dereferenced because DM was used >>>>> too early without correct initialization: >>>>> - malloc_simple returned NULL when called from >>>>> preloader_console_init() >>>>> because gd->malloc_limit was 0 >>>>> - uclass_add dereferenced gd->uclass_root members which were NULL >>>>> because >>>>> dm_init (or one of its relatives) has not been called. >>>>> >>>>> All this is fixed by calling spl_early_init before calling >>>>> preloader_console_init. >>>>> >>>>> This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial") >>>>> >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>> --- >>>>> v2: >>>>> - Don't remove gd->malloc_base assignment at the end of board_init_f() >>>>> (moved to an extra patch) >>>>> >>>>> arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c >>>>> b/arch/arm/mach-socfpga/spl_gen5.c >>>>> index d6fe7d35af..9bdfaa3c1e 100644 >>>>> --- a/arch/arm/mach-socfpga/spl_gen5.c >>>>> +++ b/arch/arm/mach-socfpga/spl_gen5.c >>>>> @@ -86,6 +86,7 @@ void board_init_f(ulong dummy) >>>>> const struct cm_config *cm_default_cfg = >>>>> cm_get_default_config(); >>>>> unsigned long sdram_size; >>>>> unsigned long reg; >>>>> + int ret; >>>>> /* >>>>> * First C code to run. Clear fake OCRAM ECC first as SBE >>>>> @@ -152,6 +153,12 @@ void board_init_f(ulong dummy) >>>>> /* unfreeze / thaw all IO banks */ >>>>> sys_mgr_frzctrl_thaw_req(); >>>>> + ret = spl_early_init(); >>>> Uh, but isn't this called from common/spl/spl.c ? I suspect the SoCFPGA >>>> SPL is a bit weird. >>> Ehrm, I copied this from spl_s10.c, but other boards seem to do this, >>> too. Honestly, I don't know how any SPL can use DM serial without this >>> being called. Maybe other SPLs initialize the serial port later (not in >>> board_init_f). >> I mean, spl_early_init() is called in common/spl/spl.c , which is common >> code. Maybe the socfpga SPL is structured in a really weird way (I think >> it is). > > Not exactly: common/spl/spl.c calls spl_common_init(), just like > spl_early_init() does. Given the names, I think spl_early_init() is > meant to be called early, e.g. from board_init_f() ;-) > > Oh, I just saw spl_common_init() emits a debug print "spl_early_init()", > so that might have tricked you...? Ah, yes, I think so. >>> common/spl/spl.c calls spl_init(), which also calls the part that >>> spl_early_init() calls. >>> >>> I can only take other SPLs as reference and from reading all the code, I >>> think this should be good. >> Right > > So is this change OK for v2018.09 once I fix the dts thing? Given that > v2018.07 is broken for socfpga gen5, it would be good to merge it > before. I can prepare a v3 of the series with only minimal changes in > the socfpga files and resend the rest as detached patches. Looks good, yes.
diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c index d6fe7d35af..9bdfaa3c1e 100644 --- a/arch/arm/mach-socfpga/spl_gen5.c +++ b/arch/arm/mach-socfpga/spl_gen5.c @@ -86,6 +86,7 @@ void board_init_f(ulong dummy) const struct cm_config *cm_default_cfg = cm_get_default_config(); unsigned long sdram_size; unsigned long reg; + int ret; /* * First C code to run. Clear fake OCRAM ECC first as SBE @@ -152,6 +153,12 @@ void board_init_f(ulong dummy) /* unfreeze / thaw all IO banks */ sys_mgr_frzctrl_thaw_req(); + ret = spl_early_init(); + if (ret) { + debug("spl_early_init() failed: %d\n", ret); + hang(); + } + /* enable console uart printing */ preloader_console_init();
There were NULL pointers dereferenced because DM was used too early without correct initialization: - malloc_simple returned NULL when called from preloader_console_init() because gd->malloc_limit was 0 - uclass_add dereferenced gd->uclass_root members which were NULL because dm_init (or one of its relatives) has not been called. All this is fixed by calling spl_early_init before calling preloader_console_init. This fixes commit 73172753f4f3 ("ARM: socfpga: Convert to DM serial") Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- v2: - Don't remove gd->malloc_base assignment at the end of board_init_f() (moved to an extra patch) arch/arm/mach-socfpga/spl_gen5.c | 7 +++++++ 1 file changed, 7 insertions(+)