diff mbox series

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

Message ID 15baf76c271a0ae09f7b8556e50f2b4251e7049d.1674617130.git.jpoimboe@kernel.org
State New
Headers
Series powerpc: Fix livepatch module re-patching issue |

Commit Message

Josh Poimboeuf Jan. 25, 2023, 3:38 a.m. UTC 
  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 <jpoimboe@kernel.org>
---
 arch/powerpc/kernel/module_64.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Song Liu Jan. 25, 2023, 5:53 a.m. UTC | #1 
On Tue, Jan 24, 2023 at 7:38 PM Josh Poimboeuf <jpoimboe@kernel.org> 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 <jpoimboe@kernel.org>

Acked-by: Song Liu <song@kernel.org>
Petr Mladek Jan. 25, 2023, 1:24 p.m. UTC | #2 
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 <jpoimboe@kernel.org>

Looks good:

Reviewed-by: Petr Mladek <pmladek@suse.com>

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
Miroslav Benes Jan. 27, 2023, 12:38 p.m. UTC | #3 
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 <jpoimboe@kernel.org>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M
diff mbox series

Patch

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);