From: Borislav Petkov <bp@alien8.de>
To: Gregor Herburger <gregor.herburger@ew.tq-group.com>
Cc: york.sun@nxp.com, mchehab@kernel.org, tony.luck@intel.com,
james.morse@arm.com, rrichter@marvell.com,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] edac: fsl_ddr_edac: fix expected data message
Date: Thu, 3 Sep 2020 12:58:49 +0200 [thread overview]
Message-ID: <20200903105849.GC5462@zn.tnic> (raw)
In-Reply-To: <20200827075600.22335-1-gregor.herburger@ew.tq-group.com>
On Thu, Aug 27, 2020 at 09:56:00AM +0200, Gregor Herburger wrote:
> When a correctable single bit error occurs, the driver calculates the
> bad_data_bit respectively the bad_ecc_bit. If there is no error in the
> corresponding data, the value becomes -1. With this the expected data
> message is calculated.
>
> In the case of an error in the lower 32 bits or no error (-1) the right
> side operand of the bit-shift becomes negative which is undefined
> behavior.
>
> This can result in wrong and misleading messages like this:
> [ 311.103794] EDAC FSL_DDR MC0: Faulty Data bit: 36
> [ 311.108490] EDAC FSL_DDR MC0: Expected Data / ECC: 0xffffffef_ffffffff / 0x80000059
> [ 311.116135] EDAC FSL_DDR MC0: Captured Data / ECC: 0xffffffff_ffffffef / 0x59
>
> Fix this by only calculating the expected data where the error occurred.
>
> With the fix the dmesg output looks like this:
> [ 311.103794] EDAC FSL_DDR MC0: Faulty Data bit: 36
> [ 311.108490] EDAC FSL_DDR MC0: Expected Data / ECC: 0xffffffef_ffffffef / 0x59
> [ 311.116135] EDAC FSL_DDR MC0: Captured Data / ECC: 0xffffffff_ffffffef / 0x59
>
> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> ---
> drivers/edac/fsl_ddr_edac.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 6d8ea226010d..4b6989cf1947 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -343,9 +343,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>
> fsl_mc_printk(mci, KERN_ERR,
> "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> - cap_high ^ (1 << (bad_data_bit - 32)),
> - cap_low ^ (1 << bad_data_bit),
> - syndrome ^ (1 << bad_ecc_bit));
> + (bad_data_bit > 31) ? cap_high ^ (1 << (bad_data_bit - 32)) : cap_high,
> + (bad_data_bit <= 31) ? cap_low ^ (1 << (bad_data_bit)) : cap_low,
But if bad_data_bit is -1, this check above will hit and you'd still
shift by -1, IINM.
How about you fix it properly, clean it up and make it more readable in
the process (pasting the code directly instead of a diff because a diff
is less readable):
if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
sbe_ecc_decode(cap_high, cap_low, syndrome,
&bad_data_bit, &bad_ecc_bit);
if (bad_data_bit != -1) {
if (bad_data_bit > 31)
cap_high ^= 1 << (bad_data_bit - 32);
else
cap_low ^= 1 << bad_data_bit;
fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
fsl_mc_printk(mci, KERN_ERR, "Expected Data: %#8.8x_%08x\n",
cap_high, cap_low);
}
if (bad_ecc_bit != -1) {
fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
fsl_mc_printk(mci, KERN_ERR, "Expected ECC: %#2.2x\n",
syndrome ^ (1 << bad_ecc_bit));
}
}
This way you print only when the respective faulty bits have been
properly found and not print anything otherwise.
Hmm?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
next prev parent reply other threads:[~2020-09-03 11:10 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-24 11:18 [PATCH 1/1] edac: fsl_ddr_edac: fix expected data message Gregor Herburger
2020-08-17 9:53 ` Borislav Petkov
2020-08-27 7:56 ` [PATCH v2 " Gregor Herburger
2020-09-03 10:58 ` Borislav Petkov [this message]
2020-09-04 6:52 (EXT) " Gregor Herburger
2020-09-04 9:17 ` Borislav Petkov
2020-09-04 13:32 ` Gregor Herburger
2020-09-08 19:24 ` Borislav Petkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200903105849.GC5462@zn.tnic \
--to=bp@alien8.de \
--cc=gregor.herburger@ew.tq-group.com \
--cc=james.morse@arm.com \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=rrichter@marvell.com \
--cc=tony.luck@intel.com \
--cc=york.sun@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).