Implements dt_processor_hartid() to get the hart ID of the given
device tree node and do some checks if CPU is available and given device
tree node has proper riscv,isa property.

As a helper function dt_get_cpuid() is introduced to deal specifically
with reg propery of a CPU device node.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - s/of_get_cpu_hwid()/dt_get_cpu_id().
 - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
 - Add empty line before last return in dt_get_cpu_hwid().
 - s/riscv_of_processor_hartid/dt_processor_cpuid().
 - Use pointer-to_const for node argument of dt_processor_cpuid().
 - Use for hart_id unsigned long type as according to the spec for RV128
   mhartid register will be 128 bit long.
 - Update commit message and subject.
 - use 'CPU' instead of 'HART'.
 - Drop thread argument of dt_get_cpu_id() (of_get_cpu_hwid) as it is
   expected to be always 0 according to RISC-V's DTS binding.
---
 xen/arch/riscv/include/asm/smp.h |  3 ++
 xen/arch/riscv/smpboot.c         | 66 ++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h
index 5e170b57b3..9d846a1338 100644
--- a/xen/arch/riscv/include/asm/smp.h
+++ b/xen/arch/riscv/include/asm/smp.h
@@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
 
 void setup_tp(unsigned int cpuid);
 
+struct dt_device_node;
+int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);
+
 #endif
 
 /*
diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c
index 0371dfa53e..0b00dd0eb2 100644
--- a/xen/arch/riscv/smpboot.c
+++ b/xen/arch/riscv/smpboot.c
@@ -1,5 +1,8 @@
 #include <xen/cpumask.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/types.h>
 #include <xen/sections.h>
 
 cpumask_t __read_mostly cpu_online_map;
@@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
     cpumask_set_cpu(0, &cpu_possible_map);
     cpumask_set_cpu(0, &cpu_online_map);
 }
+
+/**
+ * dt_get_cpuid - Get the cpuid from a CPU device node
+ *
+ * @cpun: CPU number(logical index) for which device node is required
+ *
+ * Return: The cpuid for the CPU node or ~0ULL if not found.
+ */
+static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
+{
+    const __be32 *cell;
+    int ac;
+    uint32_t len;
+
+    ac = dt_n_addr_cells(cpun);
+    cell = dt_get_property(cpun, "reg", &len);
+    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
+        return ~0ULL;
+
+    return dt_read_number(cell, ac);
+}
+
+/*
+ * Returns the cpuid of the given device tree node, or -ENODEV if the node
+ * isn't an enabled and valid RISC-V hart node.
+ */
+int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
+{
+    const char *isa;
+
+    if ( !dt_device_is_compatible(node, "riscv") )
+    {
+        printk("Found incompatible CPU\n");
+        return -ENODEV;
+    }
+
+    *cpuid = dt_get_cpuid(node);
+    if ( *cpuid == ~0UL )
+    {
+        printk("Found CPU without CPU ID\n");
+        return -ENODEV;
+    }
+
+    if ( !dt_device_is_available(node))
+    {
+        printk("CPU with cpuid=%lu is not available\n", *cpuid);
+        return -ENODEV;
+    }
+
+    if ( dt_property_read_string(node, "riscv,isa", &isa) )
+    {
+        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
+        return -ENODEV;
+    }
+
+    if ( isa[0] != 'r' || isa[1] != 'v' )
+    {
+        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
+        return -ENODEV;
+    }
+
+    return 0;
+}
-- 
2.49.0

(adding Bertrand as the one further DT maintainer, for a respective question
below)

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> Implements dt_processor_hartid()

There's no such function (anymore).

