[PATCH] common: board_f: Fix crash in print_cpuinfo

[PATCH] common: board_f: Fix crash in print_cpuinfo

[Christian Kohlschütter]

With CONFIG_DISPLAY_CPUINFO=y and CONFIG_CPU=y, the initcall sequence
may fail (and therefore hang the boot process) with an -ENODEV (err=-19)
error code.

This is caused by either cpu_get_current_dev/cpu_get_desc failing to
return CPU information.

If no CPU information can be obtained, fall-back to the non-Driver Model
implementation of print_cpuinfo.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
 common/board_f.c | 14 +++++++++-----
 include/init.h   |  3 +--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 18e2246733..0656845e24 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -182,9 +182,12 @@ static int print_resetinfo(void)
 }
 #endif
 
-#if defined(CONFIG_DISPLAY_CPUINFO) && CONFIG_IS_ENABLED(CPU)
-static int print_cpuinfo(void)
+#if defined(CONFIG_DISPLAY_CPUINFO)
+static int print_cpuinfo_0(void)
 {
+#if !CONFIG_IS_ENABLED(CPU)
+	return print_cpuinfo();
+#else
 	struct udevice *dev;
 	char desc[512];
 	int ret;
@@ -193,19 +196,20 @@ static int print_cpuinfo(void)
 	if (!dev) {
 		debug("%s: Could not get CPU device\n",
 		      __func__);
-		return -ENODEV;
+		return print_cpuinfo(); // fallback
 	}
 
 	ret = cpu_get_desc(dev, desc, sizeof(desc));
 	if (ret) {
 		debug("%s: Could not get CPU description (err = %d)\n",
 		      dev->name, ret);
-		return ret;
+		return print_cpuinfo(); // fallback
 	}
 
 	printf("CPU:   %s\n", desc);
 
 	return 0;
+#endif
 }
 #endif
 
@@ -866,7 +870,7 @@ static const init_fnc_t init_sequence_f[] = {
 	print_resetinfo,
 #endif
 #if defined(CONFIG_DISPLAY_CPUINFO)
-	print_cpuinfo,		/* display cpu info (and speed) */
+	print_cpuinfo_0,		/* display cpu info (and speed) */
 #endif
 #if defined(CONFIG_DTB_RESELECT)
 	embedded_dtb_select,
diff --git a/include/init.h b/include/init.h
index 7b8f62c121..9837c22b75 100644
--- a/include/init.h
+++ b/include/init.h
@@ -210,14 +210,13 @@ int pci_init(void);
  */
 int init_cache_f_r(void);
 
-#if !CONFIG_IS_ENABLED(CPU)
 /**
  * print_cpuinfo() - Display information about the CPU
  *
  * Return: 0 if OK, -ve on error
  */
 int print_cpuinfo(void);
-#endif
+
 int timer_init(void);
 
 #if defined(CONFIG_DTB_RESELECT)
-- 
2.36.2

Re: [PATCH] common: board_f: Fix crash in print_cpuinfo

[Simon Glass]

Hi Christian,

On Wed, 28 Sept 2022 at 18:20, Christian Kohlschütter
<christian@kohlschutter.com> wrote:
>
> With CONFIG_DISPLAY_CPUINFO=y and CONFIG_CPU=y, the initcall sequence
> may fail (and therefore hang the boot process) with an -ENODEV (err=-19)
> error code.
>
> This is caused by either cpu_get_current_dev/cpu_get_desc failing to
> return CPU information.
>
> If no CPU information can be obtained, fall-back to the non-Driver Model
> implementation of print_cpuinfo.
>
> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
> ---
>  common/board_f.c | 14 +++++++++-----
>  include/init.h   |  3 +--
>  2 files changed, 10 insertions(+), 7 deletions(-)

No, we don't want to do this. If you have CPU enabled then the device
must return the info. The non-DM code will go away one day. It is not
intended as a fallback.

Regards,
Simon

Re: [PATCH] common: board_f: Fix crash in print_cpuinfo

[Christian Kohlschütter]

> On 29. Sep 2022, at 04:36, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Christian,
> 
> On Wed, 28 Sept 2022 at 18:20, Christian Kohlschütter
> <christian@kohlschutter.com> wrote:
>> 
>> With CONFIG_DISPLAY_CPUINFO=y and CONFIG_CPU=y, the initcall sequence
>> may fail (and therefore hang the boot process) with an -ENODEV (err=-19)
>> error code.
>> 
>> This is caused by either cpu_get_current_dev/cpu_get_desc failing to
>> return CPU information.
>> 
>> If no CPU information can be obtained, fall-back to the non-Driver Model
>> implementation of print_cpuinfo.
>> 
>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>> ---
>> common/board_f.c | 14 +++++++++-----
>> include/init.h   |  3 +--
>> 2 files changed, 10 insertions(+), 7 deletions(-)
> 
> No, we don't want to do this. If you have CPU enabled then the device
> must return the info. The non-DM code will go away one day. It is not
> intended as a fallback.
> 
> Regards,
> Simon

Thanks for the clarification, Simon. That's what I thought (it's not really documented with CONFIG_CPU but I get the idea).

It looks like the new CONFIG_CPU feature isn't really supported for ARM boards, and the existing print_cpuinfo shows additional information that may not be captured with the new setup, such as "reset cause", etc. What are the plans/timeline for implementing the new feature?

I'm specifically asking because the new feature may help improve the coverage of smbios information available downstream.

Re: [PATCH] common: board_f: Fix crash in print_cpuinfo

[Simon Glass]

Hi Christian,

On Thu, 29 Sept 2022 at 08:24, Christian Kohlschütter
<christian@kohlschutter.com> wrote:
>
> > On 29. Sep 2022, at 04:36, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Christian,
> >
> > On Wed, 28 Sept 2022 at 18:20, Christian Kohlschütter
> > <christian@kohlschutter.com> wrote:
> >>
> >> With CONFIG_DISPLAY_CPUINFO=y and CONFIG_CPU=y, the initcall sequence
> >> may fail (and therefore hang the boot process) with an -ENODEV (err=-19)
> >> error code.
> >>
> >> This is caused by either cpu_get_current_dev/cpu_get_desc failing to
> >> return CPU information.
> >>
> >> If no CPU information can be obtained, fall-back to the non-Driver Model
> >> implementation of print_cpuinfo.
> >>
> >> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
> >> ---
> >> common/board_f.c | 14 +++++++++-----
> >> include/init.h   |  3 +--
> >> 2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > No, we don't want to do this. If you have CPU enabled then the device
> > must return the info. The non-DM code will go away one day. It is not
> > intended as a fallback.
> >
> > Regards,
> > Simon
>
> Thanks for the clarification, Simon. That's what I thought (it's not really documented with CONFIG_CPU but I get the idea).
>
> It looks like the new CONFIG_CPU feature isn't really supported for ARM boards, and the existing print_cpuinfo shows additional information that may not be captured with the new setup, such as "reset cause", etc. What are the plans/timeline for implementing the new feature?

It really depends on when people send patches for it. You could send
something to help here.

>
> I'm specifically asking because the new feature may help improve the coverage of smbios information available downstream.
>

OK

Regards,
Simon