riscv/efi_stub: Fix get_boot_hartid_from_fdt() return value

PATCH In review Sunil V L 6 participants 12 messages Jan 28, 2022 – Feb 17, 2022

Subsystems (3)

Stable branch RISC-V EFI

lore

Share & tools

lore

Sunil V L

author

2022-01-28 04:50 UTC #0 lore raw

PATCH 1/1 riscv/efi_stub: Fix get_boot_hartid_from_fdt() return value

The get_boot_hartid_from_fdt() function currently returns U32_MAX for failure case which is not correct because U32_MAX is a valid hartid value. This patch fixes the issue by returning error code.

Fixes d7071743db31 ("RISC-V: Add EFI stub support.")

Cc [email protected]

Signed-off-by Sunil V L <[email protected]>

---
 drivers/firmware/efi/libstub/riscv-stub.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

drivers/firmware/efi/libstub/riscv-stub.c +10 -7

index 380e4e251399..9c460843442f 100644
@@ -25,7 +25,7 @@ typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long);
 
 static u32 hartid;
 
-static u32 get_boot_hartid_from_fdt(void)
+static int get_boot_hartid_from_fdt(void)
 {
 	const void *fdt;
 	int chosen_node, len;

@@ -33,23 +33,26 @@ static u32 get_boot_hartid_from_fdt(void)
 
 	fdt = get_efi_config_table(DEVICE_TREE_GUID);
 	if (!fdt)
-		return U32_MAX;
+		return -EINVAL;
 
 	chosen_node = fdt_path_offset(fdt, "/chosen");
 	if (chosen_node < 0)
-		return U32_MAX;
+		return -EINVAL;
 
 	prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
 	if (!prop || len != sizeof(u32))
-		return U32_MAX;
+		return -EINVAL;
 
-	return fdt32_to_cpu(*prop);
+	hartid = fdt32_to_cpu(*prop);
+	return 0;
 }
 
 efi_status_t check_platform_features(void)
 {
-	hartid = get_boot_hartid_from_fdt();
-	if (hartid == U32_MAX) {
+	int ret;
+
+	ret = get_boot_hartid_from_fdt();
+	if (ret) {
 		efi_err("/chosen/boot-hartid missing or invalid!\n");
 		return EFI_UNSUPPORTED;
 	}

Signature · 1 line

2.25.1

Heinrich Schuchardt

Reviewed-by on v1 1/1

2022-01-28 06:17 UTC #1 lore raw

Quoted 8 lines · Sunil V L

The get_boot_hartid_from_fdt() function currently returns U32_MAX for failure case which is not correct because U32_MAX is a valid hartid value. This patch fixes the issue by returning error code.
Fixes d7071743db31 ("RISC-V: Add EFI stub support.")
Cc [email protected]
Signed-off-by Sunil V L <[email protected]>

Reviewed-by Heinrich Schuchardt <[email protected]>

Sunil V L

author

2022-02-14 08:41 UTC #2 lore raw

In reply to Sunil V L

Hi Ard,
Could you please take this patch? Heinrich and Atish have added RB tag. Let me know if I need to do anything.
Thanks
Sunil

Quoted 61 lines · Sunil V L

Fixes d7071743db31 ("RISC-V: Add EFI stub support.")

Cc [email protected]

Signed-off-by Sunil V L <[email protected]>

---
 drivers/firmware/efi/libstub/riscv-stub.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

drivers/firmware/efi/libstub/riscv-stub.c +10 -7

index 380e4e251399..9c460843442f 100644
@@ -25,7 +25,7 @@ typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long);
 
 static u32 hartid;
 
-static u32 get_boot_hartid_from_fdt(void)
+static int get_boot_hartid_from_fdt(void)
 {
 	const void *fdt;
 	int chosen_node, len;

@@ -33,23 +33,26 @@ static u32 get_boot_hartid_from_fdt(void)
 
 	fdt = get_efi_config_table(DEVICE_TREE_GUID);
 	if (!fdt)
-		return U32_MAX;
+		return -EINVAL;
 
 	chosen_node = fdt_path_offset(fdt, "/chosen");
 	if (chosen_node < 0)
-		return U32_MAX;
+		return -EINVAL;
 
 	prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
 	if (!prop || len != sizeof(u32))
-		return U32_MAX;
+		return -EINVAL;
 
-	return fdt32_to_cpu(*prop);
+	hartid = fdt32_to_cpu(*prop);
+	return 0;
 }
 
 efi_status_t check_platform_features(void)
 {
-	hartid = get_boot_hartid_from_fdt();
-	if (hartid == U32_MAX) {
+	int ret;
+
+	ret = get_boot_hartid_from_fdt();
+	if (ret) {
 		efi_err("/chosen/boot-hartid missing or invalid!\n");
 		return EFI_UNSUPPORTED;
 	}

Signature · 1 line

2.25.1

Andreas Schwab

2022-02-14 09:13 UTC #3 lore raw

In reply to Sunil V L

drivers/firmware/efi/libstub/riscv-stub.c typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long); · line 28

Quoted 10 lines · Sunil V L

drivers/firmware/efi/libstub/riscv-stub.c +1 -1

index 380e4e251399..9c460843442f 100644
@@ -25,7 +25,7 @@ typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long);
 
 static u32 hartid;
 
-static u32 get_boot_hartid_from_fdt(void)
+static int get_boot_hartid_from_fdt(void)

I think the function should be renamed, now that it no longer returns the hart ID, but initializes a static variable as a side effect. Thus it no longer "gets", but "sets".

Signature · 3 lines

Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Heinrich Schuchardt

2022-02-14 09:25 UTC #4 lore raw

Quoted 8 lines · Andreas Schwab

Quoted 6 lines · Sunil V L
drivers/firmware/efi/libstub/riscv-stub.c +0 -0
index 380e4e251399..9c460843442f 100644
@@ -25,7 +25,7 @@ typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long);
 

