* [PATCH v2 01/24] EDAC, mc: Fix grain_bits calculation
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
@ 2019-06-24 15:08 ` Robert Richter
2019-08-03 10:08 ` Borislav Petkov
2019-06-24 15:08 ` [PATCH v2 02/24] EDAC, ghes: Fix grain calculation Robert Richter
` (23 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:08 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
The grain in edac is defined as "minimum granularity for an error
report, in bytes". The following calculation of the grain_bits in
edac_mc is wrong:
grain_bits = fls_long(e->grain) + 1;
Where grain_bits is defined as:
grain = 1 << grain_bits
Example:
grain = 8 # 64 bit (8 bytes)
grain_bits = fls_long(8) + 1
grain_bits = 4 + 1 = 5
grain = 1 << grain_bits
grain = 1 << 5 = 32
Replacing it with the correct calculation:
grain_bits = fls_long(e->grain - 1);
The example gives now:
grain_bits = fls_long(8 - 1)
grain_bits = fls_long(8 - 1)
grain_bits = 3
grain = 1 << 3 = 8
Note: We need to check if the hardware reports a reasonable grain != 0
and fallback with a warn_once and 1 byte granularity otherwise.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 64922c8fa7e3..45cac74ab833 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -1235,9 +1235,15 @@ 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;
+ /*
+ * We expect the hw to report a reasonable grain, fallback to
+ * 1 byte granularity otherwise.
+ */
+ if (WARN_ON_ONCE(!e->grain))
+ e->grain = 1;
+ grain_bits = fls_long(e->grain - 1);
+ /* Report the error via the trace interface */
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,
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/24] EDAC, mc: Fix grain_bits calculation
2019-06-24 15:08 ` [PATCH v2 01/24] EDAC, mc: Fix grain_bits calculation Robert Richter
@ 2019-08-03 10:08 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-03 10:08 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:08:55PM +0000, Robert Richter wrote:
> The grain in edac is defined as "minimum granularity for an error
> report, in bytes". The following calculation of the grain_bits in
> edac_mc is wrong:
>
> grain_bits = fls_long(e->grain) + 1;
>
> Where grain_bits is defined as:
>
> grain = 1 << grain_bits
>
> Example:
>
> grain = 8 # 64 bit (8 bytes)
> grain_bits = fls_long(8) + 1
> grain_bits = 4 + 1 = 5
>
> grain = 1 << grain_bits
> grain = 1 << 5 = 32
>
> Replacing it with the correct calculation:
>
> grain_bits = fls_long(e->grain - 1);
>
> The example gives now:
>
> grain_bits = fls_long(8 - 1)
> grain_bits = fls_long(8 - 1)
> grain_bits = 3
>
> grain = 1 << 3 = 8
>
> Note: We need to check if the hardware reports a reasonable grain != 0
> and fallback with a warn_once and 1 byte granularity otherwise.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/edac_mc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
Applied to the new EDAC repo:
https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-for-next
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 02/24] EDAC, ghes: Fix grain calculation
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
2019-06-24 15:08 ` [PATCH v2 01/24] EDAC, mc: Fix grain_bits calculation Robert Richter
@ 2019-06-24 15:08 ` Robert Richter
2019-08-09 13:15 ` Borislav Petkov
2019-06-24 15:08 ` [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string Robert Richter
` (22 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:08 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
The conversion from the physical address mask to a grain (defined as
granularity in bytes) is broken:
e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
E.g., a physical address mask of ~0xfff should give a grain of 0x1000,
instead the grain is wrong with the upper bits always set. We also
remove the limitation to the page size as the granularity is unrelated
to the page size used in the system. We fix this with:
e->grain = ~mem_err->physical_addr_mask + 1;
Note: We need to adopt the grain_bits calculation as e->grain is now a
power of 2 and no longer a bit mask. The formula is now the same as in
edac_mc and can later be unified.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 7f19f1c672c3..d095d98d6a8d 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -222,6 +222,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
/* Cleans the error report buffer */
memset(e, 0, sizeof (*e));
e->error_count = 1;
+ e->grain = 1;
strcpy(e->label, "unknown label");
e->msg = pvt->msg;
e->other_detail = pvt->other_detail;
@@ -317,7 +318,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
/* Error grain */
if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
- e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
+ e->grain = ~mem_err->physical_addr_mask + 1;
/* Memory error location, mapped on e->location */
p = e->location;
@@ -433,8 +434,15 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
if (p > pvt->other_detail)
*(p - 1) = '\0';
+ /*
+ * We expect the hw to report a reasonable grain, fallback to
+ * 1 byte granularity otherwise.
+ */
+ if (WARN_ON_ONCE(!e->grain))
+ e->grain = 1;
+ grain_bits = fls_long(e->grain - 1);
+
/* Generate the trace event */
- grain_bits = fls_long(e->grain);
snprintf(pvt->detail_location, sizeof(pvt->detail_location),
"APEI location: %s %s", e->location, e->other_detail);
trace_mc_event(type, e->msg, e->label, e->error_count,
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 02/24] EDAC, ghes: Fix grain calculation
2019-06-24 15:08 ` [PATCH v2 02/24] EDAC, ghes: Fix grain calculation Robert Richter
@ 2019-08-09 13:15 ` Borislav Petkov
2019-08-12 6:42 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: Borislav Petkov @ 2019-08-09 13:15 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:08:57PM +0000, Robert Richter wrote:
> The conversion from the physical address mask to a grain (defined as
> granularity in bytes) is broken:
>
> e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
>
> E.g., a physical address mask of ~0xfff should give a grain of 0x1000,
> instead the grain is wrong with the upper bits always set. We also
> remove the limitation to the page size as the granularity is unrelated
> to the page size used in the system. We fix this with:
>
> e->grain = ~mem_err->physical_addr_mask + 1;
>
> Note: We need to adopt the grain_bits calculation as e->grain is now a
> power of 2 and no longer a bit mask. The formula is now the same as in
> edac_mc and can later be unified.
Please refrain from using "We" or "I" or etc personal pronouns in a
commit message and in the code comments below.
From Documentation/process/submitting-patches.rst:
"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."
Please fix all your other commit messages for the next submission.
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/ghes_edac.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 7f19f1c672c3..d095d98d6a8d 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -222,6 +222,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> /* Cleans the error report buffer */
> memset(e, 0, sizeof (*e));
> e->error_count = 1;
> + e->grain = 1;
> strcpy(e->label, "unknown label");
> e->msg = pvt->msg;
> e->other_detail = pvt->other_detail;
> @@ -317,7 +318,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>
> /* Error grain */
> if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
> - e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> + e->grain = ~mem_err->physical_addr_mask + 1;
This is assuming that that ->physical_addr_mask is contiguous but I
don't trust any firmware. I guess we can leave it like that for now
until some "inventive" firmware actually does it.
>
> /* Memory error location, mapped on e->location */
> p = e->location;
> @@ -433,8 +434,15 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> if (p > pvt->other_detail)
> *(p - 1) = '\0';
>
> + /*
> + * We expect the hw to report a reasonable grain, fallback to
> + * 1 byte granularity otherwise.
> + */
> + if (WARN_ON_ONCE(!e->grain))
Please move that WARN_ON_ONCE in the
if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
branch above because you're presetting grain to 1 so the warn should be
close to where it could happen, i.e., when coming from the firmware.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 02/24] EDAC, ghes: Fix grain calculation
2019-08-09 13:15 ` Borislav Petkov
@ 2019-08-12 6:42 ` Robert Richter
2019-08-12 7:32 ` Borislav Petkov
0 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-08-12 6:42 UTC (permalink / raw)
To: Borislav Petkov
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 09.08.19 15:15:59, Borislav Petkov wrote:
> On Mon, Jun 24, 2019 at 03:08:57PM +0000, Robert Richter wrote:
> > The conversion from the physical address mask to a grain (defined as
> > granularity in bytes) is broken:
> >
> > e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> >
> > E.g., a physical address mask of ~0xfff should give a grain of 0x1000,
> > instead the grain is wrong with the upper bits always set. We also
> > remove the limitation to the page size as the granularity is unrelated
> > to the page size used in the system. We fix this with:
> >
> > e->grain = ~mem_err->physical_addr_mask + 1;
> >
> > Note: We need to adopt the grain_bits calculation as e->grain is now a
> > power of 2 and no longer a bit mask. The formula is now the same as in
> > edac_mc and can later be unified.
>
> Please refrain from using "We" or "I" or etc personal pronouns in a
> commit message and in the code comments below.
>
> >From Documentation/process/submitting-patches.rst:
>
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."
>
> Please fix all your other commit messages for the next submission.
Sure, will reword.
I have seen you had actively promoted this style guideline, I even was
not aware of it, thanks for the pointer.
>
> > Signed-off-by: Robert Richter <rrichter@marvell.com>
> > ---
> > drivers/edac/ghes_edac.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index 7f19f1c672c3..d095d98d6a8d 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -222,6 +222,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> > /* Cleans the error report buffer */
> > memset(e, 0, sizeof (*e));
> > e->error_count = 1;
> > + e->grain = 1;
> > strcpy(e->label, "unknown label");
> > e->msg = pvt->msg;
> > e->other_detail = pvt->other_detail;
> > @@ -317,7 +318,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> >
> > /* Error grain */
> > if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
> > - e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> > + e->grain = ~mem_err->physical_addr_mask + 1;
>
> This is assuming that that ->physical_addr_mask is contiguous but I
> don't trust any firmware. I guess we can leave it like that for now
> until some "inventive" firmware actually does it.
With the grain_bits calculation the mask is rounded up to the next
power of 2 value. I therefore don't see any issues for non-contiguous
bit masks. I have updated the patch description.
>
> >
> > /* Memory error location, mapped on e->location */
> > p = e->location;
> > @@ -433,8 +434,15 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> > if (p > pvt->other_detail)
> > *(p - 1) = '\0';
> >
> > + /*
> > + * We expect the hw to report a reasonable grain, fallback to
> > + * 1 byte granularity otherwise.
> > + */
> > + if (WARN_ON_ONCE(!e->grain))
>
> Please move that WARN_ON_ONCE in the
>
> if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
>
> branch above because you're presetting grain to 1 so the warn should be
> close to where it could happen, i.e., when coming from the firmware.
The reason this is here is because this check will be moved to
edac_raw_mc_handle_error() to unify edac_mc and ghes code (see patch
#4). I understand the warn should be close to its source, on the other
side we need the check for all the drivers that setup the grain. Thus,
it cannot be in the driver that is setting up the grain.
Thanks,
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 02/24] EDAC, ghes: Fix grain calculation
2019-08-12 6:42 ` Robert Richter
@ 2019-08-12 7:32 ` Borislav Petkov
2019-08-12 12:05 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: Borislav Petkov @ 2019-08-12 7:32 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Aug 12, 2019 at 06:42:00AM +0000, Robert Richter wrote:
> I have seen you had actively promoted this style guideline, I even was
> not aware of it, thanks for the pointer.
It is about time we started writing proper commit messages. How long are
we trying, 20 years...?
> With the grain_bits calculation the mask is rounded up to the next
> power of 2 value.
mask = 0xffffffffff00ff00
~mask = 0x0000000000ff00ff
~mask + 1 = 0x0000000000ff0100
Your "trick" of adding a 1 to get to the most significant bit simply
doesn't work here. Thus:
"I guess we can leave it like that for now until some "inventive"
firmware actually does it."
> The reason this is here is because this check will be moved to
> edac_raw_mc_handle_error() to unify edac_mc and ghes code (see patch
> #4).
Ok.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 02/24] EDAC, ghes: Fix grain calculation
2019-08-12 7:32 ` Borislav Petkov
@ 2019-08-12 12:05 ` Robert Richter
2019-08-12 12:38 ` Borislav Petkov
0 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-08-12 12:05 UTC (permalink / raw)
To: Borislav Petkov
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 12.08.19 09:32:22, Borislav Petkov wrote:
> On Mon, Aug 12, 2019 at 06:42:00AM +0000, Robert Richter wrote:
> > With the grain_bits calculation the mask is rounded up to the next
> > power of 2 value.
>
> mask = 0xffffffffff00ff00
grain = ~mask + 1
> ~mask = 0x0000000000ff00ff
> ~mask + 1 = 0x0000000000ff0100
grain_bits = fls_long(e->grain - 1);
grain_bits = 24
grain = 1 << grain_bits
grain = 0x1000000
So for masks in the range from 0xffffffffff000000 to
0xffffffffff7fffff we have grain_bits set to 24, which corresponds to
a grain of 0x1000000. Looks good to me.
>
> Your "trick" of adding a 1 to get to the most significant bit simply
> doesn't work here. Thus:
>
> "I guess we can leave it like that for now until some "inventive"
> firmware actually does it."
Fine to me.
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 02/24] EDAC, ghes: Fix grain calculation
2019-08-12 12:05 ` Robert Richter
@ 2019-08-12 12:38 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-12 12:38 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Aug 12, 2019 at 12:05:25PM +0000, Robert Richter wrote:
> So for masks in the range from 0xffffffffff000000 to
> 0xffffffffff7fffff we have grain_bits set to 24, which corresponds to
> a grain of 0x1000000.
I don't think you're reading what I'm trying to say so let me go into
more detail:
I'm very suspicious about any and all information we get from firmware.
I think that is clear why by now.
If we get an address mask, we better sanity-check that mask. For
example, whether it is contiguous or whether the set bits in it are even
making any sense and so on.
What you're doing is assuming the firmware will give you a sensible mask
and you start working with it without checking it.
For example, if you get a mask of 0xffffffffff00ff00, how do you know
that the grain bits are really 24? Says who? There's a hole in the damn
mask so it could just as well be *anything* *but* an address mask. Hell,
it can be some random garbage.
Do you catch my drift now?
But, since we don't use the grain all too much and don't depend on it
yet, we keep it simple and lazy for now:
> > "I guess we can leave it like that for now until some "inventive"
> > firmware actually does it."
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
2019-06-24 15:08 ` [PATCH v2 01/24] EDAC, mc: Fix grain_bits calculation Robert Richter
2019-06-24 15:08 ` [PATCH v2 02/24] EDAC, ghes: Fix grain calculation Robert Richter
@ 2019-06-24 15:08 ` Robert Richter
2019-08-02 17:04 ` James Morse
2019-08-13 8:09 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 04/24] EDAC, ghes: Unify trace_mc_event() code with edac_mc driver Robert Richter
` (21 subsequent siblings)
24 siblings, 2 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:08 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
The detail_location[] string in struct ghes_edac_pvt is complete
useless and data is just copied around. Put everything into
e->other_detail from the beginning.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index d095d98d6a8d..049de73c3bad 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -21,8 +21,7 @@ struct ghes_edac_pvt {
struct mem_ctl_info *mci;
/* Buffers for the error handling routine */
- char detail_location[240];
- char other_detail[160];
+ char other_detail[400];
char msg[80];
};
@@ -224,13 +223,14 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
e->error_count = 1;
e->grain = 1;
strcpy(e->label, "unknown label");
- e->msg = pvt->msg;
- e->other_detail = pvt->other_detail;
e->top_layer = -1;
e->mid_layer = -1;
e->low_layer = -1;
- *pvt->other_detail = '\0';
+ e->msg = pvt->msg;
+ e->other_detail = pvt->other_detail;
+
*pvt->msg = '\0';
+ *pvt->other_detail = '\0';
switch (sev) {
case GHES_SEV_CORRECTED:
@@ -361,6 +361,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
/* All other fields are mapped on e->other_detail */
p = pvt->other_detail;
+ p += snprintf(p, sizeof(pvt->other_detail),
+ "APEI location: %s ", e->location);
if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_STATUS) {
u64 status = mem_err->error_status;
@@ -443,12 +445,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
grain_bits = fls_long(e->grain - 1);
/* Generate the trace event */
- snprintf(pvt->detail_location, sizeof(pvt->detail_location),
- "APEI location: %s %s", e->location, e->other_detail);
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, pvt->detail_location);
+ grain_bits, e->syndrome, e->other_detail);
edac_raw_mc_handle_error(type, mci, e);
spin_unlock_irqrestore(&ghes_lock, flags);
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string
2019-06-24 15:08 ` [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string Robert Richter
@ 2019-08-02 17:04 ` James Morse
2019-08-07 9:00 ` Robert Richter
2019-08-13 8:09 ` Borislav Petkov
1 sibling, 1 reply; 52+ messages in thread
From: James Morse @ 2019-08-02 17:04 UTC (permalink / raw)
To: Robert Richter
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
Hi Robert,
On 24/06/2019 16:08, Robert Richter wrote:
> The detail_location[] string in struct ghes_edac_pvt is complete
> useless and data is just copied around. Put everything into
> e->other_detail from the beginning.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index d095d98d6a8d..049de73c3bad 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -21,8 +21,7 @@ struct ghes_edac_pvt {
> struct mem_ctl_info *mci;
>
> /* Buffers for the error handling routine */
> - char detail_location[240];
> - char other_detail[160];
> + char other_detail[400];
> char msg[80];
> };
>
> @@ -224,13 +223,14 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> e->error_count = 1;
> e->grain = 1;
> strcpy(e->label, "unknown label");
> - e->msg = pvt->msg;
> - e->other_detail = pvt->other_detail;
> e->top_layer = -1;
> e->mid_layer = -1;
> e->low_layer = -1;
> - *pvt->other_detail = '\0';
> + e->msg = pvt->msg;
> + e->other_detail = pvt->other_detail;
> +
> *pvt->msg = '\0';
> + *pvt->other_detail = '\0';
>
> switch (sev) {
> case GHES_SEV_CORRECTED:
> @@ -361,6 +361,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>
> /* All other fields are mapped on e->other_detail */
> p = pvt->other_detail;
> + p += snprintf(p, sizeof(pvt->other_detail),
> + "APEI location: %s ", e->location);
> if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_STATUS) {
> u64 status = mem_err->error_status;
>
> @@ -443,12 +445,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> grain_bits = fls_long(e->grain - 1);
>
> /* Generate the trace event */
> - snprintf(pvt->detail_location, sizeof(pvt->detail_location),
> - "APEI location: %s %s", e->location, e->other_detail);
> 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, pvt->detail_location);
> + grain_bits, e->syndrome, e->other_detail);
>
> edac_raw_mc_handle_error(type, mci, e);
> spin_unlock_irqrestore(&ghes_lock, flags);
After a game of spot-the-difference: you added a newline.
Reviewed-by: James Morse <james.morse@arm.com>
Previously here:
https://lore.kernel.org/linux-edac/7017c91e-8923-c8d2-26ca-875328ab855a@arm.com/
Please pick up tags when posting a new version.
Thanks,
James
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string
2019-08-02 17:04 ` James Morse
@ 2019-08-07 9:00 ` Robert Richter
0 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-08-07 9:00 UTC (permalink / raw)
To: James Morse
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 02.08.19 18:04:46, James Morse wrote:
> On 24/06/2019 16:08, Robert Richter wrote:
> > The detail_location[] string in struct ghes_edac_pvt is complete
> > useless and data is just copied around. Put everything into
> > e->other_detail from the beginning.
I am updating the description here to clarify it is only the internal
buffer that is removed. The other_detail[] string still provides a
decoded information of the apei error record.
> >
> > Signed-off-by: Robert Richter <rrichter@marvell.com>
> > ---
>
>
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index d095d98d6a8d..049de73c3bad 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -21,8 +21,7 @@ struct ghes_edac_pvt {
> > struct mem_ctl_info *mci;
> >
> > /* Buffers for the error handling routine */
> > - char detail_location[240];
> > - char other_detail[160];
> > + char other_detail[400];
> > char msg[80];
> > };
> >
> > @@ -224,13 +223,14 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> > e->error_count = 1;
> > e->grain = 1;
> > strcpy(e->label, "unknown label");
> > - e->msg = pvt->msg;
> > - e->other_detail = pvt->other_detail;
> > e->top_layer = -1;
> > e->mid_layer = -1;
> > e->low_layer = -1;
> > - *pvt->other_detail = '\0';
> > + e->msg = pvt->msg;
> > + e->other_detail = pvt->other_detail;
> > +
> > *pvt->msg = '\0';
> > + *pvt->other_detail = '\0';
There are not code changes here, just a reordering for better
comparization with its counterpart in edac_mc.c. However, I am
removing this hunk here.
> >
> > switch (sev) {
> > case GHES_SEV_CORRECTED:
> > @@ -361,6 +361,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> >
> > /* All other fields are mapped on e->other_detail */
> > p = pvt->other_detail;
> > + p += snprintf(p, sizeof(pvt->other_detail),
> > + "APEI location: %s ", e->location);
> > if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_STATUS) {
> > u64 status = mem_err->error_status;
> >
> > @@ -443,12 +445,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> > grain_bits = fls_long(e->grain - 1);
> >
> > /* Generate the trace event */
> > - snprintf(pvt->detail_location, sizeof(pvt->detail_location),
> > - "APEI location: %s %s", e->location, e->other_detail);
> > 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, pvt->detail_location);
> > + grain_bits, e->syndrome, e->other_detail);
> >
> > edac_raw_mc_handle_error(type, mci, e);
> > spin_unlock_irqrestore(&ghes_lock, flags);
>
> After a game of spot-the-difference: you added a newline.
> Reviewed-by: James Morse <james.morse@arm.com>
>
> Previously here:
> https://lore.kernel.org/linux-edac/7017c91e-8923-c8d2-26ca-875328ab855a@arm.com/
>
> Please pick up tags when posting a new version.
Let me know what you mean with this, I would like to ease review for
you. I tried to describe changes for v2 as detailed as possible.
Thank you.
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string
2019-06-24 15:08 ` [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string Robert Richter
2019-08-02 17:04 ` James Morse
@ 2019-08-13 8:09 ` Borislav Petkov
1 sibling, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-13 8:09 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:08:59PM +0000, Robert Richter wrote:
> The detail_location[] string in struct ghes_edac_pvt is complete
s/complete/completely/
> useless and data is just copied around. Put everything into
> e->other_detail from the beginning.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/ghes_edac.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 04/24] EDAC, ghes: Unify trace_mc_event() code with edac_mc driver
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (2 preceding siblings ...)
2019-06-24 15:08 ` [PATCH v2 03/24] EDAC, ghes: Remove pvt->detail_location string Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 05/24] EDAC, mc: Fix and improve sysfs init functions Robert Richter
` (20 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Duplicate code, remove it. The only difference is the missing
IS_ENABLED(CONFIG_RAS) switch in ghes_edac which we will need there
too.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 34 +++++++++++++++++-----------------
drivers/edac/ghes_edac.c | 16 +---------------
2 files changed, 18 insertions(+), 32 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 45cac74ab833..4bbc8aeddf30 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -1056,6 +1056,23 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
{
char detail[80];
int pos[EDAC_MAX_LAYERS] = { e->top_layer, e->mid_layer, e->low_layer };
+ u8 grain_bits;
+
+ /*
+ * We expect the hw to report a reasonable grain, fallback to
+ * 1 byte granularity otherwise.
+ */
+ if (WARN_ON_ONCE(!e->grain))
+ e->grain = 1;
+ grain_bits = fls_long(e->grain - 1);
+
+ /* Report the error via the trace interface */
+ 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) {
@@ -1095,7 +1112,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;
edac_dbg(3, "MC%d\n", mci->mc_idx);
@@ -1235,22 +1251,6 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
if (p > e->location)
*(p - 1) = '\0';
- /*
- * We expect the hw to report a reasonable grain, fallback to
- * 1 byte granularity otherwise.
- */
- if (WARN_ON_ONCE(!e->grain))
- e->grain = 1;
- grain_bits = fls_long(e->grain - 1);
-
- /* Report the error via the trace interface */
- 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);
-
edac_raw_mc_handle_error(type, mci, e);
}
EXPORT_SYMBOL_GPL(edac_mc_handle_error);
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 049de73c3bad..bd3be25d0d3f 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -200,7 +200,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;
@@ -436,21 +435,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
if (p > pvt->other_detail)
*(p - 1) = '\0';
- /*
- * We expect the hw to report a reasonable grain, fallback to
- * 1 byte granularity otherwise.
- */
- if (WARN_ON_ONCE(!e->grain))
- e->grain = 1;
- grain_bits = fls_long(e->grain - 1);
-
- /* Generate the trace event */
- 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);
-
edac_raw_mc_handle_error(type, mci, e);
+
spin_unlock_irqrestore(&ghes_lock, flags);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 05/24] EDAC, mc: Fix and improve sysfs init functions
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (3 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 04/24] EDAC, ghes: Unify trace_mc_event() code with edac_mc driver Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-13 8:26 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro Robert Richter
` (19 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Remove gotos as they just create overhead. Also, fix debug message for
the case edac_create_dimm_object() is failing.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc_sysfs.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 7c01e1cc030c..29dd9719f82f 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -655,8 +655,9 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
err = device_add(&dimm->dev);
if (err)
put_device(&dimm->dev);
-
- edac_dbg(0, "created rank/dimm device %s\n", dev_name(&dimm->dev));
+ else
+ edac_dbg(0, "created rank/dimm device %s\n",
+ dev_name(&dimm->dev));
return err;
}
@@ -938,7 +939,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
if (err < 0) {
edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
put_device(&mci->dev);
- goto out;
+ return err;
}
/*
@@ -987,7 +988,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
}
device_unregister(&mci->dev);
-out:
return err;
}
@@ -1044,10 +1044,8 @@ int __init edac_mc_sysfs_init(void)
int err;
mci_pdev = kzalloc(sizeof(*mci_pdev), GFP_KERNEL);
- if (!mci_pdev) {
- err = -ENOMEM;
- goto out;
- }
+ if (!mci_pdev)
+ return -ENOMEM;
mci_pdev->bus = edac_get_sysfs_subsys();
mci_pdev->type = &mc_attr_type;
@@ -1056,15 +1054,10 @@ int __init edac_mc_sysfs_init(void)
err = device_add(mci_pdev);
if (err < 0)
- goto out_put_device;
-
- edac_dbg(0, "device %s created\n", dev_name(mci_pdev));
-
- return 0;
+ put_device(mci_pdev);
+ else
+ edac_dbg(0, "device %s created\n", dev_name(mci_pdev));
- out_put_device:
- put_device(mci_pdev);
- out:
return err;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 05/24] EDAC, mc: Fix and improve sysfs init functions
2019-06-24 15:09 ` [PATCH v2 05/24] EDAC, mc: Fix and improve sysfs init functions Robert Richter
@ 2019-08-13 8:26 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-13 8:26 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:02PM +0000, Robert Richter wrote:
> Remove gotos as they just create overhead.
Overhead?
> Also, fix debug message for
> the case edac_create_dimm_object() is failing.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/edac_mc_sysfs.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 7c01e1cc030c..29dd9719f82f 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -655,8 +655,9 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
> err = device_add(&dimm->dev);
> if (err)
> put_device(&dimm->dev);
> -
> - edac_dbg(0, "created rank/dimm device %s\n", dev_name(&dimm->dev));
> + else
> + edac_dbg(0, "created rank/dimm device %s\n",
> + dev_name(&dimm->dev));
Please add a message to the error case too.
> return err;
> }
> @@ -938,7 +939,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
> if (err < 0) {
> edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
> put_device(&mci->dev);
> - goto out;
> + return err;
> }
>
> /*
> @@ -987,7 +988,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
> }
> device_unregister(&mci->dev);
>
> -out:
> return err;
> }
>
> @@ -1044,10 +1044,8 @@ int __init edac_mc_sysfs_init(void)
> int err;
>
> mci_pdev = kzalloc(sizeof(*mci_pdev), GFP_KERNEL);
> - if (!mci_pdev) {
> - err = -ENOMEM;
> - goto out;
> - }
> + if (!mci_pdev)
> + return -ENOMEM;
>
> mci_pdev->bus = edac_get_sysfs_subsys();
> mci_pdev->type = &mc_attr_type;
> @@ -1056,15 +1054,10 @@ int __init edac_mc_sysfs_init(void)
>
> err = device_add(mci_pdev);
> if (err < 0)
> - goto out_put_device;
> -
> - edac_dbg(0, "device %s created\n", dev_name(mci_pdev));
> -
> - return 0;
> + put_device(mci_pdev);
> + else
> + edac_dbg(0, "device %s created\n", dev_name(mci_pdev));
Ditto.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (4 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 05/24] EDAC, mc: Fix and improve sysfs init functions Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-13 14:59 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 07/24] EDAC: Kill EDAC_DIMM_OFF() macro Robert Richter
` (18 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab, Tony Luck,
Jason Baron, Qiuxu Zhuo, Tero Kristo
Cc: linux-edac, linux-kernel, Robert Richter
Get rid of this macro and instead use the new function
edac_get_dimm(). Also introduce the edac_get_dimm_by_index() function
for later use.
Semantic patch used:
@@ expression mci, a, b,c; @@
-EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, a, b, c)
+edac_get_dimm(mci, a, b, c)
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 1 +
drivers/edac/ghes_edac.c | 8 ++--
drivers/edac/i10nm_base.c | 3 +-
drivers/edac/i3200_edac.c | 3 +-
drivers/edac/i5000_edac.c | 5 +--
drivers/edac/i5100_edac.c | 3 +-
drivers/edac/i5400_edac.c | 4 +-
drivers/edac/i7300_edac.c | 3 +-
drivers/edac/i7core_edac.c | 3 +-
drivers/edac/ie31200_edac.c | 7 +---
drivers/edac/pnd2_edac.c | 4 +-
drivers/edac/sb_edac.c | 2 +-
drivers/edac/skx_base.c | 3 +-
drivers/edac/ti_edac.c | 2 +-
include/linux/edac.h | 84 +++++++++++++++++++++++--------------
15 files changed, 73 insertions(+), 62 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 4bbc8aeddf30..c959e8b1643c 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -439,6 +439,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
goto error;
mci->dimms[off] = dimm;
dimm->mci = mci;
+ dimm->idx = off;
/*
* Copy DIMM location and initialize it.
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index bd3be25d0d3f..8050f9577fe6 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -97,9 +97,8 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
if (dh->type == DMI_ENTRY_MEM_DEVICE) {
struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh;
- struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers,
- dimm_fill->count, 0, 0);
+ struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count,
+ 0, 0);
u16 rdr_mask = BIT(7) | BIT(13);
if (entry->size == 0xffff) {
@@ -521,8 +520,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
dimm_fill.mci = mci;
dmi_walk(ghes_edac_dmidecode, &dimm_fill);
} else {
- struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, 0, 0, 0);
+ struct dimm_info *dimm = edac_get_dimm(mci, 0, 0, 0);
dimm->nr_pages = 1;
dimm->grain = 128;
diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_base.c
index 6f06aec4877c..1fc9ea849ada 100644
--- a/drivers/edac/i10nm_base.c
+++ b/drivers/edac/i10nm_base.c
@@ -152,8 +152,7 @@ static int i10nm_get_dimm_config(struct mem_ctl_info *mci)
ndimms = 0;
for (j = 0; j < I10NM_NUM_DIMMS; j++) {
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, i, j, 0);
+ dimm = edac_get_dimm(mci, i, j, 0);
mtr = I10NM_GET_DIMMMTR(imc, i, j);
mcddrtcfg = I10NM_GET_MCDDRTCFG(imc, i, j);
edac_dbg(1, "dimmmtr 0x%x mcddrtcfg 0x%x (mc%d ch%d dimm%d)\n",
diff --git a/drivers/edac/i3200_edac.c b/drivers/edac/i3200_edac.c
index 299b441647cd..432b375a4075 100644
--- a/drivers/edac/i3200_edac.c
+++ b/drivers/edac/i3200_edac.c
@@ -392,8 +392,7 @@ static int i3200_probe1(struct pci_dev *pdev, int dev_idx)
unsigned long nr_pages;
for (j = 0; j < nr_channels; j++) {
- struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, i, j, 0);
+ struct dimm_info *dimm = edac_get_dimm(mci, i, j, 0);
nr_pages = drb_to_nr_pages(drbs, stacked, j, i);
if (nr_pages == 0)
diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
index 078a7351bf05..1a6f69c859ab 100644
--- a/drivers/edac/i5000_edac.c
+++ b/drivers/edac/i5000_edac.c
@@ -1275,9 +1275,8 @@ static int i5000_init_csrows(struct mem_ctl_info *mci)
if (!MTR_DIMMS_PRESENT(mtr))
continue;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers,
- channel / MAX_BRANCHES,
- channel % MAX_BRANCHES, slot);
+ dimm = edac_get_dimm(mci, channel / MAX_BRANCHES,
+ channel % MAX_BRANCHES, slot);
csrow_megs = pvt->dimm_info[slot][channel].megabytes;
dimm->grain = 8;
diff --git a/drivers/edac/i5100_edac.c b/drivers/edac/i5100_edac.c
index b506eef6b146..39ba7f2414ae 100644
--- a/drivers/edac/i5100_edac.c
+++ b/drivers/edac/i5100_edac.c
@@ -858,8 +858,7 @@ static void i5100_init_csrows(struct mem_ctl_info *mci)
if (!npages)
continue;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers,
- chan, rank, 0);
+ dimm = edac_get_dimm(mci, chan, rank, 0);
dimm->nr_pages = npages;
dimm->grain = 32;
diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
index 6f8bcdb9256a..a50a8707337b 100644
--- a/drivers/edac/i5400_edac.c
+++ b/drivers/edac/i5400_edac.c
@@ -1196,8 +1196,8 @@ static int i5400_init_dimms(struct mem_ctl_info *mci)
if (!MTR_DIMMS_PRESENT(mtr))
continue;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers,
- channel / 2, channel % 2, slot);
+ dimm = edac_get_dimm(mci, channel / 2, channel % 2,
+ slot);
size_mb = pvt->dimm_info[slot][channel].megabytes;
diff --git a/drivers/edac/i7300_edac.c b/drivers/edac/i7300_edac.c
index 7bf910d54d11..747ee36a808c 100644
--- a/drivers/edac/i7300_edac.c
+++ b/drivers/edac/i7300_edac.c
@@ -794,8 +794,7 @@ static int i7300_init_csrows(struct mem_ctl_info *mci)
for (ch = 0; ch < max_channel; ch++) {
int channel = to_channel(ch, branch);
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, branch, ch, slot);
+ dimm = edac_get_dimm(mci, branch, ch, slot);
dinfo = &pvt->dimm_info[slot][channel];
diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
index a71cca6eeb33..b3135b208f9a 100644
--- a/drivers/edac/i7core_edac.c
+++ b/drivers/edac/i7core_edac.c
@@ -585,8 +585,7 @@ static int get_dimm_config(struct mem_ctl_info *mci)
if (!DIMM_PRESENT(dimm_dod[j]))
continue;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers,
- i, j, 0);
+ dimm = edac_get_dimm(mci, i, j, 0);
banks = numbank(MC_DOD_NUMBANK(dimm_dod[j]));
ranks = numrank(MC_DOD_NUMRANK(dimm_dod[j]));
rows = numrow(MC_DOD_NUMROW(dimm_dod[j]));
diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c
index d26300f9cb07..4f65073f230b 100644
--- a/drivers/edac/ie31200_edac.c
+++ b/drivers/edac/ie31200_edac.c
@@ -490,9 +490,7 @@ static int ie31200_probe1(struct pci_dev *pdev, int dev_idx)
if (dimm_info[j][i].dual_rank) {
nr_pages = nr_pages / 2;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, (i * 2) + 1,
- j, 0);
+ dimm = edac_get_dimm(mci, (i * 2) + 1, j, 0);
dimm->nr_pages = nr_pages;
edac_dbg(0, "set nr pages: 0x%lx\n", nr_pages);
dimm->grain = 8; /* just a guess */
@@ -503,8 +501,7 @@ static int ie31200_probe1(struct pci_dev *pdev, int dev_idx)
dimm->dtype = DEV_UNKNOWN;
dimm->edac_mode = EDAC_UNKNOWN;
}
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, i * 2, j, 0);
+ dimm = edac_get_dimm(mci, i * 2, j, 0);
dimm->nr_pages = nr_pages;
edac_dbg(0, "set nr pages: 0x%lx\n", nr_pages);
dimm->grain = 8; /* same guess */
diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index 903a4f1fadcc..2f7dcafd84b1 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -1234,7 +1234,7 @@ static void apl_get_dimm_config(struct mem_ctl_info *mci)
if (!(chan_mask & BIT(i)))
continue;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, i, 0, 0);
+ dimm = edac_get_dimm(mci, i, 0, 0);
if (!dimm) {
edac_dbg(0, "No allocated DIMM for channel %d\n", i);
continue;
@@ -1314,7 +1314,7 @@ static void dnv_get_dimm_config(struct mem_ctl_info *mci)
if (!ranks_of_dimm[j])
continue;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, i, j, 0);
+ dimm = edac_get_dimm(mci, i, j, 0);
if (!dimm) {
edac_dbg(0, "No allocated DIMM for channel %d DIMM %d\n", i, j);
continue;
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 37746b045e18..3e5631562264 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -1620,7 +1620,7 @@ static int __populate_dimms(struct mem_ctl_info *mci,
}
for (j = 0; j < max_dimms_per_channel; j++) {
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, i, j, 0);
+ dimm = edac_get_dimm(mci, i, j, 0);
if (pvt->info.type == KNIGHTS_LANDING) {
pci_read_config_dword(pvt->knl.pci_channel[i],
knl_mtr_reg, &mtr);
diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
index a5c8fa3a249a..5af18f6206e9 100644
--- a/drivers/edac/skx_base.c
+++ b/drivers/edac/skx_base.c
@@ -177,8 +177,7 @@ static int skx_get_dimm_config(struct mem_ctl_info *mci)
pci_read_config_dword(imc->chan[i].cdev, 0x8C, &amap);
pci_read_config_dword(imc->chan[i].cdev, 0x400, &mcddrtcfg);
for (j = 0; j < SKX_NUM_DIMMS; j++) {
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
- mci->n_layers, i, j, 0);
+ dimm = edac_get_dimm(mci, i, j, 0);
pci_read_config_dword(imc->chan[i].cdev,
0x80 + 4 * j, &mtr);
if (IS_DIMM_PRESENT(mtr)) {
diff --git a/drivers/edac/ti_edac.c b/drivers/edac/ti_edac.c
index 6ac26d1b929f..8be3e89a510e 100644
--- a/drivers/edac/ti_edac.c
+++ b/drivers/edac/ti_edac.c
@@ -135,7 +135,7 @@ static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type)
u32 val;
u32 memsize;
- dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0);
+ dimm = edac_get_dimm(mci, 0, 0, 0);
val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG);
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 342dabda9c7e..1367a3fc544f 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -403,37 +403,6 @@ struct edac_mc_layer {
__i; \
})
-/**
- * EDAC_DIMM_PTR - Macro responsible to get a pointer inside a pointer array
- * for the element given by [layer0,layer1,layer2] position
- *
- * @layers: a struct edac_mc_layer array, describing how many elements
- * were allocated for each layer
- * @var: name of the var where we want to get the pointer
- * (like mci->dimms)
- * @nlayers: Number of layers at the @layers array
- * @layer0: layer0 position
- * @layer1: layer1 position. Unused if n_layers < 2
- * @layer2: layer2 position. Unused if n_layers < 3
- *
- * For 1 layer, this macro returns "var[layer0]";
- *
- * For 2 layers, this macro is similar to allocate a bi-dimensional array
- * and to return "var[layer0][layer1]";
- *
- * For 3 layers, this macro is similar to allocate a tri-dimensional array
- * and to return "var[layer0][layer1][layer2]";
- */
-#define EDAC_DIMM_PTR(layers, var, nlayers, layer0, layer1, layer2) ({ \
- typeof(*var) __p; \
- int ___i = EDAC_DIMM_OFF(layers, nlayers, layer0, layer1, layer2); \
- if (___i < 0) \
- __p = NULL; \
- else \
- __p = (var)[___i]; \
- __p; \
-})
-
struct dimm_info {
struct device dev;
@@ -443,6 +412,7 @@ struct dimm_info {
unsigned location[EDAC_MAX_LAYERS];
struct mem_ctl_info *mci; /* the parent */
+ int idx; /* index within the parent dimm array */
u32 grain; /* granularity of reported error in bytes */
enum dev_type dtype; /* memory device type */
@@ -669,4 +639,56 @@ struct mem_ctl_info {
bool fake_inject_ue;
u16 fake_inject_count;
};
+
+/**
+ * edac_get_dimm_by_index - Get DIMM info from a memory controller
+ * given by an index
+ *
+ * @mci: a struct mem_ctl_info
+ * @index: index in the memory controller's DIMM array
+ *
+ * Returns a struct dimm_info*.
+ */
+static inline struct dimm_info *
+edac_get_dimm_by_index(struct mem_ctl_info *mci, int index)
+{
+ if (index < 0 || index >= mci->tot_dimms)
+ return NULL;
+
+ if (WARN_ON_ONCE(mci->dimms[index]->idx != index))
+ return NULL;
+
+ return mci->dimms[index];
+}
+
+/**
+ * edac_get_dimm - Get DIMM info from a memory controller given by
+ * [layer0,layer1,layer2] position
+ *
+ * @mci: a struct mem_ctl_info
+ * @layer0: layer0 position
+ * @layer1: layer1 position. Unused if n_layers < 2
+ * @layer2: layer2 position. Unused if n_layers < 3
+ *
+ * For 1 layer, this macro returns "dimms[layer0]";
+ *
+ * For 2 layers, this macro is similar to allocate a bi-dimensional array
+ * and to return "dimms[layer0][layer1]";
+ *
+ * For 3 layers, this macro is similar to allocate a tri-dimensional array
+ * and to return "dimms[layer0][layer1][layer2]";
+ */
+static inline struct dimm_info *
+edac_get_dimm(struct mem_ctl_info *mci, int layer0, int layer1, int layer2)
+{
+ int index = layer0;
+
+ if (index >= 0 && mci->n_layers > 1)
+ index = index * mci->layers[1].size + layer1;
+ if (index >= 0 && mci->n_layers > 2)
+ index = index * mci->layers[2].size + layer2;
+
+ return edac_get_dimm_by_index(mci, index);
+}
+
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro
2019-06-24 15:09 ` [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro Robert Richter
@ 2019-08-13 14:59 ` Borislav Petkov
2019-08-27 12:20 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: Borislav Petkov @ 2019-08-13 14:59 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, Tony Luck, Jason Baron,
Qiuxu Zhuo, Tero Kristo, linux-edac, linux-kernel
> Subject: Re: [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro
s/Kill/Replace ... with/
On Mon, Jun 24, 2019 at 03:09:06PM +0000, Robert Richter wrote:
> Get rid of this macro and instead use the new function
> edac_get_dimm(). Also introduce the edac_get_dimm_by_index() function
> for later use.
Some blurb about *why* you're doing this won't hurt here.
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index bd3be25d0d3f..8050f9577fe6 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -97,9 +97,8 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
>
> if (dh->type == DMI_ENTRY_MEM_DEVICE) {
> struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh;
> - struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
> - mci->n_layers,
> - dimm_fill->count, 0, 0);
> + struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count,
> + 0, 0);
You can let it stick out.
> u16 rdr_mask = BIT(7) | BIT(13);
>
> if (entry->size == 0xffff) {
...
> diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
> index 6f8bcdb9256a..a50a8707337b 100644
> --- a/drivers/edac/i5400_edac.c
> +++ b/drivers/edac/i5400_edac.c
> @@ -1196,8 +1196,8 @@ static int i5400_init_dimms(struct mem_ctl_info *mci)
> if (!MTR_DIMMS_PRESENT(mtr))
> continue;
>
> - dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers,
> - channel / 2, channel % 2, slot);
> + dimm = edac_get_dimm(mci, channel / 2, channel % 2,
> + slot);
Also, let it stick out.
> @@ -669,4 +639,56 @@ struct mem_ctl_info {
> bool fake_inject_ue;
> u16 fake_inject_count;
> };
> +
> +/**
> + * edac_get_dimm_by_index - Get DIMM info from a memory controller
> + * given by an index
> + *
> + * @mci: a struct mem_ctl_info
> + * @index: index in the memory controller's DIMM array
> + *
> + * Returns a struct dimm_info*.
or NULL on failure.
> + */
> +static inline struct dimm_info *
> +edac_get_dimm_by_index(struct mem_ctl_info *mci, int index)
> +{
> + if (index < 0 || index >= mci->tot_dimms)
> + return NULL;
> +
> + if (WARN_ON_ONCE(mci->dimms[index]->idx != index))
> + return NULL;
> +
> + return mci->dimms[index];
> +}
> +
> +/**
> + * edac_get_dimm - Get DIMM info from a memory controller given by
> + * [layer0,layer1,layer2] position
> + *
> + * @mci: a struct mem_ctl_info
> + * @layer0: layer0 position
> + * @layer1: layer1 position. Unused if n_layers < 2
> + * @layer2: layer2 position. Unused if n_layers < 3
> + *
> + * For 1 layer, this macro returns "dimms[layer0]";
macro? Copy-paste I guess :)
Below too.
> + *
> + * For 2 layers, this macro is similar to allocate a bi-dimensional array
> + * and to return "dimms[layer0][layer1]";
> + *
> + * For 3 layers, this macro is similar to allocate a tri-dimensional array
> + * and to return "dimms[layer0][layer1][layer2]";
> + */
> +static inline struct dimm_info *
> +edac_get_dimm(struct mem_ctl_info *mci, int layer0, int layer1, int layer2)
> +{
> + int index = layer0;
> +
> + if (index >= 0 && mci->n_layers > 1)
Can layer0 be negative here to warrant that check?
> + index = index * mci->layers[1].size + layer1;
> + if (index >= 0 && mci->n_layers > 2)
Same here.
> + index = index * mci->layers[2].size + layer2;
> +
> + return edac_get_dimm_by_index(mci, index);
> +}
> +
> #endif
> --
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro
2019-08-13 14:59 ` Borislav Petkov
@ 2019-08-27 12:20 ` Robert Richter
0 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-08-27 12:20 UTC (permalink / raw)
To: Borislav Petkov
Cc: James Morse, Mauro Carvalho Chehab, Tony Luck, Jason Baron,
Qiuxu Zhuo, Tero Kristo, linux-edac, linux-kernel
On 13.08.19 16:59:47, Borislav Petkov wrote:
> > + *
> > + * For 2 layers, this macro is similar to allocate a bi-dimensional array
> > + * and to return "dimms[layer0][layer1]";
> > + *
> > + * For 3 layers, this macro is similar to allocate a tri-dimensional array
> > + * and to return "dimms[layer0][layer1][layer2]";
> > + */
> > +static inline struct dimm_info *
> > +edac_get_dimm(struct mem_ctl_info *mci, int layer0, int layer1, int layer2)
> > +{
> > + int index = layer0;
> > +
> > + if (index >= 0 && mci->n_layers > 1)
>
> Can layer0 be negative here to warrant that check?
Yes, if a mem controller can not determine a dimm's position, this
function can be called with layers set to -1.
I have reworked the patch according to all of your comments.
Thanks for review,
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 07/24] EDAC: Kill EDAC_DIMM_OFF() macro
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (5 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 06/24] EDAC: Kill EDAC_DIMM_PTR() macro Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-14 14:52 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator Robert Richter
` (17 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
We do not need to calculate the offset in the mc's dimm array, let's
just store the index in struct dimm_info and we can get rid of this
macro.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 13 ++++--------
drivers/edac/edac_mc_sysfs.c | 20 ++++--------------
include/linux/edac.h | 41 ------------------------------------
3 files changed, 8 insertions(+), 66 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index c959e8b1643c..c44bc83e8502 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -318,7 +318,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
unsigned size, tot_dimms = 1, count = 1;
unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
void *pvt, *p, *ptr = NULL;
- int i, j, row, chn, n, len, off;
+ int idx, i, j, row, chn, n, len;
bool per_rank = false;
BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
@@ -426,20 +426,15 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
memset(&pos, 0, sizeof(pos));
row = 0;
chn = 0;
- for (i = 0; i < tot_dimms; i++) {
+ for (idx = 0; idx < tot_dimms; idx++) {
chan = mci->csrows[row]->channels[chn];
- off = EDAC_DIMM_OFF(layer, n_layers, pos[0], pos[1], pos[2]);
- if (off < 0 || off >= tot_dimms) {
- edac_mc_printk(mci, KERN_ERR, "EDAC core bug: EDAC_DIMM_OFF is trying to do an illegal data access\n");
- goto error;
- }
dimm = kzalloc(sizeof(**mci->dimms), GFP_KERNEL);
if (!dimm)
goto error;
- mci->dimms[off] = dimm;
+ mci->dimms[idx] = dimm;
dimm->mci = mci;
- dimm->idx = off;
+ dimm->idx = idx;
/*
* Copy DIMM location and initialize it.
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 29dd9719f82f..a69e99206a6f 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -559,14 +559,8 @@ static ssize_t dimmdev_ce_count_show(struct device *dev,
{
struct dimm_info *dimm = to_dimm(dev);
u32 count;
- int off;
-
- off = EDAC_DIMM_OFF(dimm->mci->layers,
- dimm->mci->n_layers,
- dimm->location[0],
- dimm->location[1],
- dimm->location[2]);
- count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][off];
+
+ count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][dimm->idx];
return sprintf(data, "%u\n", count);
}
@@ -576,14 +570,8 @@ static ssize_t dimmdev_ue_count_show(struct device *dev,
{
struct dimm_info *dimm = to_dimm(dev);
u32 count;
- int off;
-
- off = EDAC_DIMM_OFF(dimm->mci->layers,
- dimm->mci->n_layers,
- dimm->location[0],
- dimm->location[1],
- dimm->location[2]);
- count = dimm->mci->ue_per_layer[dimm->mci->n_layers-1][off];
+
+ count = dimm->mci->ue_per_layer[dimm->mci->n_layers-1][dimm->idx];
return sprintf(data, "%u\n", count);
}
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 1367a3fc544f..2ee9b8598ae0 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -362,47 +362,6 @@ struct edac_mc_layer {
*/
#define EDAC_MAX_LAYERS 3
-/**
- * EDAC_DIMM_OFF - Macro responsible to get a pointer offset inside a pointer
- * array for the element given by [layer0,layer1,layer2]
- * position
- *
- * @layers: a struct edac_mc_layer array, describing how many elements
- * were allocated for each layer
- * @nlayers: Number of layers at the @layers array
- * @layer0: layer0 position
- * @layer1: layer1 position. Unused if n_layers < 2
- * @layer2: layer2 position. Unused if n_layers < 3
- *
- * For 1 layer, this macro returns "var[layer0] - var";
- *
- * For 2 layers, this macro is similar to allocate a bi-dimensional array
- * and to return "var[layer0][layer1] - var";
- *
- * For 3 layers, this macro is similar to allocate a tri-dimensional array
- * and to return "var[layer0][layer1][layer2] - var".
- *
- * A loop could be used here to make it more generic, but, as we only have
- * 3 layers, this is a little faster.
- *
- * By design, layers can never be 0 or more than 3. If that ever happens,
- * a NULL is returned, causing an OOPS during the memory allocation routine,
- * with would point to the developer that he's doing something wrong.
- */
-#define EDAC_DIMM_OFF(layers, nlayers, layer0, layer1, layer2) ({ \
- int __i; \
- if ((nlayers) == 1) \
- __i = layer0; \
- else if ((nlayers) == 2) \
- __i = (layer1) + ((layers[1]).size * (layer0)); \
- else if ((nlayers) == 3) \
- __i = (layer2) + ((layers[2]).size * ((layer1) + \
- ((layers[1]).size * (layer0)))); \
- else \
- __i = -EINVAL; \
- __i; \
-})
-
struct dimm_info {
struct device dev;
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 07/24] EDAC: Kill EDAC_DIMM_OFF() macro
2019-06-24 15:09 ` [PATCH v2 07/24] EDAC: Kill EDAC_DIMM_OFF() macro Robert Richter
@ 2019-08-14 14:52 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-14 14:52 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:09PM +0000, Robert Richter wrote:
> We do not need to calculate the offset in the mc's dimm array, let's
> just store the index in struct dimm_info and we can get rid of this
> macro.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/edac_mc.c | 13 ++++--------
> drivers/edac/edac_mc_sysfs.c | 20 ++++--------------
> include/linux/edac.h | 41 ------------------------------------
> 3 files changed, 8 insertions(+), 66 deletions(-)
I like this cleanup a lot. Good!
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index c959e8b1643c..c44bc83e8502 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -318,7 +318,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
> unsigned size, tot_dimms = 1, count = 1;
> unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
> void *pvt, *p, *ptr = NULL;
> - int i, j, row, chn, n, len, off;
> + int idx, i, j, row, chn, n, len;
> bool per_rank = false;
>
> BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
> @@ -426,20 +426,15 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
> memset(&pos, 0, sizeof(pos));
> row = 0;
> chn = 0;
> - for (i = 0; i < tot_dimms; i++) {
> + for (idx = 0; idx < tot_dimms; idx++) {
> chan = mci->csrows[row]->channels[chn];
> - off = EDAC_DIMM_OFF(layer, n_layers, pos[0], pos[1], pos[2]);
> - if (off < 0 || off >= tot_dimms) {
> - edac_mc_printk(mci, KERN_ERR, "EDAC core bug: EDAC_DIMM_OFF is trying to do an illegal data access\n");
> - goto error;
> - }
Btw, right around here there's a comment:
/*
* Allocate and fill the dimm structs
*/
and since you're cleaning up all this, can you please add another patch
which takes all that code under the comment and see if you can carve it
out into a separate helper edac_alloc_dimms() or so. Because that
edac_mc_alloc() function is huuuge.
Btw, the code under
/*
* Alocate and fill the csrow/channels structs
*/
begs to be a separate function too :-)
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (6 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 07/24] EDAC: Kill EDAC_DIMM_OFF() macro Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-14 15:18 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 09/24] EDAC, mc: Cleanup _edac_mc_free() code Robert Richter
` (16 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Make code more readable by introducing a mci_for_each_dimm() iterator.
Now, we just get a pointer to a struct dimm_info. Direct array access
using an index is no longer needed to iterate.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 18 ++++++++++--------
drivers/edac/edac_mc_sysfs.c | 34 +++++++++++++++-------------------
drivers/edac/ghes_edac.c | 8 ++++----
drivers/edac/i5100_edac.c | 11 +++++------
include/linux/edac.h | 7 +++++++
5 files changed, 41 insertions(+), 37 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index c44bc83e8502..27277ca46ab3 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -145,15 +145,18 @@ static void edac_mc_dump_channel(struct rank_info *chan)
edac_dbg(4, " channel->dimm = %p\n", chan->dimm);
}
-static void edac_mc_dump_dimm(struct dimm_info *dimm, int number)
+static void edac_mc_dump_dimm(struct dimm_info *dimm)
{
char location[80];
+ if (!dimm->nr_pages)
+ return;
+
edac_dimm_info_location(dimm, location, sizeof(location));
edac_dbg(4, "%s%i: %smapped as virtual row %d, chan %d\n",
dimm->mci->csbased ? "rank" : "dimm",
- number, location, dimm->csrow, dimm->cschannel);
+ dimm->idx, location, dimm->csrow, dimm->cschannel);
edac_dbg(4, " dimm = %p\n", dimm);
edac_dbg(4, " dimm->label = '%s'\n", dimm->label);
edac_dbg(4, " dimm->nr_pages = 0x%x\n", dimm->nr_pages);
@@ -703,6 +706,7 @@ EXPORT_SYMBOL_GPL(edac_get_owner);
int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
const struct attribute_group **groups)
{
+ struct dimm_info *dimm;
int ret = -EINVAL;
edac_dbg(0, "\n");
@@ -727,9 +731,8 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
if (csrow->channels[j]->dimm->nr_pages)
edac_mc_dump_channel(csrow->channels[j]);
}
- for (i = 0; i < mci->tot_dimms; i++)
- if (mci->dimms[i]->nr_pages)
- edac_mc_dump_dimm(mci->dimms[i], i);
+ mci_for_each_dimm(mci, dimm)
+ edac_mc_dump_dimm(dimm);
}
#endif
mutex_lock(&mem_ctls_mutex);
@@ -1104,6 +1107,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
const char *msg,
const char *other_detail)
{
+ struct dimm_info *dimm;
char *p;
int row = -1, chan = -1;
int pos[EDAC_MAX_LAYERS] = { top_layer, mid_layer, low_layer };
@@ -1163,9 +1167,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
p = e->label;
*p = '\0';
- for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm = mci->dimms[i];
-
+ mci_for_each_dimm(mci, dimm) {
if (top_layer >= 0 && top_layer != dimm->location[0])
continue;
if (mid_layer >= 0 && mid_layer != dimm->location[1])
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index a69e99206a6f..4d15c88a52cd 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -623,8 +623,7 @@ static const struct device_type dimm_attr_type = {
/* Create a DIMM object under specifed memory controller device */
static int edac_create_dimm_object(struct mem_ctl_info *mci,
- struct dimm_info *dimm,
- int index)
+ struct dimm_info *dimm)
{
int err;
dimm->mci = mci;
@@ -634,9 +633,9 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
dimm->dev.parent = &mci->dev;
if (mci->csbased)
- dev_set_name(&dimm->dev, "rank%d", index);
+ dev_set_name(&dimm->dev, "rank%d", dimm->idx);
else
- dev_set_name(&dimm->dev, "dimm%d", index);
+ dev_set_name(&dimm->dev, "dimm%d", dimm->idx);
dev_set_drvdata(&dimm->dev, dimm);
pm_runtime_forbid(&mci->dev);
@@ -910,7 +909,8 @@ static const struct device_type mci_attr_type = {
int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
const struct attribute_group **groups)
{
- int i, err;
+ struct dimm_info *dimm;
+ int err;
/* get the /sys/devices/system/edac subsys reference */
mci->dev.type = &mci_attr_type;
@@ -933,14 +933,13 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
/*
* Create the dimm/rank devices
*/
- for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm = mci->dimms[i];
+ mci_for_each_dimm(mci, dimm) {
/* Only expose populated DIMMs */
if (!dimm->nr_pages)
continue;
#ifdef CONFIG_EDAC_DEBUG
- edac_dbg(1, "creating dimm%d, located at ", i);
+ edac_dbg(1, "creating dimm%d, located at ", dimm->idx);
if (edac_debug_level >= 1) {
int lay;
for (lay = 0; lay < mci->n_layers; lay++)
@@ -950,9 +949,10 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
printk(KERN_CONT "\n");
}
#endif
- err = edac_create_dimm_object(mci, dimm, i);
+ err = edac_create_dimm_object(mci, dimm);
if (err) {
- edac_dbg(1, "failure: create dimm %d obj\n", i);
+ edac_dbg(1, "failure: create dimm %d obj\n",
+ dimm->idx);
goto fail_unregister_dimm;
}
}
@@ -967,12 +967,9 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
return 0;
fail_unregister_dimm:
- for (i--; i >= 0; i--) {
- struct dimm_info *dimm = mci->dimms[i];
- if (!dimm->nr_pages)
- continue;
-
- device_unregister(&dimm->dev);
+ mci_for_each_dimm(mci, dimm) {
+ if (device_is_registered(&dimm->dev))
+ device_unregister(&dimm->dev);
}
device_unregister(&mci->dev);
@@ -984,7 +981,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
*/
void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
{
- int i;
+ struct dimm_info *dimm;
edac_dbg(0, "\n");
@@ -995,8 +992,7 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
edac_delete_csrow_objects(mci);
#endif
- for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm = mci->dimms[i];
+ mci_for_each_dimm(mci, dimm) {
if (dimm->nr_pages == 0)
continue;
edac_dbg(0, "removing device %s\n", dev_name(&dimm->dev));
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 8050f9577fe6..72e75ea5526c 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -81,11 +81,11 @@ static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg)
static int get_dimm_smbios_index(u16 handle)
{
struct mem_ctl_info *mci = ghes_pvt->mci;
- int i;
+ struct dimm_info *dimm;
- for (i = 0; i < mci->tot_dimms; i++) {
- if (mci->dimms[i]->smbios_handle == handle)
- return i;
+ mci_for_each_dimm(mci, dimm) {
+ if (dimm->smbios_handle == handle)
+ return dimm->idx;
}
return -1;
}
diff --git a/drivers/edac/i5100_edac.c b/drivers/edac/i5100_edac.c
index 39ba7f2414ae..7ec42b26a716 100644
--- a/drivers/edac/i5100_edac.c
+++ b/drivers/edac/i5100_edac.c
@@ -846,14 +846,13 @@ static void i5100_init_interleaving(struct pci_dev *pdev,
static void i5100_init_csrows(struct mem_ctl_info *mci)
{
- int i;
struct i5100_priv *priv = mci->pvt_info;
+ struct dimm_info *dimm;
- for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm;
- const unsigned long npages = i5100_npages(mci, i);
- const unsigned chan = i5100_csrow_to_chan(mci, i);
- const unsigned rank = i5100_csrow_to_rank(mci, i);
+ mci_for_each_dimm(mci, dimm) {
+ const unsigned long npages = i5100_npages(mci, dimm->idx);
+ const unsigned chan = i5100_csrow_to_chan(mci, dimm->idx);
+ const unsigned rank = i5100_csrow_to_rank(mci, dimm->idx);
if (!npages)
continue;
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 2ee9b8598ae0..20a04f48616c 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -599,6 +599,13 @@ struct mem_ctl_info {
u16 fake_inject_count;
};
+#define mci_for_each_dimm(mci, dimm) \
+ for ((dimm) = (mci)->dimms[0]; \
+ (dimm); \
+ (dimm) = (dimm)->idx < (mci)->tot_dimms \
+ ? (mci)->dimms[(dimm)->idx + 1] \
+ : NULL)
+
/**
* edac_get_dimm_by_index - Get DIMM info from a memory controller
* given by an index
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator
2019-06-24 15:09 ` [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator Robert Richter
@ 2019-08-14 15:18 ` Borislav Petkov
2019-08-28 8:18 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: Borislav Petkov @ 2019-08-14 15:18 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:11PM +0000, Robert Richter wrote:
> Make code more readable by introducing a mci_for_each_dimm() iterator.
> Now, we just get a pointer to a struct dimm_info. Direct array access
> using an index is no longer needed to iterate.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/edac_mc.c | 18 ++++++++++--------
> drivers/edac/edac_mc_sysfs.c | 34 +++++++++++++++-------------------
> drivers/edac/ghes_edac.c | 8 ++++----
> drivers/edac/i5100_edac.c | 11 +++++------
> include/linux/edac.h | 7 +++++++
> 5 files changed, 41 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index c44bc83e8502..27277ca46ab3 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -145,15 +145,18 @@ static void edac_mc_dump_channel(struct rank_info *chan)
> edac_dbg(4, " channel->dimm = %p\n", chan->dimm);
> }
>
> -static void edac_mc_dump_dimm(struct dimm_info *dimm, int number)
> +static void edac_mc_dump_dimm(struct dimm_info *dimm)
> {
> char location[80];
>
> + if (!dimm->nr_pages)
> + return;
> +
What's that for?
> edac_dimm_info_location(dimm, location, sizeof(location));
>
> edac_dbg(4, "%s%i: %smapped as virtual row %d, chan %d\n",
> dimm->mci->csbased ? "rank" : "dimm",
> - number, location, dimm->csrow, dimm->cschannel);
> + dimm->idx, location, dimm->csrow, dimm->cschannel);
> edac_dbg(4, " dimm = %p\n", dimm);
> edac_dbg(4, " dimm->label = '%s'\n", dimm->label);
> edac_dbg(4, " dimm->nr_pages = 0x%x\n", dimm->nr_pages);
> @@ -703,6 +706,7 @@ EXPORT_SYMBOL_GPL(edac_get_owner);
> int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
> const struct attribute_group **groups)
> {
> + struct dimm_info *dimm;
> int ret = -EINVAL;
> edac_dbg(0, "\n");
>
> @@ -727,9 +731,8 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
> if (csrow->channels[j]->dimm->nr_pages)
> edac_mc_dump_channel(csrow->channels[j]);
> }
> - for (i = 0; i < mci->tot_dimms; i++)
> - if (mci->dimms[i]->nr_pages)
> - edac_mc_dump_dimm(mci->dimms[i], i);
<---- newline here.
> + mci_for_each_dimm(mci, dimm)
> + edac_mc_dump_dimm(dimm);
> }
> #endif
> mutex_lock(&mem_ctls_mutex);
...
> @@ -950,9 +949,10 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
> printk(KERN_CONT "\n");
> }
> #endif
> - err = edac_create_dimm_object(mci, dimm, i);
> + err = edac_create_dimm_object(mci, dimm);
> if (err) {
> - edac_dbg(1, "failure: create dimm %d obj\n", i);
> + edac_dbg(1, "failure: create dimm %d obj\n",
> + dimm->idx);
> goto fail_unregister_dimm;
> }
> }
Since you're touching this, pls do
s/dimm/DIMM/g
in the user-visible strings because it is an abbreviation.
> @@ -967,12 +967,9 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
> return 0;
>
> fail_unregister_dimm:
> - for (i--; i >= 0; i--) {
> - struct dimm_info *dimm = mci->dimms[i];
> - if (!dimm->nr_pages)
> - continue;
> -
> - device_unregister(&dimm->dev);
> + mci_for_each_dimm(mci, dimm) {
> + if (device_is_registered(&dimm->dev))
> + device_unregister(&dimm->dev);
> }
> device_unregister(&mci->dev);
>
> @@ -984,7 +981,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
> */
> void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
> {
> - int i;
> + struct dimm_info *dimm;
>
> edac_dbg(0, "\n");
>
> @@ -995,8 +992,7 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
> edac_delete_csrow_objects(mci);
> #endif
>
> - for (i = 0; i < mci->tot_dimms; i++) {
> - struct dimm_info *dimm = mci->dimms[i];
> + mci_for_each_dimm(mci, dimm) {
> if (dimm->nr_pages == 0)
> continue;
> edac_dbg(0, "removing device %s\n", dev_name(&dimm->dev));
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 8050f9577fe6..72e75ea5526c 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -81,11 +81,11 @@ static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg)
> static int get_dimm_smbios_index(u16 handle)
> {
> struct mem_ctl_info *mci = ghes_pvt->mci;
> - int i;
> + struct dimm_info *dimm;
>
> - for (i = 0; i < mci->tot_dimms; i++) {
> - if (mci->dimms[i]->smbios_handle == handle)
> - return i;
> + mci_for_each_dimm(mci, dimm) {
> + if (dimm->smbios_handle == handle)
> + return dimm->idx;
> }
> return -1;
> }
> diff --git a/drivers/edac/i5100_edac.c b/drivers/edac/i5100_edac.c
> index 39ba7f2414ae..7ec42b26a716 100644
> --- a/drivers/edac/i5100_edac.c
> +++ b/drivers/edac/i5100_edac.c
> @@ -846,14 +846,13 @@ static void i5100_init_interleaving(struct pci_dev *pdev,
>
> static void i5100_init_csrows(struct mem_ctl_info *mci)
> {
> - int i;
> struct i5100_priv *priv = mci->pvt_info;
> + struct dimm_info *dimm;
>
> - for (i = 0; i < mci->tot_dimms; i++) {
> - struct dimm_info *dimm;
> - const unsigned long npages = i5100_npages(mci, i);
> - const unsigned chan = i5100_csrow_to_chan(mci, i);
> - const unsigned rank = i5100_csrow_to_rank(mci, i);
> + mci_for_each_dimm(mci, dimm) {
> + const unsigned long npages = i5100_npages(mci, dimm->idx);
> + const unsigned chan = i5100_csrow_to_chan(mci, dimm->idx);
> + const unsigned rank = i5100_csrow_to_rank(mci, dimm->idx);
>
> if (!npages)
> continue;
This cannot be right: the code here under this does now:
dimm = edac_get_dimm(mci, chan, rank, 0);
but dimm is already set. It used to get the DIMM using chan and rank but
now you're iterating over the already initialized pointers. So I think
you don't need the edac_get_dimm() anymore.
Also fix those up too, while at it pls:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#235: FILE: drivers/edac/i5100_edac.c:854:
+ const unsigned chan = i5100_csrow_to_chan(mci, dimm->idx);
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#236: FILE: drivers/edac/i5100_edac.c:855:
+ const unsigned rank = i5100_csrow_to_rank(mci, dimm->idx);
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index 2ee9b8598ae0..20a04f48616c 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -599,6 +599,13 @@ struct mem_ctl_info {
> u16 fake_inject_count;
> };
>
> +#define mci_for_each_dimm(mci, dimm) \
> + for ((dimm) = (mci)->dimms[0]; \
> + (dimm); \
> + (dimm) = (dimm)->idx < (mci)->tot_dimms \
> + ? (mci)->dimms[(dimm)->idx + 1] \
> + : NULL)
> +
> /**
> * edac_get_dimm_by_index - Get DIMM info from a memory controller
> * given by an index
> --
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator
2019-08-14 15:18 ` Borislav Petkov
@ 2019-08-28 8:18 ` Robert Richter
0 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-08-28 8:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 14.08.19 17:18:24, Borislav Petkov wrote:
> On Mon, Jun 24, 2019 at 03:09:11PM +0000, Robert Richter wrote:
> > Make code more readable by introducing a mci_for_each_dimm() iterator.
> > Now, we just get a pointer to a struct dimm_info. Direct array access
> > using an index is no longer needed to iterate.
> >
> > Signed-off-by: Robert Richter <rrichter@marvell.com>
> > ---
> > drivers/edac/edac_mc.c | 18 ++++++++++--------
> > drivers/edac/edac_mc_sysfs.c | 34 +++++++++++++++-------------------
> > drivers/edac/ghes_edac.c | 8 ++++----
> > drivers/edac/i5100_edac.c | 11 +++++------
> > include/linux/edac.h | 7 +++++++
> > 5 files changed, 41 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> > index c44bc83e8502..27277ca46ab3 100644
> > --- a/drivers/edac/edac_mc.c
> > +++ b/drivers/edac/edac_mc.c
> > @@ -145,15 +145,18 @@ static void edac_mc_dump_channel(struct rank_info *chan)
> > edac_dbg(4, " channel->dimm = %p\n", chan->dimm);
> > }
> >
> > -static void edac_mc_dump_dimm(struct dimm_info *dimm, int number)
> > +static void edac_mc_dump_dimm(struct dimm_info *dimm)
> > {
> > char location[80];
> >
> > + if (!dimm->nr_pages)
> > + return;
> > +
>
> What's that for?
This is moved from the iterator loop below to here. It limits the dump
to only populated dimm slots.
>
> > edac_dimm_info_location(dimm, location, sizeof(location));
> >
> > edac_dbg(4, "%s%i: %smapped as virtual row %d, chan %d\n",
> > dimm->mci->csbased ? "rank" : "dimm",
> > - number, location, dimm->csrow, dimm->cschannel);
> > + dimm->idx, location, dimm->csrow, dimm->cschannel);
> > edac_dbg(4, " dimm = %p\n", dimm);
> > edac_dbg(4, " dimm->label = '%s'\n", dimm->label);
> > edac_dbg(4, " dimm->nr_pages = 0x%x\n", dimm->nr_pages);
> > diff --git a/drivers/edac/i5100_edac.c b/drivers/edac/i5100_edac.c
> > index 39ba7f2414ae..7ec42b26a716 100644
> > --- a/drivers/edac/i5100_edac.c
> > +++ b/drivers/edac/i5100_edac.c
> > @@ -846,14 +846,13 @@ static void i5100_init_interleaving(struct pci_dev *pdev,
> >
> > static void i5100_init_csrows(struct mem_ctl_info *mci)
> > {
> > - int i;
> > struct i5100_priv *priv = mci->pvt_info;
> > + struct dimm_info *dimm;
> >
> > - for (i = 0; i < mci->tot_dimms; i++) {
> > - struct dimm_info *dimm;
> > - const unsigned long npages = i5100_npages(mci, i);
> > - const unsigned chan = i5100_csrow_to_chan(mci, i);
> > - const unsigned rank = i5100_csrow_to_rank(mci, i);
> > + mci_for_each_dimm(mci, dimm) {
> > + const unsigned long npages = i5100_npages(mci, dimm->idx);
> > + const unsigned chan = i5100_csrow_to_chan(mci, dimm->idx);
> > + const unsigned rank = i5100_csrow_to_rank(mci, dimm->idx);
> >
> > if (!npages)
> > continue;
>
> This cannot be right: the code here under this does now:
>
> dimm = edac_get_dimm(mci, chan, rank, 0);
>
> but dimm is already set. It used to get the DIMM using chan and rank but
> now you're iterating over the already initialized pointers. So I think
> you don't need the edac_get_dimm() anymore.
Right, it should calculate to the same pointer we already have and can
be removed. Good catch.
>
> Also fix those up too, while at it pls:
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #235: FILE: drivers/edac/i5100_edac.c:854:
> + const unsigned chan = i5100_csrow_to_chan(mci, dimm->idx);
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #236: FILE: drivers/edac/i5100_edac.c:855:
> + const unsigned rank = i5100_csrow_to_rank(mci, dimm->idx);
I am going to fix this is a separate patch, though I will exclude
rework of struct i5100_priv.
I have reworked the patch according to all your other comments.
Thanks for review.
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 09/24] EDAC, mc: Cleanup _edac_mc_free() code
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (7 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-14 16:31 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 10/24] EDAC, mc: Remove per layer counters Robert Richter
` (15 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Remove needless and boilerplate variable declarations. No functional
changes.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 27277ca46ab3..f2acdab34eb7 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -280,26 +280,23 @@ static void _edac_mc_free(struct mem_ctl_info *mci)
{
int i, chn, row;
struct csrow_info *csr;
- const unsigned int tot_dimms = mci->tot_dimms;
- const unsigned int tot_channels = mci->num_cschannel;
- const unsigned int tot_csrows = mci->nr_csrows;
if (mci->dimms) {
- for (i = 0; i < tot_dimms; i++)
+ for (i = 0; i < mci->tot_dimms; i++)
kfree(mci->dimms[i]);
kfree(mci->dimms);
}
if (mci->csrows) {
- for (row = 0; row < tot_csrows; row++) {
+ for (row = 0; row < mci->nr_csrows; row++) {
csr = mci->csrows[row];
- if (csr) {
- if (csr->channels) {
- for (chn = 0; chn < tot_channels; chn++)
- kfree(csr->channels[chn]);
- kfree(csr->channels);
- }
- kfree(csr);
+ if (!csr)
+ continue;
+ if (csr->channels) {
+ for (chn = 0; chn < mci->num_cschannel; chn++)
+ kfree(csr->channels[chn]);
+ kfree(csr->channels);
}
+ kfree(csr);
}
kfree(mci->csrows);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 09/24] EDAC, mc: Cleanup _edac_mc_free() code
2019-06-24 15:09 ` [PATCH v2 09/24] EDAC, mc: Cleanup _edac_mc_free() code Robert Richter
@ 2019-08-14 16:31 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-14 16:31 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:13PM +0000, Robert Richter wrote:
> Remove needless and boilerplate variable declarations. No functional
> changes.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/edac_mc.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
This can go in now because unrelated cleanup.
Applied,
thanks.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 10/24] EDAC, mc: Remove per layer counters
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (8 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 09/24] EDAC, mc: Cleanup _edac_mc_free() code Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-16 9:24 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 11/24] EDAC, mc: Rework edac_raw_mc_handle_error() to use struct dimm_info Robert Richter
` (14 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Looking at how mci->{ue,ce}_per_layer[EDAC_MAX_LAYERS] is used, it
turns out that only the leaves in the memory hierarchy are consumed
(in sysfs), but not the intermediate layers, e.g.:
count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][dimm->idx];
So let's get rid of the unused counters that just add complexity.
Error counter values are directly stored in struct dimm_info now.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 98 ++++++++++++------------------------
drivers/edac/edac_mc_sysfs.c | 20 +++-----
drivers/edac/ghes_edac.c | 5 +-
include/linux/edac.h | 7 ++-
4 files changed, 44 insertions(+), 86 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index f2acdab34eb7..bce39b2e10c9 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -313,10 +313,9 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
struct csrow_info *csr;
struct rank_info *chan;
struct dimm_info *dimm;
- u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
unsigned pos[EDAC_MAX_LAYERS];
- unsigned size, tot_dimms = 1, count = 1;
- unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
+ unsigned size, tot_dimms = 1;
+ unsigned tot_csrows = 1, tot_channels = 1;
void *pvt, *p, *ptr = NULL;
int idx, i, j, row, chn, n, len;
bool per_rank = false;
@@ -342,19 +341,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
* stringent as what the compiler would provide if we could simply
* hardcode everything into a single struct.
*/
- mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
- layer = edac_align_ptr(&ptr, sizeof(*layer), n_layers);
- for (i = 0; i < n_layers; i++) {
- count *= layers[i].size;
- edac_dbg(4, "errcount layer %d size %d\n", i, count);
- ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
- ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
- tot_errcount += 2 * count;
- }
-
- edac_dbg(4, "allocating %d error counters\n", tot_errcount);
- pvt = edac_align_ptr(&ptr, sz_pvt, 1);
- size = ((unsigned long)pvt) + sz_pvt;
+ mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
+ layer = edac_align_ptr(&ptr, sizeof(*layer), n_layers);
+ pvt = edac_align_ptr(&ptr, sz_pvt, 1);
+ size = ((unsigned long)pvt) + sz_pvt;
edac_dbg(1, "allocating %u bytes for mci data (%d %s, %d csrows/channels)\n",
size,
@@ -370,10 +360,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
* rather than an imaginary chunk of memory located at address 0.
*/
layer = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)layer));
- for (i = 0; i < n_layers; i++) {
- mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
- mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
- }
pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL;
/* setup index and various internal pointers */
@@ -903,53 +889,31 @@ const char *edac_layer_name[] = {
EXPORT_SYMBOL_GPL(edac_layer_name);
static void edac_inc_ce_error(struct mem_ctl_info *mci,
- bool enable_per_layer_report,
const int pos[EDAC_MAX_LAYERS],
const u16 count)
{
- int i, index = 0;
+ struct dimm_info *dimm = edac_get_dimm(mci, pos[0], pos[1], pos[2]);
mci->ce_mc += count;
- if (!enable_per_layer_report) {
+ if (dimm)
+ dimm->ce_count += count;
+ else
mci->ce_noinfo_count += count;
- return;
- }
-
- for (i = 0; i < mci->n_layers; i++) {
- if (pos[i] < 0)
- break;
- index += pos[i];
- mci->ce_per_layer[i][index] += count;
-
- if (i < mci->n_layers - 1)
- index *= mci->layers[i + 1].size;
- }
}
static void edac_inc_ue_error(struct mem_ctl_info *mci,
- bool enable_per_layer_report,
const int pos[EDAC_MAX_LAYERS],
const u16 count)
{
- int i, index = 0;
+ struct dimm_info *dimm = edac_get_dimm(mci, pos[0], pos[1], pos[2]);
mci->ue_mc += count;
- if (!enable_per_layer_report) {
+ if (dimm)
+ dimm->ue_count += count;
+ else
mci->ue_noinfo_count += count;
- return;
- }
-
- for (i = 0; i < mci->n_layers; i++) {
- if (pos[i] < 0)
- break;
- index += pos[i];
- mci->ue_per_layer[i][index] += count;
-
- if (i < mci->n_layers - 1)
- index *= mci->layers[i + 1].size;
- }
}
static void edac_ce_error(struct mem_ctl_info *mci,
@@ -960,7 +924,6 @@ static void edac_ce_error(struct mem_ctl_info *mci,
const char *label,
const char *detail,
const char *other_detail,
- const bool enable_per_layer_report,
const unsigned long page_frame_number,
const unsigned long offset_in_page,
long grain)
@@ -983,7 +946,7 @@ static void edac_ce_error(struct mem_ctl_info *mci,
error_count, msg, msg_aux, label,
location, detail);
}
- edac_inc_ce_error(mci, enable_per_layer_report, pos, error_count);
+ edac_inc_ce_error(mci, pos, error_count);
if (mci->scrub_mode == SCRUB_SW_SRC) {
/*
@@ -1013,8 +976,7 @@ static void edac_ue_error(struct mem_ctl_info *mci,
const char *location,
const char *label,
const char *detail,
- const char *other_detail,
- const bool enable_per_layer_report)
+ const char *other_detail)
{
char *msg_aux = "";
@@ -1043,7 +1005,7 @@ static void edac_ue_error(struct mem_ctl_info *mci,
msg, msg_aux, label, location, detail);
}
- edac_inc_ue_error(mci, enable_per_layer_report, pos, error_count);
+ edac_inc_ue_error(mci, pos, error_count);
}
void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
@@ -1076,16 +1038,16 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
"page:0x%lx offset:0x%lx grain:%ld syndrome:0x%lx",
e->page_frame_number, e->offset_in_page,
e->grain, e->syndrome);
- edac_ce_error(mci, e->error_count, pos, e->msg, e->location, e->label,
- detail, e->other_detail, e->enable_per_layer_report,
+ edac_ce_error(mci, e->error_count, pos, e->msg, e->location,
+ e->label, detail, e->other_detail,
e->page_frame_number, e->offset_in_page, e->grain);
} else {
snprintf(detail, sizeof(detail),
"page:0x%lx offset:0x%lx grain:%ld",
e->page_frame_number, e->offset_in_page, e->grain);
- edac_ue_error(mci, e->error_count, pos, e->msg, e->location, e->label,
- detail, e->other_detail, e->enable_per_layer_report);
+ edac_ue_error(mci, e->error_count, pos, e->msg, e->location,
+ e->label, detail, e->other_detail);
}
@@ -1110,6 +1072,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
int pos[EDAC_MAX_LAYERS] = { top_layer, mid_layer, low_layer };
int i, n_labels = 0;
struct edac_raw_error_desc *e = &mci->error_desc;
+ bool per_layer_report = false;
edac_dbg(3, "MC%d\n", mci->mc_idx);
@@ -1127,9 +1090,9 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
/*
* Check if the event report is consistent and if the memory
- * location is known. If it is known, enable_per_layer_report will be
- * true, the DIMM(s) label info will be filled and the per-layer
- * error counters will be incremented.
+ * location is known. If it is known, the DIMM(s) label info
+ * will be filled and the per-layer error counters will be
+ * incremented.
*/
for (i = 0; i < mci->n_layers; i++) {
if (pos[i] >= (int)mci->layers[i].size) {
@@ -1147,7 +1110,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
pos[i] = -1;
}
if (pos[i] >= 0)
- e->enable_per_layer_report = true;
+ per_layer_report = true;
}
/*
@@ -1176,15 +1139,18 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
if (dimm->grain > e->grain)
e->grain = dimm->grain;
+ if (!per_layer_report)
+ continue;
+
/*
* If the error is memory-controller wide, there's no need to
* seek for the affected DIMMs because the whole
* channel/memory controller/... may be affected.
* Also, don't show errors for empty DIMM slots.
*/
- if (e->enable_per_layer_report && dimm->nr_pages) {
+ if (dimm->nr_pages) {
if (n_labels >= EDAC_MAX_LABELS) {
- e->enable_per_layer_report = false;
+ per_layer_report = false;
break;
}
n_labels++;
@@ -1215,7 +1181,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
}
}
- if (!e->enable_per_layer_report) {
+ if (!per_layer_report) {
strcpy(e->label, "any memory");
} else {
edac_dbg(4, "csrow/channel to increment: (%d,%d)\n", row, chan);
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 4d15c88a52cd..a4c1b8501ff3 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -558,10 +558,8 @@ static ssize_t dimmdev_ce_count_show(struct device *dev,
char *data)
{
struct dimm_info *dimm = to_dimm(dev);
- u32 count;
- count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][dimm->idx];
- return sprintf(data, "%u\n", count);
+ return sprintf(data, "%u\n", dimm->ce_count);
}
static ssize_t dimmdev_ue_count_show(struct device *dev,
@@ -569,10 +567,8 @@ static ssize_t dimmdev_ue_count_show(struct device *dev,
char *data)
{
struct dimm_info *dimm = to_dimm(dev);
- u32 count;
- count = dimm->mci->ue_per_layer[dimm->mci->n_layers-1][dimm->idx];
- return sprintf(data, "%u\n", count);
+ return sprintf(data, "%u\n", dimm->ue_count);
}
/* dimm/rank attribute files */
@@ -660,7 +656,9 @@ static ssize_t mci_reset_counters_store(struct device *dev,
const char *data, size_t count)
{
struct mem_ctl_info *mci = to_mci(dev);
- int cnt, row, chan, i;
+ struct dimm_info *dimm;
+ int row, chan;
+
mci->ue_mc = 0;
mci->ce_mc = 0;
mci->ue_noinfo_count = 0;
@@ -676,11 +674,9 @@ static ssize_t mci_reset_counters_store(struct device *dev,
ri->channels[chan]->ce_count = 0;
}
- cnt = 1;
- for (i = 0; i < mci->n_layers; i++) {
- cnt *= mci->layers[i].size;
- memset(mci->ce_per_layer[i], 0, cnt * sizeof(u32));
- memset(mci->ue_per_layer[i], 0, cnt * sizeof(u32));
+ mci_for_each_dimm(mci, dimm) {
+ dimm->ue_count = 0;
+ dimm->ce_count = 0;
}
mci->start_time = jiffies;
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 72e75ea5526c..757a02f2ce49 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -348,11 +348,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
mem_err->mem_dev_handle);
index = get_dimm_smbios_index(mem_err->mem_dev_handle);
- if (index >= 0) {
+ if (index >= 0)
e->top_layer = index;
- e->enable_per_layer_report = true;
- }
-
}
if (p > e->location)
*(p - 1) = '\0';
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 20a04f48616c..4dcf075e9dff 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -383,6 +383,9 @@ struct dimm_info {
unsigned csrow, cschannel; /* Points to the old API data */
u16 smbios_handle; /* Handle for SMBIOS type 17 */
+
+ u32 ce_count;
+ u32 ue_count;
};
/**
@@ -453,8 +456,6 @@ struct errcount_attribute_data {
* @location: location of the error
* @label: label of the affected DIMM(s)
* @other_detail: other driver-specific detail about the error
- * @enable_per_layer_report: if false, the error affects all layers
- * (typically, a memory controller error)
*/
struct edac_raw_error_desc {
/*
@@ -475,7 +476,6 @@ struct edac_raw_error_desc {
unsigned long syndrome;
const char *msg;
const char *other_detail;
- bool enable_per_layer_report;
};
/* MEMORY controller information structure
@@ -565,7 +565,6 @@ struct mem_ctl_info {
*/
u32 ce_noinfo_count, ue_noinfo_count;
u32 ue_mc, ce_mc;
- u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
struct completion complete;
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 10/24] EDAC, mc: Remove per layer counters
2019-06-24 15:09 ` [PATCH v2 10/24] EDAC, mc: Remove per layer counters Robert Richter
@ 2019-08-16 9:24 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-16 9:24 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:15PM +0000, Robert Richter wrote:
> Looking at how mci->{ue,ce}_per_layer[EDAC_MAX_LAYERS] is used, it
> turns out that only the leaves in the memory hierarchy are consumed
> (in sysfs), but not the intermediate layers, e.g.:
>
> count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][dimm->idx];
>
> So let's get rid of the unused counters that just add complexity.
>
> Error counter values are directly stored in struct dimm_info now.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/edac_mc.c | 98 ++++++++++++------------------------
> drivers/edac/edac_mc_sysfs.c | 20 +++-----
> drivers/edac/ghes_edac.c | 5 +-
> include/linux/edac.h | 7 ++-
> 4 files changed, 44 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index f2acdab34eb7..bce39b2e10c9 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -313,10 +313,9 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
> struct csrow_info *csr;
> struct rank_info *chan;
> struct dimm_info *dimm;
> - u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> unsigned pos[EDAC_MAX_LAYERS];
> - unsigned size, tot_dimms = 1, count = 1;
> - unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
> + unsigned size, tot_dimms = 1;
> + unsigned tot_csrows = 1, tot_channels = 1;
Pls fix those while touching this:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#48: FILE: drivers/edac/edac_mc.c:317:
+ unsigned size, tot_dimms = 1;
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#49: FILE: drivers/edac/edac_mc.c:318:
+ unsigned tot_csrows = 1, tot_channels = 1;
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 11/24] EDAC, mc: Rework edac_raw_mc_handle_error() to use struct dimm_info
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (9 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 10/24] EDAC, mc: Remove per layer counters Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 12/24] EDAC, ghes: Use standard kernel macros for page calculations Robert Richter
` (13 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Rework edac_raw_mc_handle_error() to use struct dimm_info.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 28 +++++++++++++---------------
drivers/edac/edac_mc.h | 2 ++
drivers/edac/ghes_edac.c | 5 ++++-
3 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index bce39b2e10c9..a8d4471e238c 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -889,11 +889,9 @@ const char *edac_layer_name[] = {
EXPORT_SYMBOL_GPL(edac_layer_name);
static void edac_inc_ce_error(struct mem_ctl_info *mci,
- const int pos[EDAC_MAX_LAYERS],
+ struct dimm_info *dimm,
const u16 count)
{
- struct dimm_info *dimm = edac_get_dimm(mci, pos[0], pos[1], pos[2]);
-
mci->ce_mc += count;
if (dimm)
@@ -903,11 +901,9 @@ static void edac_inc_ce_error(struct mem_ctl_info *mci,
}
static void edac_inc_ue_error(struct mem_ctl_info *mci,
- const int pos[EDAC_MAX_LAYERS],
- const u16 count)
+ struct dimm_info *dimm,
+ const u16 count)
{
- struct dimm_info *dimm = edac_get_dimm(mci, pos[0], pos[1], pos[2]);
-
mci->ue_mc += count;
if (dimm)
@@ -917,8 +913,8 @@ static void edac_inc_ue_error(struct mem_ctl_info *mci,
}
static void edac_ce_error(struct mem_ctl_info *mci,
+ struct dimm_info *dimm,
const u16 error_count,
- const int pos[EDAC_MAX_LAYERS],
const char *msg,
const char *location,
const char *label,
@@ -946,7 +942,7 @@ static void edac_ce_error(struct mem_ctl_info *mci,
error_count, msg, msg_aux, label,
location, detail);
}
- edac_inc_ce_error(mci, pos, error_count);
+ edac_inc_ce_error(mci, dimm, error_count);
if (mci->scrub_mode == SCRUB_SW_SRC) {
/*
@@ -970,8 +966,8 @@ static void edac_ce_error(struct mem_ctl_info *mci,
}
static void edac_ue_error(struct mem_ctl_info *mci,
+ struct dimm_info *dimm,
const u16 error_count,
- const int pos[EDAC_MAX_LAYERS],
const char *msg,
const char *location,
const char *label,
@@ -1005,15 +1001,15 @@ static void edac_ue_error(struct mem_ctl_info *mci,
msg, msg_aux, label, location, detail);
}
- edac_inc_ue_error(mci, pos, error_count);
+ edac_inc_ue_error(mci, dimm, error_count);
}
void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
struct mem_ctl_info *mci,
+ struct dimm_info *dimm,
struct edac_raw_error_desc *e)
{
char detail[80];
- int pos[EDAC_MAX_LAYERS] = { e->top_layer, e->mid_layer, e->low_layer };
u8 grain_bits;
/*
@@ -1038,7 +1034,7 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
"page:0x%lx offset:0x%lx grain:%ld syndrome:0x%lx",
e->page_frame_number, e->offset_in_page,
e->grain, e->syndrome);
- edac_ce_error(mci, e->error_count, pos, e->msg, e->location,
+ edac_ce_error(mci, dimm, e->error_count, e->msg, e->location,
e->label, detail, e->other_detail,
e->page_frame_number, e->offset_in_page, e->grain);
} else {
@@ -1046,7 +1042,7 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
"page:0x%lx offset:0x%lx grain:%ld",
e->page_frame_number, e->offset_in_page, e->grain);
- edac_ue_error(mci, e->error_count, pos, e->msg, e->location,
+ edac_ue_error(mci, dimm, e->error_count, e->msg, e->location,
e->label, detail, e->other_detail);
}
@@ -1212,6 +1208,8 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
if (p > e->location)
*(p - 1) = '\0';
- edac_raw_mc_handle_error(type, mci, e);
+ dimm = edac_get_dimm(mci, top_layer, mid_layer, low_layer);
+
+ edac_raw_mc_handle_error(type, mci, dimm, e);
}
EXPORT_SYMBOL_GPL(edac_mc_handle_error);
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 4165e15995ad..b816cf3caaee 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -214,6 +214,7 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
*
* @type: severity of the error (CE/UE/Fatal)
* @mci: a struct mem_ctl_info pointer
+ * @dimm: a struct dimm_info pointer
* @e: error description
*
* This raw function is used internally by edac_mc_handle_error(). It should
@@ -222,6 +223,7 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
*/
void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
struct mem_ctl_info *mci,
+ struct dimm_info *dimm,
struct edac_raw_error_desc *e);
/**
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 757a02f2ce49..786f1b32eee1 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -193,6 +193,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
{
+ struct dimm_info *dimm_info;
enum hw_event_mc_err_type type;
struct edac_raw_error_desc *e;
struct mem_ctl_info *mci;
@@ -431,7 +432,9 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
if (p > pvt->other_detail)
*(p - 1) = '\0';
- edac_raw_mc_handle_error(type, mci, e);
+ dimm_info = edac_get_dimm_by_index(mci, e->top_layer);
+
+ edac_raw_mc_handle_error(type, mci, dimm_info, e);
spin_unlock_irqrestore(&ghes_lock, flags);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 12/24] EDAC, ghes: Use standard kernel macros for page calculations
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (10 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 11/24] EDAC, mc: Rework edac_raw_mc_handle_error() to use struct dimm_info Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-02 17:04 ` James Morse
2019-06-24 15:09 ` [PATCH v2 13/24] EDAC, ghes: Add support for legacy API counters Robert Richter
` (12 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Use standard macros for page calculations.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 786f1b32eee1..746083876b5f 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -311,8 +311,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
/* Error address */
if (mem_err->validation_bits & CPER_MEM_VALID_PA) {
- e->page_frame_number = mem_err->physical_addr >> PAGE_SHIFT;
- e->offset_in_page = mem_err->physical_addr & ~PAGE_MASK;
+ e->page_frame_number = PHYS_PFN(mem_err->physical_addr);
+ e->offset_in_page = offset_in_page(mem_err->physical_addr);
}
/* Error grain */
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 12/24] EDAC, ghes: Use standard kernel macros for page calculations
2019-06-24 15:09 ` [PATCH v2 12/24] EDAC, ghes: Use standard kernel macros for page calculations Robert Richter
@ 2019-08-02 17:04 ` James Morse
2019-08-07 9:52 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: James Morse @ 2019-08-02 17:04 UTC (permalink / raw)
To: Robert Richter
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
Hi Robert,
On 24/06/2019 16:09, Robert Richter wrote:
> Use standard macros for page calculations.
>
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 786f1b32eee1..746083876b5f 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -311,8 +311,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
>
> /* Error address */
> if (mem_err->validation_bits & CPER_MEM_VALID_PA) {
> - e->page_frame_number = mem_err->physical_addr >> PAGE_SHIFT;
> - e->offset_in_page = mem_err->physical_addr & ~PAGE_MASK;
> + e->page_frame_number = PHYS_PFN(mem_err->physical_addr);
> + e->offset_in_page = offset_in_page(mem_err->physical_addr);
> }
>
> /* Error grain */
>
After a shorter game of spot-the-difference:
Reviewed-by: James Morse <james.morse@arm.com>
Previously here:
https://lore.kernel.org/linux-edac/e566fe1d-ed06-53bc-6827-f6dfa32ee485@arm.com/
Please pick up tags when posting a new version.
If you don't do this, its very difficult to convince people to spend time reviewing your
series.
Thanks,
James
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 13/24] EDAC, ghes: Add support for legacy API counters
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (11 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 12/24] EDAC, ghes: Use standard kernel macros for page calculations Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-16 9:55 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 14/24] EDAC, ghes: Rework memory hierarchy detection Robert Richter
` (11 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
The ghes driver is not able yet to count legacy API counters in sysfs,
e.g.:
/sys/devices/system/edac/mc/mc0/csrow2/ce_count
/sys/devices/system/edac/mc/mc0/csrow2/ch0_ce_count
/sys/devices/system/edac/mc/mc0/csrow2/ch1_ce_count
Make counting csrows/channels generic so that the ghes driver can use
it too.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 38 +++++++++++++++++++++-----------------
drivers/edac/edac_mc.h | 7 ++++++-
drivers/edac/ghes_edac.c | 2 +-
3 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index a8d4471e238c..eea09c6acd3e 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -1007,7 +1007,8 @@ static void edac_ue_error(struct mem_ctl_info *mci,
void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
struct mem_ctl_info *mci,
struct dimm_info *dimm,
- struct edac_raw_error_desc *e)
+ struct edac_raw_error_desc *e,
+ int row, int chan)
{
char detail[80];
u8 grain_bits;
@@ -1046,7 +1047,22 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
e->label, detail, e->other_detail);
}
+ /* old API's counters */
+ if (dimm) {
+ row = dimm->csrow;
+ chan = dimm->cschannel;
+ }
+ if (mci->csrows && row >= 0) {
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ mci->csrows[row]->ce_count += e->error_count;
+ if (chan >= 0)
+ mci->csrows[row]->channels[chan]->ce_count += e->error_count;
+ } else {
+ mci->csrows[row]->ue_count += e->error_count;
+ }
+ edac_dbg(4, "csrow/channel to increment: (%d,%d)\n", row, chan);
+ }
}
EXPORT_SYMBOL_GPL(edac_raw_mc_handle_error);
@@ -1177,22 +1193,10 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
}
}
- if (!per_layer_report) {
+ if (!per_layer_report)
strcpy(e->label, "any memory");
- } else {
- edac_dbg(4, "csrow/channel to increment: (%d,%d)\n", row, chan);
- if (p == e->label)
- strcpy(e->label, "unknown memory");
- if (type == HW_EVENT_ERR_CORRECTED) {
- if (row >= 0) {
- mci->csrows[row]->ce_count += error_count;
- if (chan >= 0)
- mci->csrows[row]->channels[chan]->ce_count += error_count;
- }
- } else
- if (row >= 0)
- mci->csrows[row]->ue_count += error_count;
- }
+ else if (!*e->label)
+ strcpy(e->label, "unknown memory");
/* Fill the RAM location data */
p = e->location;
@@ -1210,6 +1214,6 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
dimm = edac_get_dimm(mci, top_layer, mid_layer, low_layer);
- edac_raw_mc_handle_error(type, mci, dimm, e);
+ edac_raw_mc_handle_error(type, mci, dimm, e, row, chan);
}
EXPORT_SYMBOL_GPL(edac_mc_handle_error);
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index b816cf3caaee..c4ddd5c1e24c 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -216,6 +216,10 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
* @mci: a struct mem_ctl_info pointer
* @dimm: a struct dimm_info pointer
* @e: error description
+ * @row: csrow hint if there is no dimm info (<0 if
+ * unknown)
+ * @chan: cschannel hint if there is no dimm info (<0 if
+ * unknown)
*
* This raw function is used internally by edac_mc_handle_error(). It should
* only be called directly when the hardware error come directly from BIOS,
@@ -224,7 +228,8 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
struct mem_ctl_info *mci,
struct dimm_info *dimm,
- struct edac_raw_error_desc *e);
+ struct edac_raw_error_desc *e,
+ int row, int chan);
/**
* edac_mc_handle_error() - Reports a memory event to userspace.
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 746083876b5f..8063996a311d 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -434,7 +434,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
dimm_info = edac_get_dimm_by_index(mci, e->top_layer);
- edac_raw_mc_handle_error(type, mci, dimm_info, e);
+ edac_raw_mc_handle_error(type, mci, dimm_info, e, -1, -1);
spin_unlock_irqrestore(&ghes_lock, flags);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 13/24] EDAC, ghes: Add support for legacy API counters
2019-06-24 15:09 ` [PATCH v2 13/24] EDAC, ghes: Add support for legacy API counters Robert Richter
@ 2019-08-16 9:55 ` Borislav Petkov
2019-08-30 9:35 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: Borislav Petkov @ 2019-08-16 9:55 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:22PM +0000, Robert Richter wrote:
> The ghes driver is not able yet to count legacy API counters in sysfs,
> e.g.:
>
> /sys/devices/system/edac/mc/mc0/csrow2/ce_count
> /sys/devices/system/edac/mc/mc0/csrow2/ch0_ce_count
> /sys/devices/system/edac/mc/mc0/csrow2/ch1_ce_count
>
> Make counting csrows/channels generic so that the ghes driver can use
> it too.
What for?
ghes_edac enumerates the DIMMs from SMBIOS - it doesn't need chip
selects and ranks. Those are used when you can't count the DIMMs
properly...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 13/24] EDAC, ghes: Add support for legacy API counters
2019-08-16 9:55 ` Borislav Petkov
@ 2019-08-30 9:35 ` Robert Richter
0 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-08-30 9:35 UTC (permalink / raw)
To: Borislav Petkov
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 16.08.19 11:55:59, Borislav Petkov wrote:
> On Mon, Jun 24, 2019 at 03:09:22PM +0000, Robert Richter wrote:
> > The ghes driver is not able yet to count legacy API counters in sysfs,
> > e.g.:
> >
> > /sys/devices/system/edac/mc/mc0/csrow2/ce_count
> > /sys/devices/system/edac/mc/mc0/csrow2/ch0_ce_count
> > /sys/devices/system/edac/mc/mc0/csrow2/ch1_ce_count
> >
> > Make counting csrows/channels generic so that the ghes driver can use
> > it too.
>
> What for?
Same was asked here:
https://lore.kernel.org/patchwork/patch/1080277/
Actually it is a fix for the counters exposed by the legacy API for
the ghes driver. Counters are broken (set to zero). The ghes driver is
the only where errors are reported using edac_raw_mc_handle_error()
instead of edac_mc_handle_error(). The fix is to move the error
counting to edac_mc_handle_error() where the other counters are
incremented.
All distributions that I have checked enable the legacy API option
(CONFIG_EDAC_LEGACY_SYSFS=y) and the interface cannot be disabled for
individual drivers. As long as the counters are exposed, their values
should be correct. See all options discussed in the thread from v1.
> ghes_edac enumerates the DIMMs from SMBIOS - it doesn't need chip
> selects and ranks. Those are used when you can't count the DIMMs
> properly...
Right, but that is true also for other drivers (actually all other
drivers since DIMMs are used now). It is to support older tools that
deal with */csrow*/ch* instead of */dimm* in sysfs.
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 14/24] EDAC, ghes: Rework memory hierarchy detection
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (12 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 13/24] EDAC, ghes: Add support for legacy API counters Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-20 8:56 ` Borislav Petkov
2019-06-24 15:09 ` [PATCH v2 15/24] EDAC, ghes: Extract numa node information for each dimm Robert Richter
` (10 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
In a later patch we want to add more information about the memory
hierarchy (NUMA topology, DIMM label information). Rework memory
hierarchy detection to make the code extendable for this.
The general approach is roughly like:
mem_info_setup();
for_each_node(nid) {
mci = edac_mc_alloc(nid);
mem_info_prepare_mci(mci);
edac_mc_add_mc(mci);
};
This patch introduces mem_info_setup() and mem_info_prepare_mci().
All data of the memory hierarchy is collected in a local struct
ghes_mem_info.
Note: Per (NUMA) node registration will be implemented in a later
patch.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 166 ++++++++++++++++++++++++++++++---------
1 file changed, 127 insertions(+), 39 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 8063996a311d..44bfb499b147 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -65,17 +65,53 @@ struct memdev_dmi_entry {
u16 conf_mem_clk_speed;
} __attribute__((__packed__));
-struct ghes_edac_dimm_fill {
- struct mem_ctl_info *mci;
- unsigned count;
+struct ghes_dimm_info {
+ struct dimm_info dimm_info;
+ int idx;
+};
+
+struct ghes_mem_info {
+ int num_dimm;
+ struct ghes_dimm_info *dimms;
};
+static struct ghes_mem_info mem_info;
+
+#define for_each_dimm(dimm) \
+ for (dimm = mem_info.dimms; \
+ dimm < mem_info.dimms + mem_info.num_dimm; \
+ dimm++)
+
static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg)
{
- int *num_dimm = arg;
+ int *num = arg;
if (dh->type == DMI_ENTRY_MEM_DEVICE)
- (*num_dimm)++;
+ (*num)++;
+}
+
+static int ghes_dimm_info_init(int num)
+{
+ struct ghes_dimm_info *dimm;
+ int idx = 0;
+
+ memset(&mem_info, 0, sizeof(mem_info));
+
+ if (num <= 0)
+ return -EINVAL;
+
+ mem_info.dimms = kcalloc(num, sizeof(*mem_info.dimms), GFP_KERNEL);
+ if (!mem_info.dimms)
+ return -ENOMEM;
+
+ mem_info.num_dimm = num;
+
+ for_each_dimm(dimm) {
+ dimm->idx = idx;
+ idx++;
+ }
+
+ return 0;
}
static int get_dimm_smbios_index(u16 handle)
@@ -92,18 +128,15 @@ static int get_dimm_smbios_index(u16 handle)
static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
{
- struct ghes_edac_dimm_fill *dimm_fill = arg;
- struct mem_ctl_info *mci = dimm_fill->mci;
-
if (dh->type == DMI_ENTRY_MEM_DEVICE) {
+ int *idx = arg;
struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh;
- struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count,
- 0, 0);
+ struct ghes_dimm_info *mi = &mem_info.dimms[*idx];
+ struct dimm_info *dimm = &mi->dimm_info;
u16 rdr_mask = BIT(7) | BIT(13);
if (entry->size == 0xffff) {
- pr_info("Can't get DIMM%i size\n",
- dimm_fill->count);
+ pr_info("Can't get DIMM%i size\n", mi->idx);
dimm->nr_pages = MiB_TO_PAGES(32);/* Unknown */
} else if (entry->size == 0x7fff) {
dimm->nr_pages = MiB_TO_PAGES(entry->extended_size);
@@ -177,7 +210,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
if (dimm->nr_pages) {
edac_dbg(1, "DIMM%i: %s size = %d MB%s\n",
- dimm_fill->count, edac_mem_types[dimm->mtype],
+ mi->idx, edac_mem_types[dimm->mtype],
PAGES_TO_MiB(dimm->nr_pages),
(dimm->edac_mode != EDAC_NONE) ? "(ECC)" : "");
edac_dbg(2, "\ttype %d, detail 0x%02x, width %d(total %d)\n",
@@ -187,10 +220,74 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
dimm->smbios_handle = entry->handle;
- dimm_fill->count++;
+ (*idx)++;
}
}
+static int mem_info_setup(void)
+{
+ int num = 0;
+ int idx = 0;
+ int ret;
+
+ /* Get the number of DIMMs */
+ dmi_walk(ghes_edac_count_dimms, &num);
+
+ ret = ghes_dimm_info_init(num);
+ if (ret)
+ return ret;
+
+ dmi_walk(ghes_edac_dmidecode, &idx);
+
+ return 0;
+}
+
+static int mem_info_setup_fake(void)
+{
+ struct dimm_info *dimm;
+ int ret;
+
+ ret = ghes_dimm_info_init(1);
+ if (ret)
+ return ret;
+
+ dimm = &mem_info.dimms->dimm_info;
+ dimm->nr_pages = 1;
+ dimm->grain = 128;
+ dimm->mtype = MEM_UNKNOWN;
+ dimm->dtype = DEV_UNKNOWN;
+ dimm->edac_mode = EDAC_SECDED;
+
+ return 0;
+}
+
+static void mem_info_prepare_mci(struct mem_ctl_info *mci)
+{
+ struct dimm_info *mci_dimm, *dmi_dimm;
+ struct ghes_dimm_info *dimm;
+ int index = 0;
+
+ for_each_dimm(dimm) {
+ dmi_dimm = &dimm->dimm_info;
+ mci_dimm = edac_get_dimm_by_index(mci, index);
+
+ index++;
+ if (index > mci->tot_dimms)
+ break;
+
+ mci_dimm->nr_pages = dmi_dimm->nr_pages;
+ mci_dimm->mtype = dmi_dimm->mtype;
+ mci_dimm->edac_mode = dmi_dimm->edac_mode;
+ mci_dimm->dtype = dmi_dimm->dtype;
+ mci_dimm->grain = dmi_dimm->grain;
+ mci_dimm->smbios_handle = dmi_dimm->smbios_handle;
+ }
+
+ if (index != mci->tot_dimms)
+ pr_warn("Unexpected number of DIMMs: %d (exp. %d)\n",
+ index, mci->tot_dimms);
+}
+
void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
{
struct dimm_info *dimm_info;
@@ -450,10 +547,9 @@ static struct acpi_platform_list plat_list[] = {
int ghes_edac_register(struct ghes *ghes, struct device *dev)
{
bool fake = false;
- int rc, num_dimm = 0;
+ int rc;
struct mem_ctl_info *mci;
struct edac_mc_layer layers[1];
- struct ghes_edac_dimm_fill dimm_fill;
int idx = -1;
if (IS_ENABLED(CONFIG_X86)) {
@@ -471,22 +567,24 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
if (atomic_inc_return(&ghes_init) > 1)
return 0;
- /* Get the number of DIMMs */
- dmi_walk(ghes_edac_count_dimms, &num_dimm);
-
- /* Check if we've got a bogus BIOS */
- if (num_dimm == 0) {
+ rc = mem_info_setup();
+ if (rc == -EINVAL) {
+ /* we've got a bogus BIOS */
fake = true;
- num_dimm = 1;
+ rc = mem_info_setup_fake();
+ }
+ if (rc < 0) {
+ pr_err("Can't allocate memory for DIMM data\n");
+ return rc;
}
layers[0].type = EDAC_MC_LAYER_ALL_MEM;
- layers[0].size = num_dimm;
+ layers[0].size = mem_info.num_dimm;
layers[0].is_virt_csrow = true;
mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
if (!mci) {
- pr_info("Can't allocate memory for EDAC data\n");
+ pr_err("Can't allocate memory for EDAC data\n");
return -ENOMEM;
}
@@ -512,26 +610,14 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
pr_info("If you find incorrect reports, please contact your hardware vendor\n");
pr_info("to correct its BIOS.\n");
- pr_info("This system has %d DIMM sockets.\n", num_dimm);
+ pr_info("This system has %d DIMM sockets.\n", mem_info.num_dimm);
}
- if (!fake) {
- dimm_fill.count = 0;
- dimm_fill.mci = mci;
- dmi_walk(ghes_edac_dmidecode, &dimm_fill);
- } else {
- struct dimm_info *dimm = edac_get_dimm(mci, 0, 0, 0);
-
- dimm->nr_pages = 1;
- dimm->grain = 128;
- dimm->mtype = MEM_UNKNOWN;
- dimm->dtype = DEV_UNKNOWN;
- dimm->edac_mode = EDAC_SECDED;
- }
+ mem_info_prepare_mci(mci);
rc = edac_mc_add_mc(mci);
if (rc < 0) {
- pr_info("Can't register at EDAC core\n");
+ pr_err("Can't register at EDAC core\n");
edac_mc_free(mci);
return -ENODEV;
}
@@ -548,4 +634,6 @@ void ghes_edac_unregister(struct ghes *ghes)
mci = ghes_pvt->mci;
edac_mc_del_mc(mci->pdev);
edac_mc_free(mci);
+
+ kfree(mem_info.dimms);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 14/24] EDAC, ghes: Rework memory hierarchy detection
2019-06-24 15:09 ` [PATCH v2 14/24] EDAC, ghes: Rework memory hierarchy detection Robert Richter
@ 2019-08-20 8:56 ` Borislav Petkov
0 siblings, 0 replies; 52+ messages in thread
From: Borislav Petkov @ 2019-08-20 8:56 UTC (permalink / raw)
To: Robert Richter
Cc: James Morse, Mauro Carvalho Chehab, linux-edac, linux-kernel
On Mon, Jun 24, 2019 at 03:09:24PM +0000, Robert Richter wrote:
> In a later patch we want to add more information about the memory
> hierarchy (NUMA topology, DIMM label information). Rework memory
> hierarchy detection to make the code extendable for this.
>
> The general approach is roughly like:
>
> mem_info_setup();
> for_each_node(nid) {
> mci = edac_mc_alloc(nid);
> mem_info_prepare_mci(mci);
> edac_mc_add_mc(mci);
> };
>
> This patch introduces mem_info_setup() and mem_info_prepare_mci().
Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.
> All data of the memory hierarchy is collected in a local struct
> ghes_mem_info.
>
> Note: Per (NUMA) node registration will be implemented in a later
> patch.
That sentence is not needed in the commit message.
> Signed-off-by: Robert Richter <rrichter@marvell.com>
> ---
> drivers/edac/ghes_edac.c | 166 ++++++++++++++++++++++++++++++---------
> 1 file changed, 127 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 8063996a311d..44bfb499b147 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -65,17 +65,53 @@ struct memdev_dmi_entry {
> u16 conf_mem_clk_speed;
> } __attribute__((__packed__));
>
> -struct ghes_edac_dimm_fill {
> - struct mem_ctl_info *mci;
> - unsigned count;
All those "dimm" and "info" words everywhere are making my head spin.
Let's make it more readable:
> +struct ghes_dimm_info {
> + struct dimm_info dimm_info;
> + int idx;
> +};
> +
> +struct ghes_mem_info {
> + int num_dimm;
> + struct ghes_dimm_info *dimms;
> };
>
> +static struct ghes_mem_info mem_info;
/* A DIMM */
struct ghes_dimm {
struct dimm_info dimm;
int idx;
};
/* The memory layout of the system */
struct ghes_memory {
struct ghes_dimm *dimms;
int num_dimms;
};
static struct ghes_memory mem;
> +
> +#define for_each_dimm(dimm) \
> + for (dimm = mem_info.dimms; \
> + dimm < mem_info.dimms + mem_info.num_dimm; \
> + dimm++)
> +
> static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg)
> {
> - int *num_dimm = arg;
> + int *num = arg;
>
> if (dh->type == DMI_ENTRY_MEM_DEVICE)
> - (*num_dimm)++;
> + (*num)++;
> +}
> +
> +static int ghes_dimm_info_init(int num)
ghes_dimm_init()
... you get the idea - let's drop the _info crap.
> +{
> + struct ghes_dimm_info *dimm;
> + int idx = 0;
> +
> + memset(&mem_info, 0, sizeof(mem_info));
> +
> + if (num <= 0)
> + return -EINVAL;
Move that check into the caller mem_info_setup() so that you don't do
the memset unnecessarily.
> +
> + mem_info.dimms = kcalloc(num, sizeof(*mem_info.dimms), GFP_KERNEL);
> + if (!mem_info.dimms)
> + return -ENOMEM;
> +
> + mem_info.num_dimm = num;
> +
> + for_each_dimm(dimm) {
> + dimm->idx = idx;
> + idx++;
> + }
or simply
for_each_dimm(dimm)
dimm->idx = idx++;
> +
> + return 0;
> }
>
> static int get_dimm_smbios_index(u16 handle)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 15/24] EDAC, ghes: Extract numa node information for each dimm
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (13 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 14/24] EDAC, ghes: Rework memory hierarchy detection Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-08-02 17:05 ` James Morse
2019-06-24 15:09 ` [PATCH v2 16/24] EDAC, ghes: Moving code around ghes_edac_register() Robert Richter
` (9 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
In a later patch we want to have one mc device per node. This patch
extracts the numa node information for each dimm. This is done by
collecting the physical address ranges from the DMI table (Memory
Array Mapped Address - Type 19 of SMBIOS spec). The node information
for a physical address is already know to a numa aware system (e.g. by
using the ACPI _PXM method or the ACPI SRAT table), so based on the PA
we can assign the node id to the dimms.
A fallback that disables numa is implemented in case the node
information is inconsistent.
E.g., on a ThunderX2 system the following node mappings are found
based on the DMI table:
EDAC DEBUG: mem_info_setup: DIMM0: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM1: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM2: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM3: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM4: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM5: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM6: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM7: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0
EDAC DEBUG: mem_info_setup: DIMM8: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM9: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM10: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM11: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM12: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM13: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM14: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
EDAC DEBUG: mem_info_setup: DIMM15: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 98 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 1 deletion(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 44bfb499b147..793362bea044 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -65,14 +65,32 @@ struct memdev_dmi_entry {
u16 conf_mem_clk_speed;
} __attribute__((__packed__));
+/* Memory Array Mapped Address - Type 19 of SMBIOS spec */
+struct memarr_dmi_entry {
+ u8 type;
+ u8 length;
+ u16 handle;
+ u32 start;
+ u32 end;
+ u16 phys_mem_array_handle;
+ u8 partition_width;
+ u64 ext_start;
+ u64 ext_end;
+} __attribute__((__packed__));
+
struct ghes_dimm_info {
struct dimm_info dimm_info;
int idx;
+ int numa_node;
+ phys_addr_t start;
+ phys_addr_t end;
+ u16 phys_handle;
};
struct ghes_mem_info {
- int num_dimm;
+ int num_dimm;
struct ghes_dimm_info *dimms;
+ int dimms_per_node[MAX_NUMNODES];
};
static struct ghes_mem_info mem_info;
@@ -108,12 +126,52 @@ static int ghes_dimm_info_init(int num)
for_each_dimm(dimm) {
dimm->idx = idx;
+ dimm->numa_node = NUMA_NO_NODE;
idx++;
}
return 0;
}
+static void ghes_edac_set_nid(const struct dmi_header *dh, void *arg)
+{
+ struct memarr_dmi_entry *entry = (struct memarr_dmi_entry *)dh;
+ struct ghes_dimm_info *dimm;
+ phys_addr_t start, end;
+ int nid;
+
+ if (dh->type != DMI_ENTRY_MEM_ARRAY_MAPPED_ADDR)
+ return;
+
+ /* only support SMBIOS 2.7+ */
+ if (entry->length < sizeof(*entry))
+ return;
+
+ if (entry->start == 0xffffffff)
+ start = entry->ext_start;
+ else
+ start = entry->start;
+ if (entry->end == 0xffffffff)
+ end = entry->ext_end;
+ else
+ end = entry->end;
+
+ if (!pfn_valid(PHYS_PFN(start)))
+ return;
+
+ nid = pfn_to_nid(PHYS_PFN(start));
+ if (nid < 0 || nid >= MAX_NUMNODES || !node_possible(nid))
+ nid = NUMA_NO_NODE;
+
+ for_each_dimm(dimm) {
+ if (entry->phys_mem_array_handle == dimm->phys_handle) {
+ dimm->numa_node = nid;
+ dimm->start = start;
+ dimm->end = end;
+ }
+ }
+}
+
static int get_dimm_smbios_index(u16 handle)
{
struct mem_ctl_info *mci = ghes_pvt->mci;
@@ -135,6 +193,8 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
struct dimm_info *dimm = &mi->dimm_info;
u16 rdr_mask = BIT(7) | BIT(13);
+ mi->phys_handle = entry->phys_mem_array_handle;
+
if (entry->size == 0xffff) {
pr_info("Can't get DIMM%i size\n", mi->idx);
dimm->nr_pages = MiB_TO_PAGES(32);/* Unknown */
@@ -224,8 +284,23 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
}
}
+static void mem_info_disable_numa(void)
+{
+ struct ghes_dimm_info *dimm;
+
+ for_each_dimm(dimm) {
+ if (dimm->numa_node != NUMA_NO_NODE)
+ mem_info.dimms_per_node[dimm->numa_node] = 0;
+ dimm->numa_node = 0;
+ }
+
+ mem_info.dimms_per_node[0] = mem_info.num_dimm;
+}
+
static int mem_info_setup(void)
{
+ struct ghes_dimm_info *dimm;
+ bool enable_numa = true;
int num = 0;
int idx = 0;
int ret;
@@ -238,6 +313,25 @@ static int mem_info_setup(void)
return ret;
dmi_walk(ghes_edac_dmidecode, &idx);
+ dmi_walk(ghes_edac_set_nid, NULL);
+
+ for_each_dimm(dimm) {
+ if (dimm->numa_node == NUMA_NO_NODE)
+ enable_numa = false;
+ else
+ mem_info.dimms_per_node[dimm->numa_node]++;
+
+ edac_dbg(1, "DIMM%i: Found mem range [%pa-%pa] on node %d\n",
+ dimm->idx, &dimm->start, &dimm->end, dimm->numa_node);
+ }
+
+ if (enable_numa)
+ return 0;
+
+ /* something went wrong, disable numa */
+ if (num_possible_nodes() > 1)
+ pr_warn("Can't get numa info, disabling numa\n");
+ mem_info_disable_numa();
return 0;
}
@@ -258,6 +352,8 @@ static int mem_info_setup_fake(void)
dimm->dtype = DEV_UNKNOWN;
dimm->edac_mode = EDAC_SECDED;
+ mem_info_disable_numa();
+
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 15/24] EDAC, ghes: Extract numa node information for each dimm
2019-06-24 15:09 ` [PATCH v2 15/24] EDAC, ghes: Extract numa node information for each dimm Robert Richter
@ 2019-08-02 17:05 ` James Morse
2019-08-09 13:09 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: James Morse @ 2019-08-02 17:05 UTC (permalink / raw)
To: Robert Richter
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
Hi Robert,
On 24/06/2019 16:09, Robert Richter wrote:
> In a later patch we want to have one mc device per node. This patch
> extracts the numa node information for each dimm. This is done by
> collecting the physical address ranges from the DMI table (Memory
> Array Mapped Address - Type 19 of SMBIOS spec). The node information
> for a physical address is already know to a numa aware system (e.g. by
> using the ACPI _PXM method or the ACPI SRAT table), so based on the PA
> we can assign the node id to the dimms.
I really don't like the way this depends on the rest of the kernel's NUMA support.
mm's policies around the placement of data change with these settings, that shouldn't
matter here. Reporting physical errors shouldn't be influenced by mm's placement policy.
pfn_valid() is a sore subject on arm64, it will return false for random pages that the
firmware is using, or wrote data to with unusual memory attributes. Depending on it makes
this code fragile...
> A fallback that disables numa is implemented in case the node
> information is inconsistent.
... which is why you need a fallback.
All this makes it difficult to explain why this things view of memory is as it is.
Making the RAS/edac code unpredictable like this is a hard sell.
You need to squint through Kconfig, SRAT and the UEFI memory map.
(due to pfn_valid(): the behaviour here can change over a reboot)
Can we 'just' use the type-16 handle to group the DIMMs?
As an illustration:
http://www.linux-arm.org/git?p=linux-jm.git;a=shortlog;h=refs/heads/edac_ghes_2level_dimms/v0
This reflects the SMBIOS tables on my NUMA desktop, and doesn't depend on any of the
above. I'd be interested to know what is missing from this approach.
(which numa node? I don't think we need to know the mapping of mcX<->nid up front. We can
find it from the faulting physical address when we get an error report).
N.B, your mail is still arriving base64 encoded. It looks like this:
https://lore.kernel.org/linux-edac/20190624150758.6695-16-rrichter@marvell.com/raw
Lei Wang found:
> Ah I found if without explicit "--transfer-encoding=7bit" when do "git
> send-mail", my ubuntu box sent out base64 by default.
(but his mail didn't get archived for some reason)
Thanks,
James
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 15/24] EDAC, ghes: Extract numa node information for each dimm
2019-08-02 17:05 ` James Morse
@ 2019-08-09 13:09 ` Robert Richter
0 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-08-09 13:09 UTC (permalink / raw)
To: James Morse
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
Hi James,
On 02.08.19 18:05:07, James Morse wrote:
> On 24/06/2019 16:09, Robert Richter wrote:
> > In a later patch we want to have one mc device per node. This patch
> > extracts the numa node information for each dimm. This is done by
> > collecting the physical address ranges from the DMI table (Memory
> > Array Mapped Address - Type 19 of SMBIOS spec). The node information
> > for a physical address is already know to a numa aware system (e.g. by
> > using the ACPI _PXM method or the ACPI SRAT table), so based on the PA
> > we can assign the node id to the dimms.
>
> I really don't like the way this depends on the rest of the kernel's NUMA support.
> mm's policies around the placement of data change with these settings, that shouldn't
> matter here. Reporting physical errors shouldn't be influenced by mm's placement policy.
>
> pfn_valid() is a sore subject on arm64, it will return false for random pages that the
> firmware is using, or wrote data to with unusual memory attributes. Depending on it makes
> this code fragile...
>
>
> > A fallback that disables numa is implemented in case the node
> > information is inconsistent.
>
> ... which is why you need a fallback.
I don't agree here. pfn_valid() and page_to_nid() are reliable used in
numa systems to identify the node of a physical address. Same is used
here. If firmware does not provide consistent topology data, numa
would not work either and a non-numa fallback is in place. You say
this code is fragile, which would mean numa code is fragile too, but
it isn't.
Node information and the 1:1 mapping between node and an edac mc
device are essential for error handling. All other drivers have one mc
device per node. If you don't follow this topology layout you will
have significant differences in the ghes error handling compared to
other drivers. But this driver (and arm64 systems) should provide a
similar functionality.
> All this makes it difficult to explain why this things view of memory is as it is.
> Making the RAS/edac code unpredictable like this is a hard sell.
>
> You need to squint through Kconfig, SRAT and the UEFI memory map.
> (due to pfn_valid(): the behaviour here can change over a reboot)
>
>
> Can we 'just' use the type-16 handle to group the DIMMs?
>
> As an illustration:
> http://www.linux-arm.org/git?p=linux-jm.git;a=shortlog;h=refs/heads/edac_ghes_2level_dimms/v0
I have looked into your code. You group all dimms under md0 and have
an additional layer for the phys mem arrays. This ignores the cper
layers (node, card, module). The way you add the layer may cause the
creation of dimm entries under md0 that even do not exist in the dmi
table. But dimms and their labels created by edac should reflect the
system as described in the dmi table.
My code creates one mdX device per node and groups the dimms under
them according to the dmi table. For this it further parses the
physical address range of the memory arrays and extracts the numa node
from it. I don't see what is wrong with that. The only added
dependency is the node lookup which is used somewhere else in the
kernel anyway on numa systems. But it provides a much better grouping
of hardware errors, which is then similar to other drivers.
I think your concern is more about code complexity, so I will go
through my code and keep it as simple as possible.
> This reflects the SMBIOS tables on my NUMA desktop, and doesn't depend on any of the
> above. I'd be interested to know what is missing from this approach.
>
> (which numa node? I don't think we need to know the mapping of mcX<->nid up front. We can
> find it from the faulting physical address when we get an error report).
>
>
> N.B, your mail is still arriving base64 encoded. It looks like this:
> https://lore.kernel.org/linux-edac/20190624150758.6695-16-rrichter@marvell.com/raw
>
> Lei Wang found:
> > Ah I found if without explicit "--transfer-encoding=7bit" when do "git
> > send-mail", my ubuntu box sent out base64 by default.
>
> (but his mail didn't get archived for some reason)
Thanks for the note, I will check the encoding.
Thank you for review.
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 16/24] EDAC, ghes: Moving code around ghes_edac_register()
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (14 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 15/24] EDAC, ghes: Extract numa node information for each dimm Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 17/24] EDAC, ghes: Create one memory controller device per node Robert Richter
` (8 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
This is in preparation of the next patch to make the changes there
more visible.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 97 ++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 44 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 793362bea044..13b74368ad81 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -640,45 +640,19 @@ static struct acpi_platform_list plat_list[] = {
{ } /* End */
};
-int ghes_edac_register(struct ghes *ghes, struct device *dev)
+static int
+ghes_edac_register_one(int nid, struct ghes *ghes, struct device *parent)
{
- bool fake = false;
int rc;
struct mem_ctl_info *mci;
struct edac_mc_layer layers[1];
- int idx = -1;
-
- if (IS_ENABLED(CONFIG_X86)) {
- /* Check if safe to enable on this system */
- idx = acpi_match_platform_list(plat_list);
- if (!force_load && idx < 0)
- return -ENODEV;
- } else {
- idx = 0;
- }
-
- /*
- * We have only one logical memory controller to which all DIMMs belong.
- */
- if (atomic_inc_return(&ghes_init) > 1)
- return 0;
-
- rc = mem_info_setup();
- if (rc == -EINVAL) {
- /* we've got a bogus BIOS */
- fake = true;
- rc = mem_info_setup_fake();
- }
- if (rc < 0) {
- pr_err("Can't allocate memory for DIMM data\n");
- return rc;
- }
layers[0].type = EDAC_MC_LAYER_ALL_MEM;
layers[0].size = mem_info.num_dimm;
layers[0].is_virt_csrow = true;
- mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
+ mci = edac_mc_alloc(nid, ARRAY_SIZE(layers), layers,
+ sizeof(struct ghes_edac_pvt));
if (!mci) {
pr_err("Can't allocate memory for EDAC data\n");
return -ENOMEM;
@@ -688,7 +662,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
ghes_pvt->ghes = ghes;
ghes_pvt->mci = mci;
- mci->pdev = dev;
+ mci->pdev = parent;
mci->mtype_cap = MEM_FLAG_EMPTY;
mci->edac_ctl_cap = EDAC_FLAG_NONE;
mci->edac_cap = EDAC_FLAG_NONE;
@@ -696,19 +670,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
mci->ctl_name = "ghes_edac";
mci->dev_name = "ghes";
- if (fake) {
- pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
- pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
- pr_info("work on such system. Use this driver with caution\n");
- } else if (idx < 0) {
- pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
- pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
- pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
- pr_info("If you find incorrect reports, please contact your hardware vendor\n");
- pr_info("to correct its BIOS.\n");
- pr_info("This system has %d DIMM sockets.\n", mem_info.num_dimm);
- }
-
mem_info_prepare_mci(mci);
rc = edac_mc_add_mc(mci);
@@ -733,3 +694,51 @@ void ghes_edac_unregister(struct ghes *ghes)
kfree(mem_info.dimms);
}
+
+int ghes_edac_register(struct ghes *ghes, struct device *dev)
+{
+ bool fake = false;
+ int rc;
+ int idx = -1;
+
+ if (IS_ENABLED(CONFIG_X86)) {
+ /* Check if safe to enable on this system */
+ idx = acpi_match_platform_list(plat_list);
+ if (!force_load && idx < 0)
+ return -ENODEV;
+ } else {
+ idx = 0;
+ }
+
+ /* We have only one ghes instance at a time. */
+ if (atomic_inc_return(&ghes_init) > 1)
+ return 0;
+
+ rc = mem_info_setup();
+ if (rc == -EINVAL) {
+ /* we've got a bogus BIOS */
+ fake = true;
+ rc = mem_info_setup_fake();
+ }
+ if (rc < 0) {
+ pr_err("Can't allocate memory for DIMM data\n");
+ return rc;
+ }
+
+ if (fake) {
+ pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
+ pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
+ pr_info("work on such system. Use this driver with caution\n");
+ } else if (idx < 0) {
+ pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
+ pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
+ pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
+ pr_info("If you find incorrect reports, please contact your hardware vendor\n");
+ pr_info("to correct its BIOS.\n");
+ pr_info("This system has %d DIMM sockets.\n", mem_info.num_dimm);
+ }
+
+ rc = ghes_edac_register_one(0, ghes, dev);
+
+ return rc;
+}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 17/24] EDAC, ghes: Create one memory controller device per node
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (15 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 16/24] EDAC, ghes: Moving code around ghes_edac_register() Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 18/24] EDAC, ghes: Fill sysfs with the DMI DIMM label information Robert Richter
` (7 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Typically for most systems, there is one edac memory controller device
per node. This patch implements the same for the ghes driver. Now,
create multiple mc devices and map the dimms based on the node id.
We need at least one node that is used as fallback if no node
information is available in the error report.
Here a complete and consistent error report from a ThunderX2 system
(zero counter values dropped):
# find /sys/devices/system/edac/mc/ -name \*count | sort -V | xargs grep . | sed -e '/:0/d'
/sys/devices/system/edac/mc/mc0/ce_count:11
/sys/devices/system/edac/mc/mc0/ce_noinfo_count:1
/sys/devices/system/edac/mc/mc0/csrow2/ce_count:5
/sys/devices/system/edac/mc/mc0/csrow2/ch0_ce_count:5
/sys/devices/system/edac/mc/mc0/csrow3/ce_count:3
/sys/devices/system/edac/mc/mc0/csrow3/ch0_ce_count:3
/sys/devices/system/edac/mc/mc0/csrow4/ce_count:2
/sys/devices/system/edac/mc/mc0/csrow4/ch0_ce_count:2
/sys/devices/system/edac/mc/mc0/dimm2/dimm_ce_count:5
/sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count:3
/sys/devices/system/edac/mc/mc0/dimm4/dimm_ce_count:2
/sys/devices/system/edac/mc/mc1/ce_count:7
/sys/devices/system/edac/mc/mc1/csrow2/ce_count:4
/sys/devices/system/edac/mc/mc1/csrow2/ch0_ce_count:4
/sys/devices/system/edac/mc/mc1/csrow3/ce_count:1
/sys/devices/system/edac/mc/mc1/csrow3/ch0_ce_count:1
/sys/devices/system/edac/mc/mc1/csrow6/ce_count:2
/sys/devices/system/edac/mc/mc1/csrow6/ch0_ce_count:2
/sys/devices/system/edac/mc/mc1/dimm2/dimm_ce_count:4
/sys/devices/system/edac/mc/mc1/dimm3/dimm_ce_count:1
/sys/devices/system/edac/mc/mc1/dimm6/dimm_ce_count:2
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 126 ++++++++++++++++++++++++++++++++-------
1 file changed, 104 insertions(+), 22 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 13b74368ad81..63de11654649 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -16,6 +16,7 @@
#include <ras/ras_event.h>
struct ghes_edac_pvt {
+ struct device dev;
struct list_head list;
struct ghes *ghes;
struct mem_ctl_info *mci;
@@ -26,7 +27,7 @@ struct ghes_edac_pvt {
};
static atomic_t ghes_init = ATOMIC_INIT(0);
-static struct ghes_edac_pvt *ghes_pvt;
+struct mem_ctl_info *fallback;
/*
* Sync with other, potentially concurrent callers of
@@ -172,15 +173,15 @@ static void ghes_edac_set_nid(const struct dmi_header *dh, void *arg)
}
}
-static int get_dimm_smbios_index(u16 handle)
+static int get_dimm_smbios_index(struct mem_ctl_info *mci, u16 handle)
{
- struct mem_ctl_info *mci = ghes_pvt->mci;
struct dimm_info *dimm;
mci_for_each_dimm(mci, dimm) {
if (dimm->smbios_handle == handle)
return dimm->idx;
}
+
return -1;
}
@@ -364,6 +365,9 @@ static void mem_info_prepare_mci(struct mem_ctl_info *mci)
int index = 0;
for_each_dimm(dimm) {
+ if (mci->mc_idx != dimm->numa_node)
+ continue;
+
dmi_dimm = &dimm->dimm_info;
mci_dimm = edac_get_dimm_by_index(mci, index);
@@ -384,17 +388,35 @@ static void mem_info_prepare_mci(struct mem_ctl_info *mci)
index, mci->tot_dimms);
}
+static struct mem_ctl_info *get_mc_by_node(int nid)
+{
+ struct mem_ctl_info *mci = edac_mc_find(nid);
+
+ if (mci)
+ return mci;
+
+ if (num_possible_nodes() > 1) {
+ edac_mc_printk(fallback, KERN_WARNING,
+ "Invalid or no node information, falling back to first node: %s",
+ fallback->dev_name);
+ }
+
+ return fallback;
+}
+
void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
{
struct dimm_info *dimm_info;
enum hw_event_mc_err_type type;
struct edac_raw_error_desc *e;
struct mem_ctl_info *mci;
- struct ghes_edac_pvt *pvt = ghes_pvt;
+ struct ghes_edac_pvt *pvt;
unsigned long flags;
char *p;
+ int nid = NUMA_NO_NODE;
- if (!pvt)
+ /* We need at least one mc */
+ if (WARN_ON_ONCE(!fallback))
return;
/*
@@ -407,7 +429,11 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
spin_lock_irqsave(&ghes_lock, flags);
- mci = pvt->mci;
+ /* select the node's mc device */
+ if (mem_err->validation_bits & CPER_MEM_VALID_NODE)
+ nid = mem_err->node;
+ mci = get_mc_by_node(nid);
+ pvt = mci->pvt_info;
e = &mci->error_desc;
/* Cleans the error report buffer */
@@ -541,7 +567,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
p += sprintf(p, "DIMM DMI handle: 0x%.4x ",
mem_err->mem_dev_handle);
- index = get_dimm_smbios_index(mem_err->mem_dev_handle);
+ index = get_dimm_smbios_index(mci, mem_err->mem_dev_handle);
if (index >= 0)
e->top_layer = index;
}
@@ -640,15 +666,29 @@ static struct acpi_platform_list plat_list[] = {
{ } /* End */
};
+void ghes_edac_release(struct device *dev)
+{
+ struct ghes_edac_pvt *ghes_pvt;
+ struct mem_ctl_info *mci;
+
+ ghes_pvt = container_of(dev, struct ghes_edac_pvt, dev);
+
+ mci = ghes_pvt->mci;
+ edac_mc_del_mc(mci->pdev);
+ edac_mc_free(mci);
+}
+
static int
ghes_edac_register_one(int nid, struct ghes *ghes, struct device *parent)
{
+ struct device *dev;
+ struct ghes_edac_pvt *ghes_pvt;
int rc;
struct mem_ctl_info *mci;
struct edac_mc_layer layers[1];
layers[0].type = EDAC_MC_LAYER_ALL_MEM;
- layers[0].size = mem_info.num_dimm;
+ layers[0].size = mem_info.dimms_per_node[nid];
layers[0].is_virt_csrow = true;
mci = edac_mc_alloc(nid, ARRAY_SIZE(layers), layers,
@@ -662,43 +702,69 @@ ghes_edac_register_one(int nid, struct ghes *ghes, struct device *parent)
ghes_pvt->ghes = ghes;
ghes_pvt->mci = mci;
- mci->pdev = parent;
+ dev = &ghes_pvt->dev;
+ dev->parent = parent;
+ dev->release = ghes_edac_release;
+ dev_set_name(dev, "ghes_mc%d", nid);
+
+ rc = device_register(dev);
+ if (rc) {
+ pr_err("Can't create EDAC device (%d)\n", rc);
+ goto fail;
+ }
+
+ mci->pdev = dev;
mci->mtype_cap = MEM_FLAG_EMPTY;
mci->edac_ctl_cap = EDAC_FLAG_NONE;
mci->edac_cap = EDAC_FLAG_NONE;
mci->mod_name = "ghes_edac.c";
- mci->ctl_name = "ghes_edac";
- mci->dev_name = "ghes";
+ mci->ctl_name = "ghes_mc";
+ mci->dev_name = dev_name(dev);
mem_info_prepare_mci(mci);
rc = edac_mc_add_mc(mci);
if (rc < 0) {
- pr_err("Can't register at EDAC core\n");
- edac_mc_free(mci);
- return -ENODEV;
+ pr_err("Can't register at EDAC core (%d)\n", rc);
+ goto fail;
}
+
return 0;
+fail:
+ put_device(dev);
+ return rc;
+}
+
+static void ghes_edac_unregister_one(struct mem_ctl_info *mci)
+{
+ struct ghes_edac_pvt *pvt = mci->pvt_info;
+
+ put_device(&pvt->dev);
}
void ghes_edac_unregister(struct ghes *ghes)
{
struct mem_ctl_info *mci;
+ int nid;
- if (!ghes_pvt)
- return;
-
- mci = ghes_pvt->mci;
- edac_mc_del_mc(mci->pdev);
- edac_mc_free(mci);
+ for_each_node(nid) {
+ mci = edac_mc_find(nid);
+ /* stop fallback at last */
+ if (mci && mci != fallback)
+ ghes_edac_unregister_one(mci);
+ }
+ ghes_edac_unregister_one(fallback);
+ fallback = NULL;
kfree(mem_info.dimms);
+ atomic_dec(&ghes_init);
}
int ghes_edac_register(struct ghes *ghes, struct device *dev)
{
bool fake = false;
int rc;
+ int nid;
int idx = -1;
if (IS_ENABLED(CONFIG_X86)) {
@@ -738,7 +804,23 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
pr_info("This system has %d DIMM sockets.\n", mem_info.num_dimm);
}
- rc = ghes_edac_register_one(0, ghes, dev);
+ for_each_node(nid) {
+ if (!mem_info.dimms_per_node[nid])
+ continue;
- return rc;
+ rc = ghes_edac_register_one(nid, ghes, dev);
+ if (rc) {
+ ghes_edac_unregister(ghes);
+ return rc;
+ }
+
+ /*
+ * use the first node's mc as fallback in case we can
+ * not detect the node from the error information
+ */
+ if (!fallback)
+ fallback = edac_mc_find(nid);
+ }
+
+ return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 18/24] EDAC, ghes: Fill sysfs with the DMI DIMM label information
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (16 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 17/24] EDAC, ghes: Create one memory controller device per node Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 19/24] EDAC, mc: Introduce edac_mc_alloc_by_dimm() for per dimm allocation Robert Richter
` (6 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
This patch extracts the DIMM label from the DMI table and puts this
information into sysfs. E.g. on a ThunderX2 system we found this now:
# grep . /sys/devices/system/edac/mc/mc*/dimm*/dimm_label
/sys/devices/system/edac/mc/mc0/dimm0/dimm_label:N0 DIMM_A0
/sys/devices/system/edac/mc/mc0/dimm1/dimm_label:N0 DIMM_B0
/sys/devices/system/edac/mc/mc0/dimm2/dimm_label:N0 DIMM_C0
/sys/devices/system/edac/mc/mc0/dimm3/dimm_label:N0 DIMM_D0
/sys/devices/system/edac/mc/mc0/dimm4/dimm_label:N0 DIMM_E0
/sys/devices/system/edac/mc/mc0/dimm5/dimm_label:N0 DIMM_F0
/sys/devices/system/edac/mc/mc0/dimm6/dimm_label:N0 DIMM_G0
/sys/devices/system/edac/mc/mc0/dimm7/dimm_label:N0 DIMM_H0
/sys/devices/system/edac/mc/mc1/dimm0/dimm_label:N1 DIMM_I0
/sys/devices/system/edac/mc/mc1/dimm1/dimm_label:N1 DIMM_J0
/sys/devices/system/edac/mc/mc1/dimm2/dimm_label:N1 DIMM_K0
/sys/devices/system/edac/mc/mc1/dimm3/dimm_label:N1 DIMM_L0
/sys/devices/system/edac/mc/mc1/dimm4/dimm_label:N1 DIMM_M0
/sys/devices/system/edac/mc/mc1/dimm5/dimm_label:N1 DIMM_N0
/sys/devices/system/edac/mc/mc1/dimm6/dimm_label:N1 DIMM_O0
/sys/devices/system/edac/mc/mc1/dimm7/dimm_label:N1 DIMM_P0
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 63de11654649..9c3495f7365b 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -265,10 +265,6 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
dimm->dtype = DEV_UNKNOWN;
dimm->grain = 128; /* Likely, worse case */
- /*
- * FIXME: It shouldn't be hard to also fill the DIMM labels
- */
-
if (dimm->nr_pages) {
edac_dbg(1, "DIMM%i: %s size = %d MB%s\n",
mi->idx, edac_mem_types[dimm->mtype],
@@ -302,6 +298,7 @@ static int mem_info_setup(void)
{
struct ghes_dimm_info *dimm;
bool enable_numa = true;
+ const char *bank, *device;
int num = 0;
int idx = 0;
int ret;
@@ -317,13 +314,27 @@ static int mem_info_setup(void)
dmi_walk(ghes_edac_set_nid, NULL);
for_each_dimm(dimm) {
+ bank = device = NULL;
+ dmi_memdev_name(dimm->dimm_info.smbios_handle,
+ &bank, &device);
+ if (bank && device) {
+ snprintf(dimm->dimm_info.label,
+ sizeof(dimm->dimm_info.label),
+ "%s %s", bank, device);
+ } else {
+ *dimm->dimm_info.label = '\0';
+ }
+
if (dimm->numa_node == NUMA_NO_NODE)
enable_numa = false;
else
mem_info.dimms_per_node[dimm->numa_node]++;
- edac_dbg(1, "DIMM%i: Found mem range [%pa-%pa] on node %d\n",
- dimm->idx, &dimm->start, &dimm->end, dimm->numa_node);
+ edac_dbg(1, "DIMM%i: Found mem range [%pa-%pa] on node %d, handle: 0x%.4x%s%s\n",
+ dimm->idx, &dimm->start, &dimm->end, dimm->numa_node,
+ dimm->dimm_info.smbios_handle,
+ *dimm->dimm_info.label ? ", label: " : "",
+ dimm->dimm_info.label);
}
if (enable_numa)
@@ -381,6 +392,9 @@ static void mem_info_prepare_mci(struct mem_ctl_info *mci)
mci_dimm->dtype = dmi_dimm->dtype;
mci_dimm->grain = dmi_dimm->grain;
mci_dimm->smbios_handle = dmi_dimm->smbios_handle;
+
+ if (*dmi_dimm->label)
+ strcpy(mci_dimm->label, dmi_dimm->label);
}
if (index != mci->tot_dimms)
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 19/24] EDAC, mc: Introduce edac_mc_alloc_by_dimm() for per dimm allocation
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (17 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 18/24] EDAC, ghes: Fill sysfs with the DMI DIMM label information Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 20/24] EDAC, ghes: Identify dimm by node, card, module and handle Robert Richter
` (5 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
Systems using ACPI GHES for error detection do not have exact
knowledge of the memory hierarchy. Compared to other memory controller
drivers the total size of each layer is unknown (card/module,
channel/slot, etc.). But there is the total number of dimms. So add a
function to allocate an mc device this way. The edac's driver uses
internally a dimm index already for data access.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 83 ++++++++++++++++++++++++++++------------
drivers/edac/edac_mc.h | 17 ++++++--
drivers/edac/ghes_edac.c | 7 ++--
3 files changed, 76 insertions(+), 31 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index eea09c6acd3e..3a40496a1973 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -303,10 +303,11 @@ static void _edac_mc_free(struct mem_ctl_info *mci)
kfree(mci);
}
-struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
- unsigned n_layers,
- struct edac_mc_layer *layers,
- unsigned sz_pvt)
+struct mem_ctl_info *__edac_mc_alloc(unsigned mc_num,
+ unsigned dimm_num,
+ unsigned n_layers,
+ struct edac_mc_layer *layers,
+ unsigned sz_pvt)
{
struct mem_ctl_info *mci;
struct edac_mc_layer *layer;
@@ -321,6 +322,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
bool per_rank = false;
BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
+
/*
* Calculate the total amount of dimms and csrows/cschannels while
* in the old API emulation mode
@@ -336,6 +338,26 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
per_rank = true;
}
+ /* allocate dimm_num DIMMS, layer size must be zero */
+ if (dimm_num) {
+ if (dimm_num <= 0 ||
+ layers[0].size ||
+ (n_layers > 1 && layers[1].size) ||
+ (n_layers > 2 && layers[2].size)) {
+ edac_printk(KERN_WARNING, EDAC_MC,
+ "invalid layer data\n");
+ return NULL;
+ }
+
+ /*
+ * Assume 1 csrow per dimm which also means 1 channel
+ * per csrow.
+ */
+ tot_dimms = dimm_num;
+ tot_csrows = dimm_num;
+ tot_channels = 1;
+ }
+
/* Figure out the offsets of the various items from the start of an mc
* structure. We want the alignment of each item to be at least as
* stringent as what the compiler would provide if we could simply
@@ -422,25 +444,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
dimm->mci = mci;
dimm->idx = idx;
- /*
- * Copy DIMM location and initialize it.
- */
- len = sizeof(dimm->label);
- p = dimm->label;
- n = snprintf(p, len, "mc#%u", mc_num);
- p += n;
- len -= n;
- for (j = 0; j < n_layers; j++) {
- n = snprintf(p, len, "%s#%u",
- edac_layer_name[layers[j].type],
- pos[j]);
- p += n;
- len -= n;
- dimm->location[j] = pos[j];
-
- if (len <= 0)
- break;
- }
+ /* unknown location */
+ dimm->location[0] = -1;
+ dimm->location[1] = -1;
+ dimm->location[2] = -1;
/* Link it to the csrows old API data */
chan->dimm = dimm;
@@ -462,6 +469,34 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
}
}
+ /*
+ * Copy DIMM location and initialize it.
+ */
+ len = sizeof(dimm->label);
+ p = dimm->label;
+ n = snprintf(p, len, "mc#%u", mc_num);
+ p += n;
+ len -= n;
+
+ if (dimm_num) {
+ n = snprintf(p, len, "dimm#%u", idx);
+ p += n;
+ len -= n;
+ continue;
+ }
+
+ for (j = 0; j < n_layers; j++) {
+ n = snprintf(p, len, "%s#%u",
+ edac_layer_name[layers[j].type],
+ pos[j]);
+ p += n;
+ len -= n;
+ dimm->location[j] = pos[j];
+
+ if (len <= 0)
+ break;
+ }
+
/* Increment dimm location */
for (j = n_layers - 1; j >= 0; j--) {
pos[j]++;
@@ -480,7 +515,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
return NULL;
}
-EXPORT_SYMBOL_GPL(edac_mc_alloc);
+EXPORT_SYMBOL_GPL(__edac_mc_alloc);
void edac_mc_free(struct mem_ctl_info *mci)
{
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index c4ddd5c1e24c..e8215847f853 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -99,6 +99,10 @@ do { \
* edac_mc_alloc() - Allocate and partially fill a struct &mem_ctl_info.
*
* @mc_num: Memory controller number
+ * @dimm_num: Number of DIMMs to allocate. If non-zero the
+ * @layers' size parameter must be zero. Useful
+ * if the MC hierarchy is unknown but the number
+ * of DIMMs is known.
* @n_layers: Number of MC hierarchy layers
* @layers: Describes each layer as seen by the Memory Controller
* @sz_pvt: size of private storage needed
@@ -122,10 +126,15 @@ do { \
* On success, return a pointer to struct mem_ctl_info pointer;
* %NULL otherwise
*/
-struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
- unsigned n_layers,
- struct edac_mc_layer *layers,
- unsigned sz_pvt);
+struct mem_ctl_info *__edac_mc_alloc(unsigned mc_num,
+ unsigned dimm_num,
+ unsigned n_layers,
+ struct edac_mc_layer *layers,
+ unsigned sz_pvt);
+#define edac_mc_alloc(mc_num, n_layers, layers, sz_pvt) \
+ __edac_mc_alloc(mc_num, 0, n_layers, layers, sz_pvt)
+#define edac_mc_alloc_by_dimm(mc_num, dimm_num, n_layers, layers, sz_pvt) \
+ __edac_mc_alloc(mc_num, dimm_num, n_layers, layers, sz_pvt)
/**
* edac_get_owner - Return the owner's mod_name of EDAC MC
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 9c3495f7365b..97f992006281 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -702,11 +702,12 @@ ghes_edac_register_one(int nid, struct ghes *ghes, struct device *parent)
struct edac_mc_layer layers[1];
layers[0].type = EDAC_MC_LAYER_ALL_MEM;
- layers[0].size = mem_info.dimms_per_node[nid];
+ layers[0].size = 0;
layers[0].is_virt_csrow = true;
- mci = edac_mc_alloc(nid, ARRAY_SIZE(layers), layers,
- sizeof(struct ghes_edac_pvt));
+ mci = edac_mc_alloc_by_dimm(nid, mem_info.dimms_per_node[nid],
+ ARRAY_SIZE(layers), layers,
+ sizeof(struct ghes_edac_pvt));
if (!mci) {
pr_err("Can't allocate memory for EDAC data\n");
return -ENOMEM;
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 20/24] EDAC, ghes: Identify dimm by node, card, module and handle
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (18 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 19/24] EDAC, mc: Introduce edac_mc_alloc_by_dimm() for per dimm allocation Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 21/24] EDAC, ghes: Enable per-layer reporting based on card/module Robert Richter
` (4 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
According to SMBIOS Spec. 2.7 (N.2.5 Memory Error Section), a failing
DIMM (module or rank number) can be identified by its error location
consisting of node, card and module. A module handle is used to map it
to the dimms listed in the dmi table. Collect all those data from the
error record and select the dimm accordingly. Inconsistent error
records will be reported which is the case if the same dimm handle
reports errors with different node, card or module.
The change allows to enable per-layer reporting based on node, card
and module in the next patch.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 74 +++++++++++++++++++++++++++++++++-------
1 file changed, 62 insertions(+), 12 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 97f992006281..689841c5c84d 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -81,8 +81,11 @@ struct memarr_dmi_entry {
struct ghes_dimm_info {
struct dimm_info dimm_info;
+ struct dimm_info *dimm;
int idx;
int numa_node;
+ int card;
+ int module;
phys_addr_t start;
phys_addr_t end;
u16 phys_handle;
@@ -128,6 +131,8 @@ static int ghes_dimm_info_init(int num)
for_each_dimm(dimm) {
dimm->idx = idx;
dimm->numa_node = NUMA_NO_NODE;
+ dimm->card = -1;
+ dimm->module = -1;
idx++;
}
@@ -395,6 +400,13 @@ static void mem_info_prepare_mci(struct mem_ctl_info *mci)
if (*dmi_dimm->label)
strcpy(mci_dimm->label, dmi_dimm->label);
+
+ /*
+ * From here on do not use any longer &dimm.dimm_info.
+ * Instead switch to the mci's dimm info which might
+ * contain updated data, such as the label.
+ */
+ dimm->dimm = mci_dimm;
}
if (index != mci->tot_dimms)
@@ -402,24 +414,46 @@ static void mem_info_prepare_mci(struct mem_ctl_info *mci)
index, mci->tot_dimms);
}
-static struct mem_ctl_info *get_mc_by_node(int nid)
+/* Requires ghes_lock being set. */
+static struct ghes_dimm_info *
+get_and_prepare_dimm_info(int nid, int card, int module, int handle)
{
- struct mem_ctl_info *mci = edac_mc_find(nid);
+ static struct ghes_dimm_info *dimm;
+ struct dimm_info *di;
- if (mci)
- return mci;
+ /*
+ * We require smbios_handle being set in the error report for
+ * per layer reporting (SMBIOS handle for the Type 17 Memory
+ * Device Structure that represents the Memory Module)
+ */
+ for_each_dimm(dimm) {
+ di = dimm->dimm;
+ if (di->smbios_handle == handle)
+ goto found;
+ }
- if (num_possible_nodes() > 1) {
- edac_mc_printk(fallback, KERN_WARNING,
- "Invalid or no node information, falling back to first node: %s",
- fallback->dev_name);
+ return NULL;
+found:
+ if (dimm->card < 0 && card >= 0)
+ dimm->card = card;
+ if (dimm->module < 0 && module >= 0)
+ dimm->module = module;
+
+ if ((num_possible_nodes() > 1 && di->mci->mc_idx != nid) ||
+ (card >= 0 && card != dimm->card) ||
+ (module >= 0 && module != dimm->module)) {
+ edac_mc_printk(di->mci, KERN_WARNING,
+ "Inconsistent error report (nid/card/module): %d/%d/%d (dimm%d: %d/%d/%d)",
+ nid, card, module, di->idx,
+ di->mci->mc_idx, dimm->card, dimm->module);
}
- return fallback;
+ return dimm;
}
void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
{
+ struct ghes_dimm_info *dimm;
struct dimm_info *dimm_info;
enum hw_event_mc_err_type type;
struct edac_raw_error_desc *e;
@@ -428,6 +462,9 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
unsigned long flags;
char *p;
int nid = NUMA_NO_NODE;
+ int card = -1;
+ int module = -1;
+ int handle = -1;
/* We need at least one mc */
if (WARN_ON_ONCE(!fallback))
@@ -443,10 +480,23 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
spin_lock_irqsave(&ghes_lock, flags);
- /* select the node's mc device */
if (mem_err->validation_bits & CPER_MEM_VALID_NODE)
nid = mem_err->node;
- mci = get_mc_by_node(nid);
+ if (mem_err->validation_bits & CPER_MEM_VALID_CARD)
+ card = mem_err->card;
+ if (mem_err->validation_bits & CPER_MEM_VALID_MODULE)
+ module = mem_err->module;
+ if (mem_err->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
+ handle = mem_err->mem_dev_handle;
+
+ dimm = get_and_prepare_dimm_info(nid, card, module, handle);
+ if (dimm)
+ mci = dimm->dimm->mci;
+ else
+ mci = edac_mc_find(nid);
+ if (!mci)
+ mci = fallback;
+
pvt = mci->pvt_info;
e = &mci->error_desc;
@@ -665,7 +715,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
if (p > pvt->other_detail)
*(p - 1) = '\0';
- dimm_info = edac_get_dimm_by_index(mci, e->top_layer);
+ dimm_info = dimm ? dimm->dimm : NULL;
edac_raw_mc_handle_error(type, mci, dimm_info, e, -1, -1);
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 21/24] EDAC, ghes: Enable per-layer reporting based on card/module
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (19 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 20/24] EDAC, ghes: Identify dimm by node, card, module and handle Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 22/24] EDAC, ghes: Move struct member smbios_handle to struct ghes_dimm_info Robert Richter
` (3 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
This patch enables per-layer reporting of the GHES driver based on
node, card and module. A dimm can be uniquely identified by those 3
identifiers. The mc device is selected by the node id. Thus, each ghes
edac memory controller device has a 2-dimensional layer hierarchy
based on card and module in the same way as most other driver have. An
error log looks as follows now:
[ 8902.592060] {4}[Hardware Error]: Error 6, type: corrected
[ 8902.597534] {4}[Hardware Error]: section_type: memory error
[ 8902.603267] {4}[Hardware Error]: error_status: 0x0000000000000400
[ 8902.609522] {4}[Hardware Error]: physical_address: 0x000000b3bb7d3000
[ 8902.616126] {4}[Hardware Error]: node: 1 card: 3 module: 0 rank: 1 bank: 771 column: 14 bit_position: 16
[ 8902.625854] {4}[Hardware Error]: DIMM location: N1 DIMM_L0
[ 8902.807783] EDAC MC1: 1 CE ghes_mc on N1 DIMM_L0 (card:3 module:0 page:0xb3bb7d3 offset:0x0 grain:0 syndrome:0x0 - APEI location: node:1 card:3 module:0 rank:1 bank:771 col:14 bit_pos:16 handle:0x0052 status(0x0000000000000400): Storage error in DRAM memory)
GHES error reports are now similar to edac_mc reports. This patch
moves common code of ghes and edac_mc to edac_raw_mc_handle_error().
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 45 ++++++++++++++----------
drivers/edac/ghes_edac.c | 76 ++++++++++++++++++----------------------
include/linux/edac.h | 2 ++
3 files changed, 63 insertions(+), 60 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 3a40496a1973..9383a1179b83 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -915,11 +915,13 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
EXPORT_SYMBOL_GPL(edac_mc_find_csrow_by_page);
const char *edac_layer_name[] = {
- [EDAC_MC_LAYER_BRANCH] = "branch",
- [EDAC_MC_LAYER_CHANNEL] = "channel",
- [EDAC_MC_LAYER_SLOT] = "slot",
- [EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
- [EDAC_MC_LAYER_ALL_MEM] = "memory",
+ [EDAC_MC_LAYER_BRANCH] = "branch",
+ [EDAC_MC_LAYER_CHANNEL] = "channel",
+ [EDAC_MC_LAYER_SLOT] = "slot",
+ [EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
+ [EDAC_MC_LAYER_ALL_MEM] = "memory",
+ [EDAC_MC_LAYER_CARD] = "card",
+ [EDAC_MC_LAYER_MODULE] = "module",
};
EXPORT_SYMBOL_GPL(edac_layer_name);
@@ -1046,7 +1048,26 @@ void edac_raw_mc_handle_error(const enum hw_event_mc_err_type type,
int row, int chan)
{
char detail[80];
+ int idx;
+ int pos[EDAC_MAX_LAYERS] = { e->top_layer, e->mid_layer,
+ e->low_layer };
u8 grain_bits;
+ char *p;
+
+ /* Fill the RAM location data */
+ p = e->location;
+
+ for (idx = 0; idx < mci->n_layers; idx++) {
+ if (pos[idx] < 0)
+ continue;
+
+ p += sprintf(p, "%s:%d ",
+ edac_layer_name[mci->layers[idx].type],
+ pos[idx]);
+ }
+
+ if (p > e->location)
+ *(p - 1) = '\0';
/*
* We expect the hw to report a reasonable grain, fallback to
@@ -1233,20 +1254,6 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
else if (!*e->label)
strcpy(e->label, "unknown memory");
- /* Fill the RAM location data */
- p = e->location;
-
- for (i = 0; i < mci->n_layers; i++) {
- if (pos[i] < 0)
- continue;
-
- p += sprintf(p, "%s:%d ",
- edac_layer_name[mci->layers[i].type],
- pos[i]);
- }
- if (p > e->location)
- *(p - 1) = '\0';
-
dimm = edac_get_dimm(mci, top_layer, mid_layer, low_layer);
edac_raw_mc_handle_error(type, mci, dimm, e, row, chan);
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 689841c5c84d..fb5a54e27917 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -178,18 +178,6 @@ static void ghes_edac_set_nid(const struct dmi_header *dh, void *arg)
}
}
-static int get_dimm_smbios_index(struct mem_ctl_info *mci, u16 handle)
-{
- struct dimm_info *dimm;
-
- mci_for_each_dimm(mci, dimm) {
- if (dimm->smbios_handle == handle)
- return dimm->idx;
- }
-
- return -1;
-}
-
static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
{
if (dh->type == DMI_ENTRY_MEM_DEVICE) {
@@ -500,11 +488,13 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
pvt = mci->pvt_info;
e = &mci->error_desc;
+ edac_dbg(3, "MC%d\n", mci->mc_idx);
+
/* Cleans the error report buffer */
memset(e, 0, sizeof (*e));
+
e->error_count = 1;
e->grain = 1;
- strcpy(e->label, "unknown label");
e->top_layer = -1;
e->mid_layer = -1;
e->low_layer = -1;
@@ -514,6 +504,25 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
*pvt->msg = '\0';
*pvt->other_detail = '\0';
+ if (dimm) {
+ /* The DIMM could be identified. */
+ e->top_layer = dimm->card;
+ e->mid_layer = dimm->module;
+ strcpy(e->label, dimm->dimm->label);
+ } else if (nid >= 0 || card >= 0 || module >= 0 || handle >= 0) {
+ /*
+ * We have at least some information and can do a
+ * per-layer reporting, but the exact location is
+ * unknown.
+ */
+ e->top_layer = card;
+ e->mid_layer = module;
+ strcpy(e->label, "unknown memory");
+ } else {
+ /* No error location at all. */
+ strcpy(e->label, "any memory");
+ }
+
switch (sev) {
case GHES_SEV_CORRECTED:
type = HW_EVENT_ERR_CORRECTED;
@@ -533,8 +542,10 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
(long long)mem_err->validation_bits);
/* Error type, mapped on e->msg */
+ p = pvt->msg;
+ p += sprintf(p, "%s", mci->ctl_name);
if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
- p = pvt->msg;
+ p += sprintf(p, ": ");
switch (mem_err->error_type) {
case 0:
p += sprintf(p, "Unknown");
@@ -588,8 +599,6 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
p += sprintf(p, "reserved error (%d)",
mem_err->error_type);
}
- } else {
- strcpy(pvt->msg, "unknown error");
}
/* Error address */
@@ -602,8 +611,9 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
e->grain = ~mem_err->physical_addr_mask + 1;
- /* Memory error location, mapped on e->location */
- p = e->location;
+ /* Memory error location, mapped on e->other_detail */
+ p = pvt->other_detail;
+ p += snprintf(p, sizeof(pvt->other_detail), "APEI location: ");
if (mem_err->validation_bits & CPER_MEM_VALID_NODE)
p += sprintf(p, "node:%d ", mem_err->node);
if (mem_err->validation_bits & CPER_MEM_VALID_CARD)
@@ -621,27 +631,8 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
if (mem_err->validation_bits & CPER_MEM_VALID_BIT_POSITION)
p += sprintf(p, "bit_pos:%d ", mem_err->bit_pos);
if (mem_err->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
- const char *bank = NULL, *device = NULL;
- int index = -1;
-
- dmi_memdev_name(mem_err->mem_dev_handle, &bank, &device);
- if (bank != NULL && device != NULL)
- p += sprintf(p, "DIMM location:%s %s ", bank, device);
- else
- p += sprintf(p, "DIMM DMI handle: 0x%.4x ",
- mem_err->mem_dev_handle);
-
- index = get_dimm_smbios_index(mci, mem_err->mem_dev_handle);
- if (index >= 0)
- e->top_layer = index;
+ p += sprintf(p, "handle:0x%.4x ", handle);
}
- if (p > e->location)
- *(p - 1) = '\0';
-
- /* All other fields are mapped on e->other_detail */
- p = pvt->other_detail;
- p += snprintf(p, sizeof(pvt->other_detail),
- "APEI location: %s ", e->location);
if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_STATUS) {
u64 status = mem_err->error_status;
@@ -749,11 +740,14 @@ ghes_edac_register_one(int nid, struct ghes *ghes, struct device *parent)
struct ghes_edac_pvt *ghes_pvt;
int rc;
struct mem_ctl_info *mci;
- struct edac_mc_layer layers[1];
+ struct edac_mc_layer layers[2];
- layers[0].type = EDAC_MC_LAYER_ALL_MEM;
+ layers[0].type = EDAC_MC_LAYER_CARD;
layers[0].size = 0;
- layers[0].is_virt_csrow = true;
+ layers[0].is_virt_csrow = false;
+ layers[1].type = EDAC_MC_LAYER_MODULE;
+ layers[1].size = 0;
+ layers[1].is_virt_csrow = false;
mci = edac_mc_alloc_by_dimm(nid, mem_info.dimms_per_node[nid],
ARRAY_SIZE(layers), layers,
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 4dcf075e9dff..40e7da735e48 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -336,6 +336,8 @@ enum edac_mc_layer_type {
EDAC_MC_LAYER_SLOT,
EDAC_MC_LAYER_CHIP_SELECT,
EDAC_MC_LAYER_ALL_MEM,
+ EDAC_MC_LAYER_CARD, /* SMBIOS Type 16 Memory Array */
+ EDAC_MC_LAYER_MODULE, /* SMBIOS Type 17 Memory Device */
};
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 22/24] EDAC, ghes: Move struct member smbios_handle to struct ghes_dimm_info
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (20 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 21/24] EDAC, ghes: Enable per-layer reporting based on card/module Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 23/24] EDAC, Documentation: Describe CPER module definition and DIMM ranks Robert Richter
` (2 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
This is private to the ghes_edac driver, so better keep that data in
the struct ghes_dimm_info.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/ghes_edac.c | 15 +++++++--------
include/linux/edac.h | 2 --
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index fb5a54e27917..714623204232 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -88,6 +88,7 @@ struct ghes_dimm_info {
int module;
phys_addr_t start;
phys_addr_t end;
+ u16 smbios_handle;
u16 phys_handle;
};
@@ -187,6 +188,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
struct dimm_info *dimm = &mi->dimm_info;
u16 rdr_mask = BIT(7) | BIT(13);
+ mi->smbios_handle = entry->handle;
mi->phys_handle = entry->phys_mem_array_handle;
if (entry->size == 0xffff) {
@@ -268,8 +270,6 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
entry->total_width, entry->data_width);
}
- dimm->smbios_handle = entry->handle;
-
(*idx)++;
}
}
@@ -308,8 +308,7 @@ static int mem_info_setup(void)
for_each_dimm(dimm) {
bank = device = NULL;
- dmi_memdev_name(dimm->dimm_info.smbios_handle,
- &bank, &device);
+ dmi_memdev_name(dimm->smbios_handle, &bank, &device);
if (bank && device) {
snprintf(dimm->dimm_info.label,
sizeof(dimm->dimm_info.label),
@@ -325,7 +324,7 @@ static int mem_info_setup(void)
edac_dbg(1, "DIMM%i: Found mem range [%pa-%pa] on node %d, handle: 0x%.4x%s%s\n",
dimm->idx, &dimm->start, &dimm->end, dimm->numa_node,
- dimm->dimm_info.smbios_handle,
+ dimm->smbios_handle,
*dimm->dimm_info.label ? ", label: " : "",
dimm->dimm_info.label);
}
@@ -384,7 +383,6 @@ static void mem_info_prepare_mci(struct mem_ctl_info *mci)
mci_dimm->edac_mode = dmi_dimm->edac_mode;
mci_dimm->dtype = dmi_dimm->dtype;
mci_dimm->grain = dmi_dimm->grain;
- mci_dimm->smbios_handle = dmi_dimm->smbios_handle;
if (*dmi_dimm->label)
strcpy(mci_dimm->label, dmi_dimm->label);
@@ -415,13 +413,14 @@ get_and_prepare_dimm_info(int nid, int card, int module, int handle)
* Device Structure that represents the Memory Module)
*/
for_each_dimm(dimm) {
- di = dimm->dimm;
- if (di->smbios_handle == handle)
+ if (dimm->smbios_handle == handle)
goto found;
}
return NULL;
found:
+ di = dimm->dimm;
+
if (dimm->card < 0 && card >= 0)
dimm->card = card;
if (dimm->module < 0 && module >= 0)
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 40e7da735e48..32ad882bea15 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -384,8 +384,6 @@ struct dimm_info {
unsigned csrow, cschannel; /* Points to the old API data */
- u16 smbios_handle; /* Handle for SMBIOS type 17 */
-
u32 ce_count;
u32 ue_count;
};
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 23/24] EDAC, Documentation: Describe CPER module definition and DIMM ranks
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (21 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 22/24] EDAC, ghes: Move struct member smbios_handle to struct ghes_dimm_info Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-24 15:09 ` [PATCH v2 24/24] EDAC, ghes: Disable legacy API for ARM64 Robert Richter
2019-08-02 7:58 ` [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab, Tony Luck,
Jonathan Corbet
Cc: linux-edac, linux-kernel, Robert Richter, linux-doc
Update on CPER DIMM naming convention and DIMM ranks.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
Documentation/admin-guide/ras.rst | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/Documentation/admin-guide/ras.rst b/Documentation/admin-guide/ras.rst
index c7495e42e6f4..4e2a01c77a9c 100644
--- a/Documentation/admin-guide/ras.rst
+++ b/Documentation/admin-guide/ras.rst
@@ -330,9 +330,12 @@ There can be multiple csrows and multiple channels.
.. [#f4] Nowadays, the term DIMM (Dual In-line Memory Module) is widely
used to refer to a memory module, although there are other memory
- packaging alternatives, like SO-DIMM, SIMM, etc. Along this document,
- and inside the EDAC system, the term "dimm" is used for all memory
- modules, even when they use a different kind of packaging.
+ packaging alternatives, like SO-DIMM, SIMM, etc. The UEFI
+ specification (Version 2.7) defines a memory module in the Common
+ Platform Error Record (CPER) section to be an SMBIOS Memory Device
+ (Type 17). Along this document, and inside the EDAC system, the term
+ "dimm" is used for all memory modules, even when they use a
+ different kind of packaging.
Memory controllers allow for several csrows, with 8 csrows being a
typical value. Yet, the actual number of csrows depends on the layout of
@@ -349,12 +352,14 @@ controllers. The following example will assume 2 channels:
| | ``ch0`` | ``ch1`` |
+============+===========+===========+
| ``csrow0`` | DIMM_A0 | DIMM_B0 |
- +------------+ | |
- | ``csrow1`` | | |
+ | | rank0 | rank0 |
+ +------------+ - | - |
+ | ``csrow1`` | rank1 | rank1 |
+------------+-----------+-----------+
| ``csrow2`` | DIMM_A1 | DIMM_B1 |
- +------------+ | |
- | ``csrow3`` | | |
+ | | rank0 | rank0 |
+ +------------+ - | - |
+ | ``csrow3`` | rank1 | rank1 |
+------------+-----------+-----------+
In the above example, there are 4 physical slots on the motherboard
@@ -374,11 +379,13 @@ which the memory DIMM is placed. Thus, when 1 DIMM is placed in each
Channel, the csrows cross both DIMMs.
Memory DIMMs come single or dual "ranked". A rank is a populated csrow.
-Thus, 2 single ranked DIMMs, placed in slots DIMM_A0 and DIMM_B0 above
-will have just one csrow (csrow0). csrow1 will be empty. On the other
-hand, when 2 dual ranked DIMMs are similarly placed, then both csrow0
-and csrow1 will be populated. The pattern repeats itself for csrow2 and
-csrow3.
+In the example above 2 dual ranked DIMMs are similarly placed. Thus,
+both csrow0 and csrow1 are populated. On the other hand, when 2 single
+ranked DIMMs are placed in slots DIMM_A0 and DIMM_B0, then they will
+have just one csrow (csrow0) and csrow1 will be empty. The pattern
+repeats itself for csrow2 and csrow3. Also note that some memory
+controller doesn't have any logic to identify the memory module, see
+``rankX`` directories below.
The representation of the above is reflected in the directory
tree in EDAC's sysfs interface. Starting in directory
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 24/24] EDAC, ghes: Disable legacy API for ARM64
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (22 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 23/24] EDAC, Documentation: Describe CPER module definition and DIMM ranks Robert Richter
@ 2019-06-24 15:09 ` Robert Richter
2019-06-26 9:33 ` James Morse
2019-08-02 7:58 ` [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
24 siblings, 1 reply; 52+ messages in thread
From: Robert Richter @ 2019-06-24 15:09 UTC (permalink / raw)
To: Borislav Petkov, James Morse, Mauro Carvalho Chehab
Cc: linux-edac, linux-kernel, Robert Richter
James Morse: "I'm all for removing/warning-its-broken it when
ghes_edac is in use."
Let's just disable legacy API for the ghes driver on arm64. Though, I
don't agree with it as there still could be some userland tools that
use this interface that cannot be used any longer after a transition
from x86 to arm64. I leave that decision up to James.
Signed-off-by: Robert Richter <rrichter@marvell.com>
---
drivers/edac/edac_mc.c | 34 +++++++++++++++++++++++++++-------
drivers/edac/edac_mc.h | 7 +++++++
drivers/edac/ghes_edac.c | 4 +++-
3 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 9383a1179b83..d7a120d7e304 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -276,16 +276,11 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
return (void *)(((unsigned long)ptr) + align - r);
}
-static void _edac_mc_free(struct mem_ctl_info *mci)
+static void _edac_mc_free_legacy(struct mem_ctl_info *mci)
{
- int i, chn, row;
+ int chn, row;
struct csrow_info *csr;
- if (mci->dimms) {
- for (i = 0; i < mci->tot_dimms; i++)
- kfree(mci->dimms[i]);
- kfree(mci->dimms);
- }
if (mci->csrows) {
for (row = 0; row < mci->nr_csrows; row++) {
csr = mci->csrows[row];
@@ -300,9 +295,34 @@ static void _edac_mc_free(struct mem_ctl_info *mci)
}
kfree(mci->csrows);
}
+}
+
+static void _edac_mc_free(struct mem_ctl_info *mci)
+{
+ int i;
+
+ if (mci->dimms) {
+ for (i = 0; i < mci->tot_dimms; i++)
+ kfree(mci->dimms[i]);
+ kfree(mci->dimms);
+ }
+
+ _edac_mc_free_legacy(mci);
+
kfree(mci);
}
+void edac_mc_disable_legacy_api(struct mem_ctl_info *mci)
+{
+#ifdef CONFIG_EDAC_LEGACY_SYSFS
+ _edac_mc_free_legacy(mci);
+ mci->nr_csrows = 0;
+ mci->num_cschannel = 0;
+ mci->csrows = NULL;
+#endif
+}
+EXPORT_SYMBOL_GPL(edac_mc_disable_legacy_api);
+
struct mem_ctl_info *__edac_mc_alloc(unsigned mc_num,
unsigned dimm_num,
unsigned n_layers,
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index e8215847f853..d1bacc5f47f5 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -270,6 +270,13 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
const char *msg,
const char *other_detail);
+/**
+ * edac_mc_disable_legacy_api() - Disable legacy sysfs API
+ *
+ * @mci: a struct mem_ctl_info pointer
+ */
+void edac_mc_disable_legacy_api(struct mem_ctl_info *mci);
+
/*
* edac misc APIs
*/
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 714623204232..804e48773f66 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -780,7 +780,9 @@ ghes_edac_register_one(int nid, struct ghes *ghes, struct device *parent)
mci->dev_name = dev_name(dev);
mem_info_prepare_mci(mci);
-
+#ifdef CONFIG_ARM64
+ edac_mc_disable_legacy_api(mci);
+#endif
rc = edac_mc_add_mc(mci);
if (rc < 0) {
pr_err("Can't register at EDAC core (%d)\n", rc);
--
2.20.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 24/24] EDAC, ghes: Disable legacy API for ARM64
2019-06-24 15:09 ` [PATCH v2 24/24] EDAC, ghes: Disable legacy API for ARM64 Robert Richter
@ 2019-06-26 9:33 ` James Morse
2019-06-26 10:11 ` Robert Richter
0 siblings, 1 reply; 52+ messages in thread
From: James Morse @ 2019-06-26 9:33 UTC (permalink / raw)
To: Robert Richter
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 24/06/2019 16:09, Robert Richter wrote:
> James Morse: "I'm all for removing/warning-its-broken it when
> ghes_edac is in use."
Thanks for taking that out of context. The very next word was 'but':
http://lore.kernel.org/r/c08290d8-3690-efa9-3bc7-37f8b1fdbfd4@arm.com
followed by details of the user-space that is still using this.
> Let's just disable legacy API for the ghes driver on arm64. Though, I
> don't agree with it as there still could be some userland tools that
Not could. Are. Someone went and found them for you.
> use this interface that cannot be used any longer after a transition
> from x86 to arm64.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 24/24] EDAC, ghes: Disable legacy API for ARM64
2019-06-26 9:33 ` James Morse
@ 2019-06-26 10:11 ` Robert Richter
0 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-06-26 10:11 UTC (permalink / raw)
To: James Morse
Cc: Borislav Petkov, Mauro Carvalho Chehab, linux-edac, linux-kernel
On 26.06.19 10:33:39, James Morse wrote:
> On 24/06/2019 16:09, Robert Richter wrote:
> > James Morse: "I'm all for removing/warning-its-broken it when
> > ghes_edac is in use."
>
> Thanks for taking that out of context. The very next word was 'but':
> http://lore.kernel.org/r/c08290d8-3690-efa9-3bc7-37f8b1fdbfd4@arm.com
>
> followed by details of the user-space that is still using this.
>
>
> > Let's just disable legacy API for the ghes driver on arm64. Though, I
> > don't agree with it as there still could be some userland tools that
>
> Not could. Are. Someone went and found them for you.
>
>
> > use this interface that cannot be used any longer after a transition
> > from x86 to arm64.
Ok, thanks for clarifying.
-Robert
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting
2019-06-24 15:08 [PATCH v2 00/24] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
` (23 preceding siblings ...)
2019-06-24 15:09 ` [PATCH v2 24/24] EDAC, ghes: Disable legacy API for ARM64 Robert Richter
@ 2019-08-02 7:58 ` Robert Richter
24 siblings, 0 replies; 52+ messages in thread
From: Robert Richter @ 2019-08-02 7:58 UTC (permalink / raw)
To: Robert Richter
Cc: Borislav Petkov, James Morse, Mauro Carvalho Chehab, Tony Luck,
linux-edac, linux-kernel
Hi all,
this is a friendly ping for review of this series.
Thanks,
-Robert
On 24.06.19 15:08:52, Robert Richter wrote:
> Current arm64 systems that use the ghes driver lack kernel support for
> a proper memory error reporting. Following issues are seen:
>
> * Error record shows insufficient data, such as "EDAC MC0: 1 CE
> unknown error on unknown label",
>
> * DMI DIMM labels are not decoded for error reporting,
>
> * No memory hierarchy known (NUMA topology),
>
> * No per layer reporting,
>
> * Significant differences to x86 error reports.
>
> This patch set addresses all the above involving a rework of the
> ghes_edac and edac_mc driver.
>
> Patch #1-#4: Fix of grain calculation in edac_mc.c (#1) and
> ghes_edac.c (#2) including unification of trace_mc_event() code (#3,
> #4).
>
> Patches #5-#12: General fixes and improvements of the ghes and mc
> drivers. Most of it is a rework of existing code without functional
> changes to improve, ease, cleanup and join common code. The changes
> are in preparation of and a requirment for the following patches that
> improve ghes error reports.
>
> Patches #13-#22: Improve error memory reporting of the ghes driver
> including:
>
> * support for legacy API (patch #12),
>
> * NUMA detection, one mc device per node (patches #13-#16),
>
> * support for DMI DIMM label information (patch #17),
>
> * per-layer reporting (patches #18-#20).
>
> Patch #23: Documentation updates.
>
> Patch #24: Disable legacy API for ARM64 ghes driver (optional, need to
> be ack'ed by James, I vote for not applying it).
>
> All changes should keep existing systems working as before. All
> systems that are using ghes will also benefit from the update. There
> is a fallback in the ghes driver that disables NUMA or enters a fake
> mode if some of the NUMA or DIMM information is inconsistent. So it
> should not break existing systems that provide broken firmware tables.
>
> The series has been tested on a Marvell/Cavium ThunderX2 system. Here
> some example logs and sysfs entries:
>
> Boot log of memory hierarchy and dimm detection:
>
> EDAC DEBUG: mem_info_setup: DIMM0: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x0038, label: N0 DIMM_A0
> EDAC DEBUG: mem_info_setup: DIMM1: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x0039, label: N0 DIMM_B0
> EDAC DEBUG: mem_info_setup: DIMM2: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x003a, label: N0 DIMM_C0
> EDAC DEBUG: mem_info_setup: DIMM3: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x003b, label: N0 DIMM_D0
> EDAC DEBUG: mem_info_setup: DIMM4: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x003c, label: N0 DIMM_E0
> EDAC DEBUG: mem_info_setup: DIMM5: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x003d, label: N0 DIMM_F0
> EDAC DEBUG: mem_info_setup: DIMM6: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x003e, label: N0 DIMM_G0
> EDAC DEBUG: mem_info_setup: DIMM7: Found mem range [0x0000008800000000-0x0000009ffcffffff] on node 0, handle: 0x003f, label: N0 DIMM_H0
> EDAC DEBUG: mem_info_setup: DIMM8: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x004f, label: N1 DIMM_I0
> EDAC DEBUG: mem_info_setup: DIMM9: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0050, label: N1 DIMM_J0
> EDAC DEBUG: mem_info_setup: DIMM10: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0051, label: N1 DIMM_K0
> EDAC DEBUG: mem_info_setup: DIMM11: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0052, label: N1 DIMM_L0
> EDAC DEBUG: mem_info_setup: DIMM12: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0053, label: N1 DIMM_M0
> EDAC DEBUG: mem_info_setup: DIMM13: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0054, label: N1 DIMM_N0
> EDAC DEBUG: mem_info_setup: DIMM14: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0055, label: N1 DIMM_O0
> EDAC DEBUG: mem_info_setup: DIMM15: Found mem range [0x0000009ffd000000-0x000000bffcffffff] on node 1, handle: 0x0056, label: N1 DIMM_P0
>
> DIMM label entries in sysfs:
>
> # grep . /sys/devices/system/edac/mc/mc*/dimm*/dimm_label
> /sys/devices/system/edac/mc/mc0/dimm0/dimm_label:N0 DIMM_A0
> /sys/devices/system/edac/mc/mc0/dimm1/dimm_label:N0 DIMM_B0
> /sys/devices/system/edac/mc/mc0/dimm2/dimm_label:N0 DIMM_C0
> /sys/devices/system/edac/mc/mc0/dimm3/dimm_label:N0 DIMM_D0
> /sys/devices/system/edac/mc/mc0/dimm4/dimm_label:N0 DIMM_E0
> /sys/devices/system/edac/mc/mc0/dimm5/dimm_label:N0 DIMM_F0
> /sys/devices/system/edac/mc/mc0/dimm6/dimm_label:N0 DIMM_G0
> /sys/devices/system/edac/mc/mc0/dimm7/dimm_label:N0 DIMM_H0
> /sys/devices/system/edac/mc/mc1/dimm0/dimm_label:N1 DIMM_I0
> /sys/devices/system/edac/mc/mc1/dimm1/dimm_label:N1 DIMM_J0
> /sys/devices/system/edac/mc/mc1/dimm2/dimm_label:N1 DIMM_K0
> /sys/devices/system/edac/mc/mc1/dimm3/dimm_label:N1 DIMM_L0
> /sys/devices/system/edac/mc/mc1/dimm4/dimm_label:N1 DIMM_M0
> /sys/devices/system/edac/mc/mc1/dimm5/dimm_label:N1 DIMM_N0
> /sys/devices/system/edac/mc/mc1/dimm6/dimm_label:N1 DIMM_O0
> /sys/devices/system/edac/mc/mc1/dimm7/dimm_label:N1 DIMM_P0
>
> Memory error reports in the kernel log:
>
> {1}[Hardware Error]: Error 4, type: corrected
> {1}[Hardware Error]: section_type: memory error
> {1}[Hardware Error]: error_status: 0x0000000000000400
> {1}[Hardware Error]: physical_address: 0x000000bd0db44000
> {1}[Hardware Error]: node: 1 card: 3 module: 0 rank: 0 bank: 256 column: 10 bit_position: 16
> {1}[Hardware Error]: DIMM location: N1 DIMM_L0
> EDAC MC1: 1 CE ghes_mc on N1 DIMM_L0 (card:3 module:0 page:0xbd0db44 offset:0x0 grain:0 syndrome:0x0 - APEI location: node:1 card:3 module:0 rank:0 bank:256 col:10 bit_pos:16 handle:0x0052 status(0x0000000000000400): Storage error in DRAM memory)
>
> Error counters in sysfs (zero counters dropped):
>
> # find /sys/devices/system/edac/mc/ -name \*count | sort -V | xargs grep . | sed -e '/:0/d'
> /sys/devices/system/edac/mc/mc0/ce_count:5
> /sys/devices/system/edac/mc/mc0/csrow0/ce_count:1
> /sys/devices/system/edac/mc/mc0/csrow0/ch0_ce_count:1
> /sys/devices/system/edac/mc/mc0/csrow3/ce_count:1
> /sys/devices/system/edac/mc/mc0/csrow3/ch0_ce_count:1
> /sys/devices/system/edac/mc/mc0/csrow4/ce_count:1
> /sys/devices/system/edac/mc/mc0/csrow4/ch0_ce_count:1
> /sys/devices/system/edac/mc/mc0/csrow6/ce_count:2
> /sys/devices/system/edac/mc/mc0/csrow6/ch0_ce_count:2
> /sys/devices/system/edac/mc/mc0/dimm0/dimm_ce_count:1
> /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count:1
> /sys/devices/system/edac/mc/mc0/dimm4/dimm_ce_count:1
> /sys/devices/system/edac/mc/mc0/dimm6/dimm_ce_count:2
> /sys/devices/system/edac/mc/mc1/ce_count:4
> /sys/devices/system/edac/mc/mc1/csrow0/ce_count:1
> /sys/devices/system/edac/mc/mc1/csrow0/ch0_ce_count:1
> /sys/devices/system/edac/mc/mc1/csrow3/ce_count:1
> /sys/devices/system/edac/mc/mc1/csrow3/ch0_ce_count:1
> /sys/devices/system/edac/mc/mc1/csrow6/ce_count:2
> /sys/devices/system/edac/mc/mc1/csrow6/ch0_ce_count:2
> /sys/devices/system/edac/mc/mc1/dimm0/dimm_ce_count:1
> /sys/devices/system/edac/mc/mc1/dimm3/dimm_ce_count:1
> /sys/devices/system/edac/mc/mc1/dimm6/dimm_ce_count:2
>
>
> v2 updates:
>
> * rebased onto bp/for-next (b2572772d13e: EDAC: Make
> edac_debugfs_create_x*() return void),
>
> * added patches to fix grain calculation (put this at the beginning
> of the series to apply them separately),
>
> * modified sysfs init functions based (EDAC, mc: Fix and improve
> sysfs init functions) on Greg's fixes (f5d59da9663d EDAC/sysfs:
> Drop device references properly),
>
> * removed duplicate code for mem_info_setup*() by moving it to
> ghes_dimm_info_init(),
>
> * fix bisecting of series,
>
> * made mem_info static,
>
> * renamed function mci_add_dimm_info() to mem_info_prepare_mci(),
>
> * added patch to move struct member smbios_handle to struct
> ghes_dimm_info,
>
> * renamed ghes_mem_info.num_per_node[] to
> ghes_mem_info.dimms_per_node[],
>
> * removed unused mem_info.enable_numa,
>
> * removed unused mem_info.num_nodes,
>
> * fixed dimm counters after sysfs reset_counters.
>
>
> Robert Richter (24):
> EDAC, mc: Fix grain_bits calculation
> EDAC, ghes: Fix grain calculation
> EDAC, ghes: Remove pvt->detail_location string
> EDAC, ghes: Unify trace_mc_event() code with edac_mc driver
> EDAC, mc: Fix and improve sysfs init functions
> EDAC: Kill EDAC_DIMM_PTR() macro
> EDAC: Kill EDAC_DIMM_OFF() macro
> EDAC: Introduce mci_for_each_dimm() iterator
> EDAC, mc: Cleanup _edac_mc_free() code
> EDAC, mc: Remove per layer counters
> EDAC, mc: Rework edac_raw_mc_handle_error() to use struct dimm_info
> EDAC, ghes: Use standard kernel macros for page calculations
> EDAC, ghes: Add support for legacy API counters
> EDAC, ghes: Rework memory hierarchy detection
> EDAC, ghes: Extract numa node information for each dimm
> EDAC, ghes: Moving code around ghes_edac_register()
> EDAC, ghes: Create one memory controller device per node
> EDAC, ghes: Fill sysfs with the DMI DIMM label information
> EDAC, mc: Introduce edac_mc_alloc_by_dimm() for per dimm allocation
> EDAC, ghes: Identify dimm by node, card, module and handle
> EDAC, ghes: Enable per-layer reporting based on card/module
> EDAC, ghes: Move struct member smbios_handle to struct ghes_dimm_info
> EDAC, Documentation: Describe CPER module definition and DIMM ranks
> EDAC, ghes: Disable legacy API for ARM64
>
> Documentation/admin-guide/ras.rst | 31 +-
> drivers/edac/edac_mc.c | 385 ++++++++++---------
> drivers/edac/edac_mc.h | 33 +-
> drivers/edac/edac_mc_sysfs.c | 95 ++---
> drivers/edac/ghes_edac.c | 609 +++++++++++++++++++++++-------
> drivers/edac/i10nm_base.c | 3 +-
> drivers/edac/i3200_edac.c | 3 +-
> drivers/edac/i5000_edac.c | 5 +-
> drivers/edac/i5100_edac.c | 14 +-
> drivers/edac/i5400_edac.c | 4 +-
> drivers/edac/i7300_edac.c | 3 +-
> drivers/edac/i7core_edac.c | 3 +-
> drivers/edac/ie31200_edac.c | 7 +-
> drivers/edac/pnd2_edac.c | 4 +-
> drivers/edac/sb_edac.c | 2 +-
> drivers/edac/skx_base.c | 3 +-
> drivers/edac/ti_edac.c | 2 +-
> include/linux/edac.h | 141 ++++---
> 18 files changed, 842 insertions(+), 505 deletions(-)
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 52+ messages in thread