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(-)
diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
index 380e4e251399..9c460843442f 100644
--- a/drivers/firmware/efi/libstub/riscv-stub.c
+++ b/drivers/firmware/efi/libstub/riscv-stub.c
@@ -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;
}
--
2.25.1
On 1/28/22 05:50, Sunil V L wrote:
> 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]>
On 2/14/22 11:15, Andreas Schwab wrote:
> On Feb 14 2022, Heinrich Schuchardt wrote:
>
>> 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.
On 2/14/22 10:12, Andreas Schwab wrote:
> On Jan 28 2022, Sunil V L wrote:
>
>> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
>> index 380e4e251399..9c460843442f 100644
>> --- a/drivers/firmware/efi/libstub/riscv-stub.c
>> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
>> @@ -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".
>
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
On Feb 14 2022, Heinrich Schuchardt wrote:
> On 2/14/22 11:15, Andreas Schwab wrote:
>> On Feb 14 2022, Heinrich Schuchardt wrote:
>>
>>> 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.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Feb 14 2022, Heinrich Schuchardt wrote:
> On 2/14/22 10:12, Andreas Schwab wrote:
>> On Jan 28 2022, Sunil V L wrote:
>>
>>> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
>>> index 380e4e251399..9c460843442f 100644
>>> --- a/drivers/firmware/efi/libstub/riscv-stub.c
>>> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
>>> @@ -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".
>>
>
> 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.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Feb 14 2022, Heinrich Schuchardt wrote:
> 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.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
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
On Fri, Jan 28, 2022 at 10:20:04AM +0530, Sunil V L wrote:
> 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(-)
>
> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> index 380e4e251399..9c460843442f 100644
> --- a/drivers/firmware/efi/libstub/riscv-stub.c
> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> @@ -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;
> }
> --
> 2.25.1
>
On Jan 28 2022, Sunil V L wrote:
> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> index 380e4e251399..9c460843442f 100644
> --- a/drivers/firmware/efi/libstub/riscv-stub.c
> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> @@ -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".
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Mon, Feb 14, 2022 at 12:09:05PM +0100, Andreas Schwab wrote:
> On Feb 14 2022, Heinrich Schuchardt wrote:
>
> > On 2/14/22 11:15, Andreas Schwab wrote:
> >> On Feb 14 2022, Heinrich Schuchardt wrote:
> >>
> >>> 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.
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
>
> --
> Andreas Schwab, [email protected]
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
On Thu, Feb 17, 2022 at 2:55 AM Sunil V L <[email protected]> wrote:
>
> On Mon, Feb 14, 2022 at 12:09:05PM +0100, Andreas Schwab wrote:
> > On Feb 14 2022, Heinrich Schuchardt wrote:
> >
> > > On 2/14/22 11:15, Andreas Schwab wrote:
> > >> On Feb 14 2022, Heinrich Schuchardt wrote:
> > >>
> > >>> 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.
>
> 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.
> 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 :)
> Hi Ard, let me know your suggestion on how to proceed with this.
>
> Thanks
> Sunil
> >
> > --
> > Andreas Schwab, [email protected]
> > GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> > "And now for something completely different."
--
Regards,
Atish
On Thu, 17 Feb 2022 at 20:47, Atish Patra <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 2:55 AM Sunil V L <[email protected]> wrote:
> >
> > On Mon, Feb 14, 2022 at 12:09:05PM +0100, Andreas Schwab wrote:
> > > On Feb 14 2022, Heinrich Schuchardt wrote:
> > >
> > > > On 2/14/22 11:15, Andreas Schwab wrote:
> > > >> On Feb 14 2022, Heinrich Schuchardt wrote:
> > > >>
> > > >>> 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.
> >
> > 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.
>
> > 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 :)
>
>
> > 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.