Quoted 9 lines

drivers/firmware/efi/libstub/riscv-stub.c typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long); · line 28
Quoted 4 lines
drivers/firmware/efi/libstub/riscv-stub.c +1 -1
  static u32 hartid;
 
-static u32 get_boot_hartid_from_fdt(void)
+static int get_boot_hartid_from_fdt(void)
I think the function should be renamed, now that it no longer returns the hart ID, but initializes a static variable as a side effect. Thus it no longer "gets", but "sets".

set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.

Best regards

Heinrich

Andreas Schwab

2022-02-14 09:36 UTC #5 lore raw

Quoted 19 lines · Heinrich Schuchardt

Quoted 8 lines · Andreas Schwab
Quoted 6 lines · Sunil V L
drivers/firmware/efi/libstub/riscv-stub.c +0 -0
index 380e4e251399..9c460843442f 100644
@@ -25,7 +25,7 @@ typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long);
 
Quoted 9 lines
drivers/firmware/efi/libstub/riscv-stub.c typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long); · line 28
Quoted 4 lines
drivers/firmware/efi/libstub/riscv-stub.c +1 -1
  static u32 hartid;
 
-static u32 get_boot_hartid_from_fdt(void)
+static int get_boot_hartid_from_fdt(void)
I think the function should be renamed, now that it no longer returns the hart ID, but initializes a static variable as a side effect. Thus it no longer "gets", but "sets".

Quoted 2 lines

set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.

Then I guess a different, more fitting name needs to be found.

Signature · 3 lines

Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Andreas Schwab

2022-02-14 10:51 UTC #6 lore raw

In reply to Heinrich Schuchardt

Quoted 2 lines · Heinrich Schuchardt

set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.

initialize_boot_hartid would fit better.

Signature · 3 lines

Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Heinrich Schuchardt

2022-02-14 11:05 UTC #7 lore raw

Quoted 7 lines · Andreas Schwab

Quoted 2 lines · Heinrich Schuchardt
set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.
initialize_boot_hartid would fit better.

Another misnomer.

Andreas Schwab