> to get the hart ID of the given
> device tree node and do some checks if CPU is available and given device
> tree node has proper riscv,isa property.
> 
> As a helper function dt_get_cpuid() is introduced to deal specifically
> with reg propery of a CPU device node.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V2:
>  - s/of_get_cpu_hwid()/dt_get_cpu_id().
>  - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
>  - Add empty line before last return in dt_get_cpu_hwid().
>  - s/riscv_of_processor_hartid/dt_processor_cpuid().
>  - Use pointer-to_const for node argument of dt_processor_cpuid().
>  - Use for hart_id unsigned long type as according to the spec for RV128
>    mhartid register will be 128 bit long.
>  - Update commit message and subject.
>  - use 'CPU' instead of 'HART'.

Was this is good move? What is returned ...

> --- a/xen/arch/riscv/include/asm/smp.h
> +++ b/xen/arch/riscv/include/asm/smp.h
> @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
>  
>  void setup_tp(unsigned int cpuid);
>  
> +struct dt_device_node;
> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);

... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm
not sure it's a hart ID either, so it may need further clarification (and I'd
expect RISC-V to have suitable terminology to tell apart the different entities).

> @@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
>      cpumask_set_cpu(0, &cpu_possible_map);
>      cpumask_set_cpu(0, &cpu_online_map);
>  }
> +
> +/**
> + * dt_get_cpuid - Get the cpuid from a CPU device node
> + *
> + * @cpun: CPU number(logical index) for which device node is required
> + *
> + * Return: The cpuid for the CPU node or ~0ULL if not found.
> + */
> +static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
> +{
> +    const __be32 *cell;
> +    int ac;

This is bogus (should be unsigned int afaict), but dictated by ...

> +    uint32_t len;
> +
> +    ac = dt_n_addr_cells(cpun);

... the return value here and ...

> +    cell = dt_get_property(cpun, "reg", &len);
> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> +        return ~0ULL;

(Nit: This doesn't match the return type of the function; same for
the function comment. Also, what if sizeof(*cell) * ac < len?)

> +    return dt_read_number(cell, ac);

... the function parameter type here. In fact, that function is raising
another question: If the "size" argument is outside of [0, 2], the value
returned is silently truncated.

More generally - are there any plans to make DT code signed-ness-correct?

> +/*
> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
> + * isn't an enabled and valid RISC-V hart node.
> + */
> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
> +{
> +    const char *isa;
> +
> +    if ( !dt_device_is_compatible(node, "riscv") )
> +    {
> +        printk("Found incompatible CPU\n");
> +        return -ENODEV;
> +    }
> +
> +    *cpuid = dt_get_cpuid(node);
> +    if ( *cpuid == ~0UL )
> +    {
> +        printk("Found CPU without CPU ID\n");
> +        return -ENODEV;
> +    }
> +
> +    if ( !dt_device_is_available(node))
> +    {
> +        printk("CPU with cpuid=%lu is not available\n", *cpuid);
> +        return -ENODEV;
> +    }
> +
> +    if ( dt_property_read_string(node, "riscv,isa", &isa) )
> +    {
> +        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
> +        return -ENODEV;
> +    }
> +
> +    if ( isa[0] != 'r' || isa[1] != 'v' )
> +    {
> +        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
> +        return -ENODEV;
> +    }
> +
> +    return 0;
> +}

I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
messages for all of the cases, but surely there are errno values better
representing the individual failure reasons?

Jan

On 5/15/25 9:56 AM, Jan Beulich wrote:
> (adding Bertrand as the one further DT maintainer, for a respective question
> below)
>
> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>> Implements dt_processor_hartid()
> There's no such function (anymore).
>
>> to get the hart ID of the given
>> device tree node and do some checks if CPU is available and given device
>> tree node has proper riscv,isa property.
>>
>> As a helper function dt_get_cpuid() is introduced to deal specifically
>> with reg propery of a CPU device node.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> ---
>> Changes in V2:
>>   - s/of_get_cpu_hwid()/dt_get_cpu_id().
>>   - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
>>   - Add empty line before last return in dt_get_cpu_hwid().
>>   - s/riscv_of_processor_hartid/dt_processor_cpuid().
>>   - Use pointer-to_const for node argument of dt_processor_cpuid().
>>   - Use for hart_id unsigned long type as according to the spec for RV128
>>     mhartid register will be 128 bit long.
>>   - Update commit message and subject.
>>   - use 'CPU' instead of 'HART'.
> Was this is good move? What is returned ...
>
>> --- a/xen/arch/riscv/include/asm/smp.h
>> +++ b/xen/arch/riscv/include/asm/smp.h
>> @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
>>   
>>   void setup_tp(unsigned int cpuid);
>>   
>> +struct dt_device_node;
>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);
> ... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm
> not sure it's a hart ID either, so it may need further clarification (and I'd
> expect RISC-V to have suitable terminology to tell apart the different entities).

 From device tree point of view (https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt)
