2023-01-25 03:38:33

by Josh Poimboeuf

Subject: [PATCH 1/2] powerpc/module_64: Improve restore_r2() return semantics

restore_r2() returns 1 on success, which is surprising for a non-boolean
function. Change it to return 0 on success and -errno on error to match
kernel coding convention.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/powerpc/kernel/module_64.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 1096d6b3a62c..016e79bba531 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -504,7 +504,7 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
u32 *prev_insn = instruction - 1;

if (is_mprofile_ftrace_call(name))
- return 1;
+ return 0;

/*
* Make sure the branch isn't a sibling call. Sibling calls aren't
@@ -512,19 +512,16 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
* restore afterwards.
*/
if (!instr_is_relative_link_branch(ppc_inst(*prev_insn)))
- return 1;
+ return 0;

if (*instruction != PPC_RAW_NOP()) {
pr_err("%s: Expected nop after call, got %08x at %pS\n",
me->name, *instruction, instruction);
- return 0;
+ return -ENOEXEC;
}

/* ld r2,R2_STACK_OFFSET(r1) */
- if (patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC)))
- return 0;
-
- return 1;
+ return patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC));
}

int apply_relocate_add(Elf64_Shdr *sechdrs,
@@ -648,8 +645,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
strtab + sym->st_name);
if (!value)
return -ENOENT;
- if (!restore_r2(strtab + sym->st_name,
- (u32 *)location + 1, me))
+ if (restore_r2(strtab + sym->st_name,
+ (u32 *)location + 1, me))
return -ENOEXEC;
} else
value += local_entry_offset(sym);
--
2.39.0

2023-01-25 05:54:18

by Song Liu

Subject: Re: [PATCH 1/2] powerpc/module_64: Improve restore_r2() return semantics

On Tue, Jan 24, 2023 at 7:38 PM Josh Poimboeuf <[email protected]> wrote:
>
> restore_r2() returns 1 on success, which is surprising for a non-boolean
> function. Change it to return 0 on success and -errno on error to match
> kernel coding convention.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>

Acked-by: Song Liu <[email protected]>

2023-01-25 13:25:00

by Petr Mladek

Subject: Re: [PATCH 1/2] powerpc/module_64: Improve restore_r2() return semantics

On Tue 2023-01-24 19:38:04, Josh Poimboeuf wrote:
> restore_r2() returns 1 on success, which is surprising for a non-boolean
> function. Change it to return 0 on success and -errno on error to match
> kernel coding convention.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>

Looks good:

Reviewed-by: Petr Mladek <[email protected]>

It is in the right direction. Just note that there are more functions
with the boolean semantic passed via int return value. But there
are also other functions already using the 0/-E* return values so
this is rather positive change.

Best Regards,
Petr

2023-01-27 12:40:48

by Miroslav Benes

Subject: Re: [PATCH 1/2] powerpc/module_64: Improve restore_r2() return semantics

On Tue, 24 Jan 2023, Josh Poimboeuf wrote:

> restore_r2() returns 1 on success, which is surprising for a non-boolean
> function. Change it to return 0 on success and -errno on error to match
> kernel coding convention.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>

Reviewed-by: Miroslav Benes <[email protected]>

M