LoongArch: Added detection of return values of some steps in the system init process

[PATCH] LoongArch: Added detection of return values of some steps in the system init process

Posted by cuitao 3 weeks, 2 days ago

Check the return value after early_memremap_ro and kobject_create_and_add,
and check whether the memory allocation was successful after kstrdup

Signed-off-by: cuitao <cuitao@kylinos.cn>
---
 arch/loongarch/kernel/env.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/kernel/env.c b/arch/loongarch/kernel/env.c
index c0a5dc9aeae2..eaa9e054209c 100644
--- a/arch/loongarch/kernel/env.c
+++ b/arch/loongarch/kernel/env.c
@@ -23,7 +23,13 @@ EXPORT_SYMBOL(loongson_sysconf);
 void __init init_environ(void)
 {
 	int efi_boot = fw_arg0;
-	char *cmdline = early_memremap_ro(fw_arg1, COMMAND_LINE_SIZE);
+	char *cmdline;
+
+	cmdline = early_memremap_ro(fw_arg1, COMMAND_LINE_SIZE);
+	if (!cmdline) {
+		pr_err("Failed to map command line memory\n");
+		return;
+	}
 
 	if (efi_boot)
 		set_bit(EFI_BOOT, &efi.flags);
@@ -46,10 +52,18 @@ static int __init init_cpu_fullname(void)
 
 	/* Parsing cpuname from DTS model property */
 	root = of_find_node_by_path("/");
+	if (!root) {
+		pr_warn("Failed to find root device node\n");
+		return -ENODEV;
+	}
 	ret = of_property_read_string(root, "model", &model);
 	if (ret == 0) {
 		cpuname = kstrdup(model, GFP_KERNEL);
-		loongson_sysconf.cpuname = strsep(&cpuname, " ");
+		if (cpuname) {
+			loongson_sysconf.cpuname = strsep(&cpuname, " ");
+		} else {
+			pr_warn("Failed to allocate memory for cpuname\n");
+		}
 	}
 	of_node_put(root);
 
@@ -67,14 +81,18 @@ static int __init fdt_cpu_clk_init(void)
 	struct device_node *np;
 
 	np = of_get_cpu_node(0, NULL);
-	if (!np)
+	if (!np) {
+		pr_warn("Failed to get CPU node\n");
 		return -ENODEV;
+	}
 
 	clk = of_clk_get(np, 0);
 	of_node_put(np);
 
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
+		pr_warn("Failed to get CPU clock\n");
 		return -ENODEV;
+	}
 
 	cpu_clock_freq = clk_get_rate(clk);
 	clk_put(clk);
@@ -109,6 +127,10 @@ static int __init boardinfo_init(void)
 	struct kobject *loongson_kobj;
 
 	loongson_kobj = kobject_create_and_add("loongson", firmware_kobj);
+	if (!loongson_kobj) {
+		pr_warn("loongson: Firmware registration failed.\n");
+		return -ENOMEM;
+	}
 
 	return sysfs_create_file(loongson_kobj, &boardinfo_attr.attr);
 }
-- 
2.33.0

Re: [PATCH] LoongArch: Added detection of return values of some steps in the system init process

Posted by Huacai Chen 3 weeks, 2 days ago

Hi, Cuitao,

Don't over-design!

