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
