Steven Rostedt wrote:

On Thu, 23 Apr 2020 17:41:52 +0200
Christophe Leroy [off-list ref] wrote:
  

quoted

quoted

diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..046485bb0a52 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c

@@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
  	}
  }
  
+#define PATCH_INSN(addr, instr)						     \
+do {									     \
+	int rc = patch_instruction((unsigned int *)(addr), instr);	     \
+	if (rc) {							     \
+		pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
+				__func__, __LINE__,			     \
+				(void *)(addr), (void *)(addr), rc);	     \
+		return rc;						     \
+	}								     \
+} while (0)
+  

I hate this kind of macro which hides the "return".

What about keeping the return action in the caller ?

Otherwise, what about implementing something based on the use of goto, 
on the same model as unsafe_put_user() for instance ?

Thanks for the review.

I noticed this as a warning from checkpatch.pl, but this looked compact 
and correct for use in the two following functions. You'll notice that I 
added it just before the two functions this is used in.

I suppose 'goto err' is usable too, but the ftrace code (patch 2) will 
end up with more changes. I'm also struggling to see how a 'goto' is 
less offensive. I think Steve's suggestion below would be the better way 
to go, to make things explicit.

#define PATCH_INSN(addr, instr) \
({
	int rc = patch_instruction((unsigned int *)(addr), instr);	     \
	if (rc)								     \
		pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
				__func__, __LINE__,			     \
				(void *)(addr), (void *)(addr), rc);	     \
	rc;								     \
})


Then you can just do:

	ret = PATCH_INSN(...);
	if (ret)
		return ret;

in the code.

That's really nice. However, in this case, I guess I can simply use an 
inline function? The primary reason I used the macro was for including a 
'return' statement in it.


Thanks for the review!
- Naveen