2022-02-14 11:29 UTC #8 lore raw

Quoted 10 lines · Heinrich Schuchardt

Quoted 7 lines · Andreas Schwab
Quoted 2 lines · Heinrich Schuchardt
set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.
initialize_boot_hartid would fit better.
Another misnomer.

But the best fit so far.

Signature · 3 lines

Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Sunil V L

author

2022-02-17 10:55 UTC #9 lore raw

Quoted 8 lines · Andreas Schwab

Quoted 6 lines · Heinrich Schuchardt
Quoted 5 lines · Andreas Schwab
Quoted 2 lines · Heinrich Schuchardt
set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
initialize_boot_hartid would fit better.
Another misnomer.
But the best fit so far.

Can we use the name init_boot_hartid_from_fdt()? While I understand Heinrich's point, I think since we have "_from_fdt", this may be fine.

I didn't rename the function since it was not recommended to do multiple things in a "Fix" patch. If we can consider this as not very serious issue which needs a "Fix" patch, then I can combine this patch with the RISCV_EFI_BOOT_PROTOCOL patch series.

Hi Ard, let me know your suggestion on how to proceed with this.

Thanks
Sunil

Quoted 5 lines

Signature · 4 lines

-- 
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Atish Patra

2022-02-17 19:47 UTC #10 lore raw

Quoted 13 lines · Sunil V L

Quoted 11 lines · Andreas Schwab
Quoted 9 lines · Heinrich Schuchardt
Quoted 7 lines · Andreas Schwab
Quoted 2 lines · Heinrich Schuchardt
set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.
initialize_boot_hartid would fit better.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Another misnomer.
But the best fit so far.
Can we use the name init_boot_hartid_from_fdt()? While I understand Heinrich's point, I think since we have "_from_fdt", this may be fine.

init_boot_hartid_from_fdt or parse_boot_hartid_from_fdt

are definitely much better than the current one.

Quoted 5 lines

I didn't rename the function since it was not recommended to do multiple things in a "Fix" patch. If we can consider this as not very serious issue which needs a "Fix" patch, then I can combine this patch with the RISCV_EFI_BOOT_PROTOCOL patch series.

IMHO, it is okay to include this in the RISCV_EFI_BOOT_PROTOCOL series as we are not going to have hartid U32_MAX in a few months :)

Quoted 9 lines

Hi Ard, let me know your suggestion on how to proceed with this.
Thanks
Sunil
Quoted 5 lines
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."

Signature · 2 lines

Regards,
Atish

Ard Biesheuvel

maintainer

2022-02-17 19:52 UTC #11 lore raw

Quoted 27 lines · Atish Patra

Quoted 13 lines · Sunil V L
Quoted 11 lines · Andreas Schwab
Quoted 9 lines · Heinrich Schuchardt
Quoted 7 lines · Andreas Schwab
Quoted 2 lines · Heinrich Schuchardt
set_boot_hartid() implies that the caller can change the boot hart ID. As this is not a case this name obviously would be a misnomer.
initialize_boot_hartid would fit better.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Another misnomer.
But the best fit so far.
Can we use the name init_boot_hartid_from_fdt()? While I understand Heinrich's point, I think since we have "_from_fdt", this may be fine.
init_boot_hartid_from_fdt or parse_boot_hartid_from_fdt
are definitely much better than the current one.

Quoted 12 lines

Quoted 5 lines
I didn't rename the function since it was not recommended to do multiple things in a "Fix" patch. If we can consider this as not very serious issue which needs a "Fix" patch, then I can combine this patch with the RISCV_EFI_BOOT_PROTOCOL patch series.
IMHO, it is okay to include this in the RISCV_EFI_BOOT_PROTOCOL series as we are not going to have hartid U32_MAX in a few months :)
Quoted 2 lines
Hi Ard, let me know your suggestion on how to proceed with this.

The patch is fine as it is. I agree that naming is important, but for a helper function that is only used a single time right in the same source file, it doesn't matter that much.

I have queued this up now.

Thanks,
Ard.