Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 4 Sep 2020 11:17:18 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Gregor Herburger <Gregor.Herburger@ew.tq-group.com>
Cc:     "york.sun@nxp.com" <york.sun@nxp.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        "james.morse@arm.com" <james.morse@arm.com>,
        "rrichter@marvell.com" <rrichter@marvell.com>,
        "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: (EXT) Re: [PATCH v2 1/1] edac: fsl_ddr_edac: fix expected data
 message
Message-ID: <20200904091718.GC21499@zn.tnic>
References: <kcEE.e0qfoTd8SOOr3lTVWaXz/A.AASg8YeC1gE@vtuxmail01.tq-net.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <kcEE.e0qfoTd8SOOr3lTVWaXz/A.AASg8YeC1gE@vtuxmail01.tq-net.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Your mail client broke threading...

On Fri, Sep 04, 2020 at 06:52:24AM +0000, Gregor Herburger wrote:

> The cap_low, cap_high and syndrome are used in the printk following the if-Block.
> This will make expected data / captured data look the same.

Right.

> @@ -334,18 +337,32 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>                 sbe_ecc_decode(cap_high, cap_low, syndrome,
>                                 &bad_data_bit, &bad_ecc_bit);
> 
> +               exp_high = cap_high;
> +               exp_low = cap_low;
> +               exp_syndrome = syndrome;
> +
>                 if (bad_data_bit != -1)
> +               {

Opening brace is on the same line for if-statements.

>                         fsl_mc_printk(mci, KERN_ERR,
>                                 "Faulty Data bit: %d\n", bad_data_bit);
> +
> +                       if (bad_data_bit < 32)
> +                               exp_low = cap_low ^ (1 << bad_data_bit);
> +                       else
> +                               exp_high = cap_high ^ (1 << (bad_data_bit - 32));
> +               }
> +
>                 if (bad_ecc_bit != -1)
> +               {

Ditto.

>                         fsl_mc_printk(mci, KERN_ERR,
>                                 "Faulty ECC bit: %d\n", bad_ecc_bit);
> 
> +                       exp_syndrome = syndrome ^ (1 << bad_ecc_bit);
> +               }
> +
>                 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));
> +                       exp_high, exp_low, exp_syndrome);
>         }
> 
>           fsl_mc_printk(mci, KERN_ERR,
>                           "Captured Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
>                           cap_high, cap_low, syndrome);
> 
> How about something like this?

My only concern here is that you'll be printing "Expected Data ..."
unconditionally even if either or both - bad_data_bit and bad_ecc_bit
- are -1.

If the driver cannot decode the data and/or ECC syndrome bits, then it
should say so - not dump expected data and claim that it is a valid
information.

So maybe in addition to the above:

	if (bad_data_bit != -1) {
		...
	} else {
		fsl_mc_printk(..., "Unable to decode the Faulty Data bit");
	}

and the same for the ECC bit.

And then print only the expected data for the bit which sbe_ecc_decode()
found correctly and not say anything otherwise.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette