On 3/29/19 5:50 AM, Michael Ellerman wrote:

Hi Mahesh,

Thanks for doing this series.

Mahesh J Salgaonkar [off-list ref] writes:

quoted

From: Mahesh Salgaonkar <redacted>

Also add cpu number while displaying mce log. This will help cleaner logs
when mce hits on multiple cpus simultaneously.

Can you include some examples of the output before and after, so it's
easier to compare what the changes are.

Sure will add that in next revision.

I think you have an example in patch 3, but it would be good to have it here.

quoted

diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
index b5fec1f9751a..44614462cb34 100644
--- a/arch/powerpc/kernel/mce.c
+++ b/arch/powerpc/kernel/mce.c

@@ -384,101 +387,100 @@ void machine_check_print_event_info(struct machine_check_event *evt,
 		break;
 	}
 
-	printk("%s%s Machine check interrupt [%s]\n", level, sevstr,

I think I'd still like the first line at least to include "machine
check" somewhere, I'm not sure everyone will understand what "MCE" means.

I reduced it to MCE so that I can pack in more stuff in 80 columns. But
you are right, let me see.

...

quoted

+
+	if (ea && evt->srr0 != ea)
+		sprintf(dar_str, "DAR: %016llx ", ea);
+	else
+		memset(dar_str, 0, sizeof(dar_str));

Just dar_str[0] = '\0' would work wouldn't it?

Yeah, that also should be enough.

quoted

+	if (in_guest || user_mode) {
+		printk("%sMCE: CPU%d: (%s) %s %s %s at %016llx %s[%s]\n",
+			level, evt->cpu, sevstr,
+			in_guest ? "Guest" : "Host",
+			err_type, subtype, evt->srr0, dar_str,
+			evt->disposition == MCE_DISPOSITION_RECOVERED ?
+			"Recovered" : "Not recovered");
+		printk("%sMCE: CPU%d: PID: %d Comm: %s\n",
+			level, evt->cpu, current->pid, current->comm);
+	} else {
+		printk("%sMCE: CPU%d: (%s) Host %s %s at %016llx %s[%s]\n",
+			level, evt->cpu, sevstr, err_type, subtype, evt->srr0,
+			dar_str,
+			evt->disposition == MCE_DISPOSITION_RECOVERED ?
+			"Recovered" : "Not recovered");
+		printk("%sMCE: CPU%d: NIP: [%016llx] %pS\n",
+			level, evt->cpu, evt->srr0, (void *)evt->srr0);
+	}

The first printf in the two cases is quite similar, seems like they
could be consolidated.

I also think it'd be clearer to print the NIP on the 2nd line in all
cases, rather than the first.

Sure, and I will then put "machine check" on 1st line like below ?

printk("%sMCE: CPU%d: machine check (%s) %s %s %s %s[%s]\n",

What about (untested) ?

  printk("%sMCE: CPU%d: (%s) %s %s %s %s[%s]\n",
	 level, evt->cpu, sevstr,
	 in_guest ? "Guest" : "Host",
	 err_type, subtype, dar_str,
	 evt->disposition == MCE_DISPOSITION_RECOVERED ?
	 "Recovered" : "Not recovered");
  
  if (in_guest || user_mode) {
	printk("%sMCE: CPU%d: PID: %d Comm: %s %sNIP: [%016llx]\n",
	       level, evt->cpu, current->pid, current->comm,
	       in_guest ? "Guest " : "", evt->srr0);
  } else {
	printk("%sMCE: CPU%d: NIP: [%016llx] %pS\n",
		level, evt->cpu, evt->srr0, (void *)evt->srr0);
  }

Sure, will make these changes.

Thanks,
-Mahesh.