On 04/24/2020 06:26 PM, Naveen N. Rao wrote:

quoted

Steven Rostedt wrote:

quoted

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

quoted

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)

quoted

      }
  }

quoted

+#define PATCH_INSN(addr, instr)                             \

+do {                                         \
+    int rc = patch_instruction((unsigned int *)(addr), 

instr);         \

quoted

+    if (rc) {                                 \
+        pr_err("%s:%d Error patching instruction at 0x%pK (%pS): 

%d\n", \

quoted

+                __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.

Sure it's be more explicit, but then more lines also. 3 lines for only 
one really usefull.

With goto, I would look like:

diff --git a/arch/powerpc/kernel/optprobes.c 

b/arch/powerpc/kernel/optprobes.c
index 046485bb0a52..938208f824da 100644

--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c

@@ -139,14 +139,14 @@ void arch_remove_optimized_kprobe(struct 

optimized_kprobe *op)
  	}
  }

-#define PATCH_INSN(addr, instr)						     \
+#define PATCH_INSN(addr, instr, label)						     \
  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;						     \
+		goto label;						     \
  	}								     \
  } while (0)

@@ -291,23 +297,8 @@ int arch_prepare_optimized_kprobe(struct 

optimized_kprobe *op, struct kprobe *p)
  		goto error;
  	}

-	rc = patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
-	if (rc) {
-		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
-				__func__, __LINE__,
-				(void *)(buff + TMPL_CALL_HDLR_IDX), rc);
-		rc = -EFAULT;
-		goto error;
-	}
-
-	rc = patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
-	if (rc) {
-		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
-				__func__, __LINE__,
-				(void *)(buff + TMPL_EMULATE_IDX), rc);
-		rc = -EFAULT;
-		goto error;
-	}
+	PATCH_INSN(buff + TMPL_CALL_HDLR_IDX, branch_op_callback, efault);
+	PATCH_INSN(buff + TMPL_EMULATE_IDX, branch_emulate_step, efault);