On Tue, Sep 9, 2025 at 10:35 AM cuitao <cuitao@kylinos.cn> wrote:
>
> Check the return value after early_memremap_ro and kobject_create_and_add,
> and check whether the memory allocation was successful after kstrdup
>
> Signed-off-by: cuitao <cuitao@kylinos.cn>
> ---
>  arch/loongarch/kernel/env.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/loongarch/kernel/env.c b/arch/loongarch/kernel/env.c
> index c0a5dc9aeae2..eaa9e054209c 100644
> --- a/arch/loongarch/kernel/env.c
> +++ b/arch/loongarch/kernel/env.c
> @@ -23,7 +23,13 @@ EXPORT_SYMBOL(loongson_sysconf);
>  void __init init_environ(void)
>  {
>         int efi_boot = fw_arg0;
> -       char *cmdline = early_memremap_ro(fw_arg1, COMMAND_LINE_SIZE);
> +       char *cmdline;
> +
> +       cmdline = early_memremap_ro(fw_arg1, COMMAND_LINE_SIZE);
 early_memremap_ro() is "return ((void __iomem *)TO_CACHE(phys_addr))"
on LoongArch, there is no chance to return NULL.

> +       if (!cmdline) {
> +               pr_err("Failed to map command line memory\n");
> +               return;
> +       }
>
>         if (efi_boot)
>                 set_bit(EFI_BOOT, &efi.flags);
> @@ -46,10 +52,18 @@ static int __init init_cpu_fullname(void)
>
>         /* Parsing cpuname from DTS model property */
>         root = of_find_node_by_path("/");
After 7b937cc243e5b1df8780a0aa743ce800df6c6 ("of: Create of_root if no
dtb provided by firmware"), there is no chance to have no root.

> +       if (!root) {
> +               pr_warn("Failed to find root device node\n");
> +               return -ENODEV;
> +       }
>         ret = of_property_read_string(root, "model", &model);
>         if (ret == 0) {
>                 cpuname = kstrdup(model, GFP_KERNEL);
> -               loongson_sysconf.cpuname = strsep(&cpuname, " ");
> +               if (cpuname) {
> +                       loongson_sysconf.cpuname = strsep(&cpuname, " ");
> +               } else {
> +                       pr_warn("Failed to allocate memory for cpuname\n");
Have a cpuname in the model string is not mandatory, print a warning for what?

> +               }
>         }
>         of_node_put(root);
>
> @@ -67,14 +81,18 @@ static int __init fdt_cpu_clk_init(void)
>         struct device_node *np;
>
>         np = of_get_cpu_node(0, NULL);
> -       if (!np)
> +       if (!np) {
> +               pr_warn("Failed to get CPU node\n");
The same, this is not mandatory.

>                 return -ENODEV;
> +       }
>
>         clk = of_clk_get(np, 0);
>         of_node_put(np);
>
> -       if (IS_ERR(clk))
> +       if (IS_ERR(clk)) {
> +               pr_warn("Failed to get CPU clock\n");
The same, this is not mandatory.


Huacai

>                 return -ENODEV;
> +       }
>
>         cpu_clock_freq = clk_get_rate(clk);
>         clk_put(clk);
> @@ -109,6 +127,10 @@ static int __init boardinfo_init(void)
>         struct kobject *loongson_kobj;
>
>         loongson_kobj = kobject_create_and_add("loongson", firmware_kobj);
> +       if (!loongson_kobj) {
> +               pr_warn("loongson: Firmware registration failed.\n");
> +               return -ENOMEM;
> +       }
>
>         return sysfs_create_file(loongson_kobj, &boardinfo_attr.attr);
>  }
> --
> 2.33.0
>

Re: [PATCH] LoongArch: Added detection of return values of some steps in the system init process

Posted by WANG Xuerui 3 weeks, 2 days ago

Hi,

On 9/9/25 11:13, Huacai Chen wrote:
> Hi, Cuitao,
> 
> Don't over-design!

I generally agree with this, but...

> On Tue, Sep 9, 2025 at 10:35 AM cuitao <cuitao@kylinos.cn> wrote:
>>
>> Check the return value after early_memremap_ro and kobject_create_and_add,
>> and check whether the memory allocation was successful after kstrdup
>>
>> Signed-off-by: cuitao <cuitao@kylinos.cn>
>> ---
>>   arch/loongarch/kernel/env.c | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/env.c b/arch/loongarch/kernel/env.c
>> index c0a5dc9aeae2..eaa9e054209c 100644
>> --- a/arch/loongarch/kernel/env.c
>> +++ b/arch/loongarch/kernel/env.c
>> @@ -23,7 +23,13 @@ EXPORT_SYMBOL(loongson_sysconf);
>>   void __init init_environ(void)
>>   {
>>          int efi_boot = fw_arg0;
>> -       char *cmdline = early_memremap_ro(fw_arg1, COMMAND_LINE_SIZE);
>> +       char *cmdline;
>> +
>> +       cmdline = early_memremap_ro(fw_arg1, COMMAND_LINE_SIZE);
>   early_memremap_ro() is "return ((void __iomem *)TO_CACHE(phys_addr))"
> on LoongArch, there is no chance to return NULL.
> 
>> +       if (!cmdline) {
>> +               pr_err("Failed to map command line memory\n");
>> +               return;
>> +       }
>>
>>          if (efi_boot)
>>                  set_bit(EFI_BOOT, &efi.flags);
>> @@ -46,10 +52,18 @@ static int __init init_cpu_fullname(void)
>>
>>          /* Parsing cpuname from DTS model property */
>>          root = of_find_node_by_path("/");
> After 7b937cc243e5b1df8780a0aa743ce800df6c6 ("of: Create of_root if no
> dtb provided by firmware"), there is no chance to have no root.

... These are indeed valid reasons, but only known by people already 
(deeply) familiar with the codebase and the port.

>> +       if (!root) {
>> +               pr_warn("Failed to find root device node\n");
>> +               return -ENODEV;
>> +       }
>>          ret = of_property_read_string(root, "model", &model);
>>          if (ret == 0) {
>>                  cpuname = kstrdup(model, GFP_KERNEL);
>> -               loongson_sysconf.cpuname = strsep(&cpuname, " ");
>> +               if (cpuname) {
>> +                       loongson_sysconf.cpuname = strsep(&cpuname, " ");
>> +               } else {
>> +                       pr_warn("Failed to allocate memory for cpuname\n");
> Have a cpuname in the model string is not mandatory, print a warning for what?

So is this piece of know-how. Therefore, I'd suggest keeping the code 
intact but adding comments regarding the safety -- why omitted checks 
here and there are safe in the port's scope.

>> +               }
>>          }
>>          of_node_put(root);
>>
>> @@ -67,14 +81,18 @@ static int __init fdt_cpu_clk_init(void)
>>          struct device_node *np;
>>
>>          np = of_get_cpu_node(0, NULL);
>> -       if (!np)
>> +       if (!np) {
>> +               pr_warn("Failed to get CPU node\n");
> The same, this is not mandatory.
> 
>>                  return -ENODEV;
>> +       }
>>
>>          clk = of_clk_get(np, 0);
>>          of_node_put(np);
>>
>> -       if (IS_ERR(clk))
>> +       if (IS_ERR(clk)) {
>> +               pr_warn("Failed to get CPU clock\n");
> The same, this is not mandatory.
> 
> 
> Huacai
> 
>>                  return -ENODEV;
>> +       }
>>
>>          cpu_clock_freq = clk_get_rate(clk);
>>          clk_put(clk);
>> @@ -109,6 +127,10 @@ static int __init boardinfo_init(void)
>>          struct kobject *loongson_kobj;
>>
>>          loongson_kobj = kobject_create_and_add("loongson", firmware_kobj);
>> +       if (!loongson_kobj) {
>> +               pr_warn("loongson: Firmware registration failed.\n");
>> +               return -ENOMEM;
>> +       }
>>
>>          return sysfs_create_file(loongson_kobj, &boardinfo_attr.attr);
>>   }
>> --
>> 2.33.0
>>

I'd agree that the remaining debug prints are less useful than the 
commentary I suggested before, and should be better left out.

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

Re: [PATCH] LoongArch: Added detection of return values of some steps in the system init process

Posted by cuitao 3 weeks, 2 days ago

Thank you for your review. 
Sorry for any inconvenience caused to you, and I will study this part of the process carefully. 
By referring to the code of other architectures, 
I think the check for kobj creation in boardinfo_init can be retained.