it is just hart which is defined as a hardware execution context, which contains all the state mandated by
the RISC-V ISA: a PC and some registers.
And also based on this for DT binding:
   For example, my Intel laptop is
   described as having one socket with two cores, each of which has two hyper
   threads.  Therefore this system has four harts.
They are using just harts and in DT it will look like 4 node with a number in reg property which they
call hart. So from DT point of view hartid is pretty fine to use.

 From specification point of view (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_hardware_platform_terminology):
   A RISC-V hardware platform can contain one or more RISC-V-compatible processing cores together
   with other non-RISC-V-compatible cores, fixed-function accelerators, various physical memory
   structures, I/O devices, and an interconnect structure to allow the components to communicate.

   A component is termed a core if it contains an independent instruction fetch unit. A RISC-V-
   compatible core might support multiple RISC-V-compatible hardware threads, or harts, through
   multithreading.
and (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_software_execution_environments_and_harts):
   From the perspective of software running in a given execution environment, a hart is a resource that
   autonomously fetches and executes RISC-V instructions within that execution environment. In this
   respect, a hart behaves like a hardware thread resource even if time-multiplexed onto real hardware by
   the execution environment. Some EEIs support the creation and destruction of additional harts, for
   example, via environment calls to fork new harts.

DT's CPU node should be refer to core plus hardware thread (or threads in case of multithreading)
in reg property to be close to the spec(?) but basically in DT they IMO just describe what in the sepc
is called an independent instruction fetch unit.

I still think that it is fine just to use hart_id. If to be close more to a spec the unit_id(?)
but it is more confusing IMO with what is use in RISC-V's DT binding.

>
>> @@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
>>       cpumask_set_cpu(0, &cpu_possible_map);
>>       cpumask_set_cpu(0, &cpu_online_map);
>>   }
>> +
>> +/**
>> + * dt_get_cpuid - Get the cpuid from a CPU device node
>> + *
>> + * @cpun: CPU number(logical index) for which device node is required
>> + *
>> + * Return: The cpuid for the CPU node or ~0ULL if not found.
>> + */
>> +static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
>> +{
>> +    const __be32 *cell;
>> +    int ac;
> This is bogus (should be unsigned int afaict), but dictated by ...
>
>> +    uint32_t len;
>> +
>> +    ac = dt_n_addr_cells(cpun);
> ... the return value here and ...
>
>> +    cell = dt_get_property(cpun, "reg", &len);
>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
>> +        return ~0ULL;
> (Nit: This doesn't match the return type of the function; same for
> the function comment. Also, what if sizeof(*cell) * ac < len?)
>
>> +    return dt_read_number(cell, ac);
> ... the function parameter type here.

I think that we can declare ac as unsigned int even dt_n_addr_cells
return int as basically it returns be32_to_cpu(*ip) which return unsigned
int and it isn't expected to be a big value so overflow of INT_MAX shouldn't
happen and it won't affect a size argument of dt_read_number().

> In fact, that function is raising
> another question: If the "size" argument is outside of [0, 2], the value
> returned is silently truncated.

Usually address-cells is 1 or 2, so it isn't expected to be higher (but DT tells
that the value is u32 so it could be higher then 2). And I think it could be also
explained by bitness of an architecture.
I think I see address-cells equal to 3 for something connected to PCI, but
probably another one functions should be used to read.

