| Message ID | 20190529084344.28562-12-rrichter@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | EDAC, mc, ghes: Fixes and updates to improve memory error reporting | expand |
Hi Robert, On 29/05/2019 09:44, Robert Richter wrote: > Almost duplicate code, remove it. almost? > Note: there is a difference in the calculation of the grain_bits, > using the edac_mc's version here. But is it the right thing to do? Is this an off-by-one bug being papered over as some cleanup? If so could you post a separate fix that can be picked up for an rc. Do Marvell have firmware that populates this field? ... Unless the argument is no one cares about this... From ghes_edac_report_mem_error(): | /* Error grain */ | if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK) | e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK); Fishy, why would the kernel page-size be relevant here? If physical_addr_mask were the same as PAGE_MASK this wouldn't this always give ~0? (masking logic like this always does my head in) /me gives it ago: | {1}[Hardware Error]: physical_address: 0x00000000deadbeef | {1}[Hardware Error]: physical_address_mask: 0xffffffffffff0000 | {1}[Hardware Error]: error_type: 6, master abort | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef | grain:-1 syndrome:0x0 - status(0x0000000000000001): reserved) That 'grain:-1' is because the calculated e->grain was an unlikely 0xffffffffffffffff. Patch incoming, if you could test it on your platform that'd be great. I don't think ghes_edac.c wants this '+1'. Thanks, James
On 29.05.19 16:12:38, James Morse wrote: > Hi Robert, > > On 29/05/2019 09:44, Robert Richter wrote: > > Almost duplicate code, remove it. > > almost? The grain ... as noted below. > > > > Note: there is a difference in the calculation of the grain_bits, > > using the edac_mc's version here. > > But is it the right thing to do? > > Is this an off-by-one bug being papered over as some cleanup? > If so could you post a separate fix that can be picked up for an rc. > > Do Marvell have firmware that populates this field? > > ... > > Unless the argument is no one cares about this... > > >From ghes_edac_report_mem_error(): > | /* Error grain */ > | if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK) > | e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK); > > Fishy, why would the kernel page-size be relevant here? That looked broken to me too, I did not put to much effort in fixing the grain yet. So I just took the edac_mc version first in the assumption, that one is working. It looks like the intention here is to limit the grain to the page size. But right, the calculation is wrong here. I am also going to reply to your patch you sent on this. > > If physical_addr_mask were the same as PAGE_MASK this wouldn't this always give ~0? > (masking logic like this always does my head in) > > /me gives it ago: > | {1}[Hardware Error]: physical_address: 0x00000000deadbeef > | {1}[Hardware Error]: physical_address_mask: 0xffffffffffff0000 > | {1}[Hardware Error]: error_type: 6, master abort > | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef > | grain:-1 syndrome:0x0 - status(0x0000000000000001): reserved) > > That 'grain:-1' is because the calculated e->grain was an unlikely 0xffffffffffffffff. > Patch incoming, if you could test it on your platform that'd be great. > > I don't think ghes_edac.c wants this '+1'. The +1 looks odd to me also for the edac_mc driver, but I need to take a closer look here as well as some logs suggest the grain is calculated correctly. I will do some further examination here and also respond to your patch. Thank you for review. -Robert
Hi Robert, On 03/06/2019 14:10, Robert Richter wrote: > On 29.05.19 16:12:38, James Morse wrote: >> On 29/05/2019 09:44, Robert Richter wrote: >>> Almost duplicate code, remove it. >> >>> Note: there is a difference in the calculation of the grain_bits, >>> using the edac_mc's version here. >> >> But is it the right thing to do? >> >> Is this an off-by-one bug being papered over as some cleanup? >> If so could you post a separate fix that can be picked up for an rc. >> >> Do Marvell have firmware that populates this field? >> >> ... >> >> Unless the argument is no one cares about this... >> >> >From ghes_edac_report_mem_error(): >> | /* Error grain */ >> | if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK) >> | e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK); >> >> Fishy, why would the kernel page-size be relevant here? > > That looked broken to me too, I did not put to much effort in fixing > the grain yet. So I just took the edac_mc version first in the > assumption, that one is working. (Ah, it would have been good to note this in the commit-message) > It looks like the intention here is to limit the grain to the page > size. I'm not convinced that makes sense. If some architecture let you configure the page-size, (as arm64 does), and your hypervisor had a bigger page-size, then any hardware fault would be rounded up to hypervisor's page-size. The kernel's page-size has very little to do with the error, it only matters for when we go unmapping stuff in memory_failure(). > But right, the calculation is wrong here. I am also going to > reply to your patch you sent on this. Thanks! >> If physical_addr_mask were the same as PAGE_MASK this wouldn't this always give ~0? >> (masking logic like this always does my head in) >> >> /me gives it ago: >> | {1}[Hardware Error]: physical_address: 0x00000000deadbeef >> | {1}[Hardware Error]: physical_address_mask: 0xffffffffffff0000 >> | {1}[Hardware Error]: error_type: 6, master abort >> | EDAC MC0: 1 CE Master abort on unknown label ( page:0xdead offset:0xbeef >> | grain:-1 syndrome:0x0 - status(0x0000000000000001): reserved) >> >> That 'grain:-1' is because the calculated e->grain was an unlikely 0xffffffffffffffff. >> Patch incoming, if you could test it on your platform that'd be great. >> >> I don't think ghes_edac.c wants this '+1'. > > The +1 looks odd to me also for the edac_mc driver, but I need to take > a closer look here as well as some logs suggest the grain is > calculated correctly. My theory on this is that ghes_edac.c is generating a grain like 0x1000, fls() does the right thing. Other edac drivers are generating a grain like 0xfff to describe the same size, fls() is now off-by-one, hence the addition. I don't have a platform where I can trigger any other edac driver to test this though. The way round this would be to put the grain_bits in struct edac_raw_error_desc so that ghes_edac.c can calculate it directly. Thanks, James
On 04.06.19 18:15:50, James Morse wrote: > On 03/06/2019 14:10, Robert Richter wrote: > > The +1 looks odd to me also for the edac_mc driver, but I need to take > > a closer look here as well as some logs suggest the grain is > > calculated correctly. > > My theory on this is that ghes_edac.c is generating a grain like 0x1000, fls() does the > right thing. Other edac drivers are generating a grain like 0xfff to describe the same > size, fls() is now off-by-one, hence the addition. > I don't have a platform where I can trigger any other edac driver to test this though. > > The way round this would be to put the grain_bits in struct edac_raw_error_desc so that > ghes_edac.c can calculate it directly. I think the grain calculation in edac_mc is broken from the beginning: 53f2d0289875 RAS: Add a tracepoint for reporting memory controller events The log we see in the patch desc is: mc_event: 1 Corrected error:memory read on memory stick DIMM_1A (mc:0 location:0:0:0 page:0x586b6e offset:0xa66 grain:32 syndrome:0x0 area:DMA) The grain reported in mc_event there is probably 8 (quad word, granularity in bytes) and calculates as follows: dimm->grain = 8 e->grain = dimm->grain grain_bits = fls_long(e->grain) + 1 = 4 + 1 = 5 __entry->grain_bits = grain_bits TP_printk() = 1 << __entry->grain_bits = 2 << 5 = 32 So the reported grain of 32 should actually be 8. I think the following is correct: grain_bits = fls_long(e->grain ? e->grain - 1 : 0); This also handles the case if e->grain is not a power of 2. Thoughts? -Robert
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index b1bd0a23d02b..8613a31dc86c 100644 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -1010,6 +1010,17 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type, struct edac_raw_error_desc *e) { char detail[80]; + u8 grain_bits; + + /* Report the error via the trace interface */ + grain_bits = fls_long(e->grain) + 1; + + if (IS_ENABLED(CONFIG_RAS)) + trace_mc_event(type, e->msg, e->label, e->error_count, + mci->mc_idx, e->top_layer, e->mid_layer, + e->low_layer, + (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page, + grain_bits, e->syndrome, e->other_detail); /* Memory type dependent details about the error */ if (type == HW_EVENT_ERR_CORRECTED) { @@ -1050,7 +1061,6 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, int row = -1, chan = -1; int pos[EDAC_MAX_LAYERS] = { top_layer, mid_layer, low_layer }; int i, n_labels = 0; - u8 grain_bits; struct edac_raw_error_desc *e = &mci->error_desc; bool per_layer_report = false; @@ -1192,16 +1202,6 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, if (p > e->location) *(p - 1) = '\0'; - /* Report the error via the trace interface */ - grain_bits = fls_long(e->grain) + 1; - - if (IS_ENABLED(CONFIG_RAS)) - trace_mc_event(type, e->msg, e->label, e->error_count, - mci->mc_idx, e->top_layer, e->mid_layer, - e->low_layer, - (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page, - grain_bits, e->syndrome, e->other_detail); - dimm = edac_get_dimm(mci, top_layer, mid_layer, low_layer); edac_raw_mc_handle_error(type, mci, dimm, e); diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index c18f16bc9e4d..f6ea4b070bfe 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -202,7 +202,6 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err) struct ghes_edac_pvt *pvt = ghes_pvt; unsigned long flags; char *p; - u8 grain_bits; if (!pvt) return; @@ -434,14 +433,6 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err) if (p > pvt->other_detail) *(p - 1) = '\0'; - /* Generate the trace event */ - grain_bits = fls_long(e->grain); - - trace_mc_event(type, e->msg, e->label, e->error_count, - mci->mc_idx, e->top_layer, e->mid_layer, e->low_layer, - (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page, - grain_bits, e->syndrome, e->other_detail); - dimm_info = edac_get_dimm_by_index(mci, e->top_layer); edac_raw_mc_handle_error(type, mci, dimm_info, e);
Almost duplicate code, remove it. Note: there is a difference in the calculation of the grain_bits, using the edac_mc's version here. Signed-off-by: Robert Richter <rrichter@marvell.com> --- drivers/edac/edac_mc.c | 22 +++++++++++----------- drivers/edac/ghes_edac.c | 9 --------- 2 files changed, 11 insertions(+), 20 deletions(-) -- 2.20.1