Re: [PATCH] x86/mce: Initialize "bank" when we find a fatal error in mce_no_way_out()

[Luck, Tony]

On Fri, Feb 01, 2019 at 10:55:53AM +0100, Borislav Petkov wrote:
> On Thu, Jan 31, 2019 at 04:33:41PM -0800, Tony Luck wrote:
> >  		if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
> > +			m->bank = i;
> 
> So conceptually this write belongs in...
> 
> >  			mce_read_aux(m, i);

Really? That function starts with the comment

	/*
	 * Read ADDR and MISC registers.
	 */

and the name is pretty descriptive that it is "reading auxiliary" information.

> 
> ... this function, i.e., in mce_read_aux() because it gets the bank
> number passed in already. And our calling pattern when populating struct
> mce is:
> 
> 	mce_gather_info()
> 	mce_read_aux()

Only two exising uses:

1) in machine_check_poll()

	mce_gather_info(&m, NULL);
	for (each bank) {
		m.bank = i;
		mce_read_aux(&m, i);
		...
	}

2) in __mc_scan_banks

	// mce_gather_info() already done in do_machine_check
	for (each bank) {
		m.bank = i;
		mce_read_aux(m, i);
		...
	}
> 
> so it'll be more robust if we moved it there.

It would be redundant to move it there for both
existing uses.

> Also, that argument "i" of mce_read_aux() is not very telling and it
> should be "bank" but that would complicate the stable backporting so if
> you feel like it, you could do a second, cleanup patch ontop to fix that
> too.

Agreed that "bank" is clearer than "i". But also agreed that should
be a separate patch for another day.

-Tony