>
> More generally - are there any plans to make DT code signed-ness-correct?
>
>> +/*
>> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
>> + * isn't an enabled and valid RISC-V hart node.
>> + */
>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>> +{
>> +    const char *isa;
>> +
>> +    if ( !dt_device_is_compatible(node, "riscv") )
>> +    {
>> +        printk("Found incompatible CPU\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    *cpuid = dt_get_cpuid(node);
>> +    if ( *cpuid == ~0UL )
>> +    {
>> +        printk("Found CPU without CPU ID\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( !dt_device_is_available(node))
>> +    {
>> +        printk("CPU with cpuid=%lu is not available\n", *cpuid);
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( dt_property_read_string(node, "riscv,isa", &isa) )
>> +    {
>> +        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( isa[0] != 'r' || isa[1] != 'v' )
>> +    {
>> +        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
>> +        return -ENODEV;
>> +    }
>> +
>> +    return 0;
>> +}
> I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
> messages for all of the cases, but surely there are errno values better
> representing the individual failure reasons?

I will update that to:

@@ -46,6 +46,7 @@ static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
  int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
  {
      const char *isa;
+    int ret;
  
      if ( !dt_device_is_compatible(node, "riscv") )
      {
@@ -57,7 +58,7 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
      if ( *cpuid == ~0UL )
      {
          printk("Found CPU without CPU ID\n");
-        return -ENODEV;
+        return -EINVAL;
      }
  
      if ( !dt_device_is_available(node))
@@ -66,16 +67,16 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
          return -ENODEV;
      }
  
-    if ( dt_property_read_string(node, "riscv,isa", &isa) )
+    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
      {
          printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
-        return -ENODEV;
+        return ret;
      }
  
      if ( isa[0] != 'r' || isa[1] != 'v' )
      {
          printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
-        return -ENODEV;
+        return -EINVAL;
      }
  
      return 0;

I think it's better now.

Thanks.

~ Oleksii

On 16.05.2025 18:02, Oleksii Kurochko wrote:
> 
> On 5/15/25 9:56 AM, Jan Beulich wrote:
>> (adding Bertrand as the one further DT maintainer, for a respective question
>> below)
>>
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> Implements dt_processor_hartid()
>> There's no such function (anymore).
>>
>>> to get the hart ID of the given
>>> device tree node and do some checks if CPU is available and given device
>>> tree node has proper riscv,isa property.
>>>
>>> As a helper function dt_get_cpuid() is introduced to deal specifically
>>> with reg propery of a CPU device node.
>>>
>>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>>> ---
>>> Changes in V2:
>>>   - s/of_get_cpu_hwid()/dt_get_cpu_id().
>>>   - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
>>>   - Add empty line before last return in dt_get_cpu_hwid().
>>>   - s/riscv_of_processor_hartid/dt_processor_cpuid().
>>>   - Use pointer-to_const for node argument of dt_processor_cpuid().
>>>   - Use for hart_id unsigned long type as according to the spec for RV128
>>>     mhartid register will be 128 bit long.
>>>   - Update commit message and subject.
>>>   - use 'CPU' instead of 'HART'.
>> Was this is good move? What is returned ...
>>
>>> --- a/xen/arch/riscv/include/asm/smp.h
>>> +++ b/xen/arch/riscv/include/asm/smp.h
>>> @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
>>>   
>>>   void setup_tp(unsigned int cpuid);
>>>   
>>> +struct dt_device_node;
>>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);
>> ... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm
>> not sure it's a hart ID either, so it may need further clarification (and I'd
>> expect RISC-V to have suitable terminology to tell apart the different entities).
> 
>  From device tree point of view (https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt)
> it is just hart which is defined as a hardware execution context, which contains all the state mandated by
> the RISC-V ISA: a PC and some registers.
> And also based on this for DT binding:
>    For example, my Intel laptop is
>    described as having one socket with two cores, each of which has two hyper
>    threads.  Therefore this system has four harts.
> They are using just harts and in DT it will look like 4 node with a number in reg property which they
> call hart. So from DT point of view hartid is pretty fine to use.
> 
>  From specification point of view (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_hardware_platform_terminology):
>    A RISC-V hardware platform can contain one or more RISC-V-compatible processing cores together
>    with other non-RISC-V-compatible cores, fixed-function accelerators, various physical memory
>    structures, I/O devices, and an interconnect structure to allow the components to communicate.
> 
>    A component is termed a core if it contains an independent instruction fetch unit. A RISC-V-
>    compatible core might support multiple RISC-V-compatible hardware threads, or harts, through
>    multithreading.
> and (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_software_execution_environments_and_harts):
>    From the perspective of software running in a given execution environment, a hart is a resource that
>    autonomously fetches and executes RISC-V instructions within that execution environment. In this
>    respect, a hart behaves like a hardware thread resource even if time-multiplexed onto real hardware by
>    the execution environment. Some EEIs support the creation and destruction of additional harts, for
>    example, via environment calls to fork new harts.
> 
> DT's CPU node should be refer to core plus hardware thread (or threads in case of multithreading)
> in reg property to be close to the spec(?) but basically in DT they IMO just describe what in the sepc
> is called an independent instruction fetch unit.
> 
> I still think that it is fine just to use hart_id. If to be close more to a spec the unit_id(?)
> but it is more confusing IMO with what is use in RISC-V's DT binding.

Based on what you quoted above I think "hart ID" is indeed appropriate here.

>>> +/*
>>> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
>>> + * isn't an enabled and valid RISC-V hart node.
>>> + */
>>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>>> +{
>>> +    const char *isa;
>>> +
>>> +    if ( !dt_device_is_compatible(node, "riscv") )
>>> +    {
>>> +        printk("Found incompatible CPU\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    *cpuid = dt_get_cpuid(node);
>>> +    if ( *cpuid == ~0UL )
>>> +    {
>>> +        printk("Found CPU without CPU ID\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( !dt_device_is_available(node))
>>> +    {
>>> +        printk("CPU with cpuid=%lu is not available\n", *cpuid);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( dt_property_read_string(node, "riscv,isa", &isa) )
>>> +    {
>>> +        printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( isa[0] != 'r' || isa[1] != 'v' )
>>> +    {
>>> +        printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>> I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
>> messages for all of the cases, but surely there are errno values better
>> representing the individual failure reasons?
> 
> I will update that to:
> 
> @@ -46,6 +46,7 @@ static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
>   int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>   {
>       const char *isa;
> +    int ret;
>   
>       if ( !dt_device_is_compatible(node, "riscv") )
>       {
> @@ -57,7 +58,7 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>       if ( *cpuid == ~0UL )
>       {
>           printk("Found CPU without CPU ID\n");
> -        return -ENODEV;
> +        return -EINVAL;

EINVAL is the most commonly used error code; I'd therefore recommend to
avoid its use unless it is indeed properly describing the situation
(invalid argument, i.e. invalid value _passed in_). Here ENODATA might
be a suitable replacement, for example.

Jan

>       }
>   
>       if ( !dt_device_is_available(node))
> @@ -66,16 +67,16 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
>           return -ENODEV;
>       }
>   
> -    if ( dt_property_read_string(node, "riscv,isa", &isa) )
> +    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
>       {
>           printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
> -        return -ENODEV;
> +        return ret;
>       }
>   
>       if ( isa[0] != 'r' || isa[1] != 'v' )
>       {
>           printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
> -        return -ENODEV;
> +        return -EINVAL;
>       }
>   
>       return 0;
> 
> I think it's better now.
> 
> Thanks.
> 
> ~ Oleksii
>