Re: [RFC PATCH 3/5] powerpc: sstep: Add instruction emulation selftests

Hi Sandipan,

I'm not really confident to review the asm, but I did have a couple of
questions about the C:

+#define MAX_INSNS	32

This doesn't seem to be used...

+int execute_instr(struct pt_regs *regs, unsigned int instr)
+{
+	extern unsigned int exec_instr_execute[];
+	extern int exec_instr(struct pt_regs *regs);

These externs sit inside the function scope. This feels less than ideal
to me - is there a reason not to have these at global scope?

+
+	if (!regs || !instr)
+		return -EINVAL;
+
+	/* Patch the NOP with the actual instruction */
+	patch_instruction(&exec_instr_execute[0], instr);
+	if (exec_instr(regs)) {
+		pr_info("execution failed, opcode = 0x%08x\n", instr);
+		return -EFAULT;
+	}
+
+	return 0;
+}

+late_initcall(run_sstep_tests);

A design question: is there a reason to run these as an initcall rather
than as a module that could either be built in or loaded separately? I'm
not saying you have to do this, but I was wondering if you had
considered it?

Lastly, snowpatch reports some checkpatch issues for this and your
remaining patches: https://patchwork.ozlabs.org/patch/1035683/ (You are
allowed to violate checkpatch rules with justification, FWIW)

Regards,
Daniel

-- 
2.19.2